-
Notifications
You must be signed in to change notification settings - Fork 140
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] Higher-Order Marble tests #153
base: master
Are you sure you want to change the base?
Conversation
@davidwdan Can you give me write permissions to your |
@martinsik you have permission now. I think the issue is that in some circumstances there are multiple test schedulers. If I hack it and pass the test scheduler through the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm probably missing something, but I'm not a fan of the instanceof
checks scattered all over the code. Is there no other way to achieve this?
use Rx\Testing\MockHigherOrderObserver; | ||
use Rx\Testing\TestScheduler; | ||
|
||
class OnNextObservableNotification extends OnNextNotification |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: should this live in Rx\Testing
instead? It depends on the TestScheduler
?
src/Testing/ColdObservable.php
Outdated
$currentObservable = $this; | ||
$disposable = new CompositeDisposable(); | ||
$scheduler = $this->scheduler; | ||
$isDisposed = false; | ||
$index = null; | ||
|
||
if (!($observer instanceof MockHigherOrderObserver)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need all these instanceof
checks? Looks wrong.
…on to a string, so we only need to check once if the value is an instance of an observable
Inner observables are now materialized when being converted to a string. This simplifies things and gets rid of the need to check the instance of the observers. The test |
return $x; | ||
} | ||
|
||
function materializeObservable(Observable $observable): array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OnNextNotification
does not live in testing
, but is now depending on this testing helpers file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could the dependencies be inversed? E.g. using ->accept()
on the notifications from the testing code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@asm89 ACK I'll refactor it in a bit. Aren't we planning to move some of the test helpers out of the testing namespace?
@davidwdan That test is irrelevant because now the It was previously used in situations where we don't want to log subscriptions. For example in tests such as MergeAllTest.php#L14 where it's not a problem anymore because there are no |
Added a higher-order marble test
I added a |
I cherry picked the higher-order test code from v1 so we can start working on getting higher-order marble tests working.
Some tests are failing.
@martinsik if you get a chance, can you look at the failing tests?