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 |
asm89
left a comment
asm89
left a comment
There was a problem hiding this comment.
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.
nit: should this live in Rx\Testing instead? It depends on the TestScheduler?
src/Testing/ColdObservable.php
Outdated
| $isDisposed = false; | ||
| $index = null; | ||
|
|
||
| if (!($observer instanceof MockHigherOrderObserver)) { |
There was a problem hiding this comment.
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.
OnNextNotification does not live in testing, but is now depending on this testing helpers file.
There was a problem hiding this comment.
Could the dependencies be inversed? E.g. using ->accept() on the notifications from the testing code?
There was a problem hiding this comment.
@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 |
5 participants
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?