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

Use a less timing-sensitive condition to hopefully fix flaky test #1125

Merged
merged 2 commits into from Apr 10, 2019

Conversation

Projects
None yet
1 participant
@nathansobo
Copy link
Contributor

commented Apr 10, 2019

Jasmine 1.3 queueing makes async testing weird. In this case, we resolve a promise with the expectation that it asynchronously displays a notification. Then, in a waitsFor block on the next line, we subscribe to onDidAddNotification. The problem is that this waitsFor block might not even be run before the notification gets displayed. So the notification is displayed before we even subscribe to onDidAddNotification and the waitsForBlock gets stuck. This kind of thing seems especially frequent on Windows/AppVeyor.

The ideal fix would be to convert all these tests to JS and use async/await. Then we could synchronously subscribe to onDidAddNotification after resolving the promise and avoid this race. But in this case, I think using a looser condition should work as well.

@nathansobo

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

/cc @atom/core @50Wliu because I think it's important that everyone understands this source of indeterminacy when combining Jasmine 1.3 waitsFor blocks with promises in async tests.

@nathansobo nathansobo merged commit 624a491 into master Apr 10, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@nathansobo nathansobo deleted the fix-flaky-test branch Apr 10, 2019

@nathansobo nathansobo self-assigned this Apr 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.