-
Notifications
You must be signed in to change notification settings - Fork 3k
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 every tests to run mode #5860
Conversation
I saw the PR title and thought you'd updated every operator in a single PR 😲😂 |
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.
This is all okay, but I don't think we should be changing of()
to cold()
. See my comment below.
}); | ||
|
||
it('should emit true if Scalar source matches with predicate', () => { | ||
const source = of(5); |
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.
issue: Probably best to not change any sources that are currently declared using of()
to cold()
, as there is a subtle difference - see #5760 and its associated issue for background.
Is this something that you've done in other PRs? I'm wondering whether or not I missed it in any of the other reviews.
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.
OK, I will change this, but I have been changing this in my other PRs as well. Should I revert from other tests?
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.
Oh, didn't see that you already asked... Yes, I was changing in other PRs as well. I'll make a revert PR.
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.
@cartant I started reading #5760 and I found it to be quite large to me. I'll give it try next time, but for now, out of curiosity, I'd like to know what would e.g. cold('a')
represent? This:
new Observable(subscriber => subscriber.next('a'))
or this:
new Observable(subscriber => setTimeout(() => subscriber.next('a'), 0))
Is #5760 trying change behavior from one to another maybe?
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.
Not quite. cold('a')
represents:
new Observable(subscriber => {
subscriber.next('a');
})
Note that there is no completion.
of('a')
is:
new Observable(subscriber => {
subscriber.next('a');
subscriber.complete();
})
But cold('(a|)')
is:
new Observable(subscriber => {
subscriber.next('a');
asapScheduler.schedule(() => subscriber.complete());
})
It's not possible - ATM - to mimic of
exactly with cold
, so we should be careful replacing of
. We should just convert the tests to run mode and leave their implementations as is.
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.
Updated PR, I think I reverted everything. Other reverts coming...
I didn't know about this subtle distinction. Thanks a lot for explanations Nicholas. Any chance to explain why asapScheduler
is used for completion notifications? If easier, you can post a link to an issue or to a source code.
Is completion notification scheduled on asapScheduler
only for frame 0 or for every frame?
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.
Thanks for the update.
Actually, what I said above was wrong. All notifications are emitted on the test scheduler, so it's kinda equivalent to:
new Observable(subscriber => {
asapScheduler.schedule(() => subscriber.next('a'));
asapScheduler.schedule(() => subscriber.complete());
})
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.
LGTM. Thanks.
Updated
every
tests to use run mode.