Skip to content
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

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

davidwdan
Copy link
Member

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?

@martinsik
Copy link
Contributor

martinsik commented Mar 11, 2017

@davidwdan Can you give me write permissions to your marbleTest branch?

@davidwdan
Copy link
Member Author

@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 onNext helper function, it works.

@coveralls
Copy link

coveralls commented Mar 11, 2017

Coverage Status

Coverage decreased (-0.09%) to 99.908% when pulling 08b0e0a on davidwdan:marbleTest into 23e32ca on ReactiveX:2.x.

Copy link
Member

@asm89 asm89 left a 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
Copy link
Member

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?

$currentObservable = $this;
$disposable = new CompositeDisposable();
$scheduler = $this->scheduler;
$isDisposed = false;
$index = null;

if (!($observer instanceof MockHigherOrderObserver)) {
Copy link
Member

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
@davidwdan
Copy link
Member Author

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 automatic_mock_observer_doesnt_create_loggable_subscription is failing, but I not sure if it's still needed or when those inner subscriptions should start and stop.

return $x;
}

function materializeObservable(Observable $observable): array
Copy link
Member

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.

Copy link
Member

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?

Copy link
Member Author

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?

@coveralls
Copy link

coveralls commented Mar 12, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling ee4ec61 on davidwdan:marbleTest into 23e32ca on ReactiveX:2.x.

@martinsik
Copy link
Contributor

martinsik commented Mar 12, 2017

@davidwdan That test is irrelevant because now the MockObserver automatically subscribes only when calling OnNextNotification:: __toString().

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 __toString() calls on $xs or $ys.

Added a higher-order marble test
@davidwdan
Copy link
Member Author

I added a groupBy marble test that is currently failing. We may have to change where the inner observable is materialized

@mbonneau mbonneau changed the base branch from 2.x to master March 14, 2017 23:35
@davidwdan davidwdan added this to the 2.1 milestone Mar 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants