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

chore: update exhaust tests to run mode #5866

Merged
merged 1 commit into from Oct 30, 2020

Conversation

jakovljevic-mladen
Copy link
Member

Updated exhaust tests to use run mode.

@cartant, I've got two issues/questions here:

  • The first test should handle a hot observable of hot observables is exactly the same as test should handle a hot observable of observables with the exception of expectSubscriptions calls. I was usually adding expectSubscriptions calls for those tests that didn't have them and wanted to do the same for this test, but then later I found out that they're the same. What I'd suggest is to transform this test to the one which would really satisfy what it says in the title (now, it doesn't handle observable of hot observables, but rather cold ones). I'd add expectSubscriptions where possible. Would you agree? If yes, is it OK to do it with this PR?
  • I'd inline empty arrays. E.g. instead of:
const y = cold('---d--e---f---|    ');
const ysubs: string[] = [];
const z = cold('     ---g--h--i---|');
expectSubscriptions(y.subscriptions).toBe(ysubs);

use like this:

const y = cold('---d--e---f---|    ');
const z = cold('     ---g--h--i---|');
expectSubscriptions(y.subscriptions).toBe([]);

I think that users could have more troubles wanting to navigate between adjacent marbles without having enough whitespace after the const ysubs line ends. What do you think about inlining them? Or, if inline is not an option (for whatever reason), I'd at least put them after all marbles strings.

@benlesh
Copy link
Member

benlesh commented Oct 30, 2020

The first test was likely used to generate a marble diagram with an automated tool we used to use.

@benlesh
Copy link
Member

benlesh commented Oct 30, 2020

I'm relatively indifferent on the location of the empty arrays for subscription. On one hand, we usually keep the subscription marbles next to the observable marbles they are related to, so putting the empty array there makes sense. It says "no subscriptions". But I guess if we wanted to be that explicit, we could do: const subs = ['------------------------------'] or something.

On the other hand, I don't think it's any more or less readable either way.

I guess I'd just say make sure it's consistent with the rest of the tests.

@benlesh
Copy link
Member

benlesh commented Oct 30, 2020

Overall, I think this is good to merge as is.

@benlesh benlesh merged commit 0eff973 into ReactiveX:master Oct 30, 2020
@benlesh
Copy link
Member

benlesh commented Oct 30, 2020

Thanks again, @jakovljevic-mladen! Very much appreciated!

@jakovljevic-mladen
Copy link
Member Author

The first test was likely used to generate a marble diagram with an automated tool we used to use.

Yes, most probably. Many tests suites don't expectSubscriptions in the first test, I was wandering why. I guess this is why 😃

Thank you Ben for the review.

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.

None yet

2 participants