fixing observable #41
Conversation
|
I've also added the proposed fixes, still WIP, waiting for your commnets |
|
I'll have a look at this soonish |
|
The last commit deals with the fact that the methods returning Observable calling the api endpoints before any subscriptions. This change allows to greatly simply the implementation of FaceAsyncSubject as it won't have to deal with async complications. Inspired by https://github.com/square/retrofit/blob/master/retrofit/src/main/java/retrofit/RxSupport.java |
| @@ -36,6 +36,9 @@ | |||
| <Reference Include="Castle.Core"> | |||
| <HintPath>..\ext\Net45\Castle.Core.dll</HintPath> | |||
| </Reference> | |||
| <Reference Include="Moq"> | |||
There was a problem hiding this comment.
Nooooooope. Using Moq to test Rx is a recipe to write bad tests because it's really hard to correctly mock Rx behavior, just use Rx in the tests.
There was a problem hiding this comment.
Ok, I will add a poco MockObserver and use that, just to get this merged.
I do not agree with your statement, if you look at the test, Moq makes perfect sens in this context. I do agree with the fact that overusing mocks is a bad thing, I try to avoid it at all costs.
There was a problem hiding this comment.
@Balauru You probably want Observer.Create
There was a problem hiding this comment.
@flagbug I don't think so, I will have to do some assertions and we don't have the rx library included, we can only depend on the System.IObserver
There was a problem hiding this comment.
You can totally add rx-main and rx-testing to the tests (and should!).
There was a problem hiding this comment.
I've update the tests and removed Moq
So, this change changes the semantics of how Observable methods work in Refit:
While I'd perhaps argue that we should switch to After, we're definitely changing the existing behavior, which means we should save that for Refit 2.0 |
|
The first 3 commits, deal with the Before functionality, that was not working, I've written failing unit tests for it. Only the last commit changes the functionality. Since it was not working in the first place, I doubt there was someone using it and thus it does not make sens to delay it until version 2.0. |
|
Hey @Balauru, thanks for hacking on this - I fixed it a different way, but it should definitely work now. I'm going to stick with the AsyncSubject-based approach for now but I might change this in 2.0 |
|
I am not giving up :), I've send a new pull request that does not break backward compatibility https://github.com/paulcbetts/refit/pull/50 |
I am trying to fix observable,
I've added a failing test to demonstrate is not working as expected. The test is expecting
OnNextto be called in 2 seconds.For what I can figure out so far
FakeAsyncSubjectis not working as expected,onNextbody is never executed because completion is null,OnCompletewill throw and exception because final will be null.Do you think this is a good approach shall I continue to investigate and come up with a fix?