Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

fix subscribeToEvents test #4166

Merged
merged 4 commits into from
Jan 16, 2017
Merged

fix subscribeToEvents test #4166

merged 4 commits into from
Jan 16, 2017

Conversation

derhuerst
Copy link
Contributor

This PR is a follow-up of #4138. It fixes the flaky test for util/subscribe-to-events.

It is not the most beautiful way to test this though. I would have preferred to use fake timers but couldn't get it to work in a reasonable time.

This fix is a workaround. I would have used sinon fake timers, but I
couldn't get them to work.
@derhuerst derhuerst added A0-pleasereview 🤓 Pull request needs code review. M7-ui labels Jan 13, 2017
.on('log', onLog)
.on('Bar', onBar);
await delay(9);
s.unsubscribe();
await delay(10);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to be sure promise are resolved or event emitted before testing the expectations, currently everything just waits for some hopefully long enough time instead.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to be sure promise are resolved or event emitted […]

The promises definitely resolve on next tick, because they are mocked.

[…] currently everything just waits for some hopefully long enough time instead.

That the events fire within a certain time period is actually what this test tries to assert.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 98300d3 on jr-fix-subscribe-test into ** on master**.

@jacogr
Copy link
Contributor

jacogr commented Jan 16, 2017

Not 100% on-board with approach, however it is an test and it does seem to be at least semi-stable at this point. Not going to gumble about it, we have real issues to address.

@jacogr jacogr added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jan 16, 2017
@gavofyork gavofyork merged commit 1d618fa into master Jan 16, 2017
@gavofyork gavofyork deleted the jr-fix-subscribe-test branch January 16, 2017 09:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants