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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix flaky test re: "toggling when the project has multiple paths" 馃 #347

Merged
merged 5 commits into from May 17, 2018

Conversation

Projects
None yet
2 participants
@jasonrudolph
Member

jasonrudolph commented May 16, 2018

Prior to this change, I was able to reliably reproduce the failures described in atom/atom#17325 when running apm test locally with Atom 1.29.0-dev-3e4b386. After this change, the tests reliably pass for me. Hopefully this resolves the flakiness described in atom/atom#17325. 馃槄

Don't mix await and waitsFor
Prior to this change, I could reliably reproduce the failures described
in  atom/atom#17325. After this change, the
test reliably passes.

@jasonrudolph jasonrudolph requested review from daviwil and maxbrunsfeld May 16, 2018

@jasonrudolph jasonrudolph changed the title from Fix flaky test re "toggling when the project has multiple paths" 馃 to Fix flaky test re: "toggling when the project has multiple paths" 馃 May 16, 2018

socketServer = net.createServer(() => {})
socketPath = path.join(rootDir1, 'some.sock')
waitsFor(done => socketServer.listen(socketPath, done))
await socketServer.listen(socketPath)

This comment has been minimized.

@maxbrunsfeld

maxbrunsfeld May 16, 2018

Contributor

Looking at the docs for Server.listen, it looks like the method just returns the Server itself, and not a Promise. So I would think that using await on the return value would be a noop.

It doesn't look like close returns a promise either, so await should be a noop there too.

I would think that you'd have to pass a callback to both methods; something like:

await new Promise(resolve => socketServer.listen(socketPath, resolve))

// ...

await new Promise(resolve => socketServer.close(resolve))

It's interesting that this fixes the tests though. Maybe the listening and closing are so fast that they already complete before the next block executes?

jasonrudolph and others added some commits May 17, 2018

Revert "Don't mix await and waitsFor"
Mixing await and waitsFor was not the cause of the problem, so revert
f11ad5e.

Co-authored-by: Max Brunsfeld <maxbrunsfeld@github.com>
Don't let test fail just because it takes too long to close the server
Co-Authored-By: Max Brunsfeld <maxbrunsfeld@github.com>
馃帹 Tweak test to clarify intent
This test was introduced in 183ec61 to
resolve #76 and #77.

Co-Authored-By: Max Brunsfeld <maxbrunsfeld@github.com>

@jasonrudolph jasonrudolph merged commit 89ec5d7 into master May 17, 2018

2 checks passed

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

@jasonrudolph jasonrudolph deleted the fix-atom-17325 branch May 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment