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: force Mocha to exit on CI #5862

Merged
merged 1 commit into from May 13, 2020
Merged

chore: force Mocha to exit on CI #5862

merged 1 commit into from May 13, 2020

Conversation

jackfranklin
Copy link
Collaborator

We have some bug where some test runs will pass but then stall as Mocha
doesn't exit cleanly. This is proving very hard to track down (I've yet
to replicate it or find the test that causes it) and it doesn't happen
consistently.

The exit flag forces Mocha to hard exit. This will help with CI
stability. The flag only gets set on CI runs.

We have some bug where some test runs will pass but then stall as Mocha
doesn't exit cleanly. This is proving very hard to track down (I've yet
to replicate it or find the test that causes it) and it doesn't happen
consistently.

The `exit` flag forces Mocha to hard exit. This will help with CI
stability. The flag only gets set on CI runs.
@jackfranklin jackfranklin merged commit 0aba6df into master May 13, 2020
@jackfranklin jackfranklin deleted the force-mocha-exit branch May 13, 2020 10:10
@AviVahl
Copy link
Contributor

AviVahl commented May 13, 2020

Please note, this flag is really dangerous. It obscures real issues and allows other "failing to cleanup" issues to slip in while turned on.

jsdom had it on for a while (removed it now), and it allowed a change where requestAnimationFrame caused a stale process. They didn't know anything about it, as the flag caused mocha to always exit.

@jackfranklin @mathiasbynens

Writing this, as I myself have been bitten by never-closing processes on puppeteer@2.1.0-2.1.1.

@mathiasbynens
Copy link
Member

Thanks for sharing those insights @AviVahl, that's good to know.

Ideally we'd find the cause for the flaky stalling but as Jack pointed out this is proving difficult. Until we find the cause and fix it, we're torn between a flaky CI (without this PR) or a less-flaky CI that might hide issues. :/ I'm not sure what the best way is to proceed here. WDYT, @jackfranklin?

@jackfranklin
Copy link
Collaborator Author

Fully aware this isn't good, but at this point it's a trade off of flakey CI that have we have to kick vs hiding a potential issue.

I'm lost in terms of debugging this further. I'd like to see if this does make our CI less flakey and if so it might be a trade-off worth taking for a bit. If it doesn't, we'll ditch it.

We don't run this flag locally so hopefully we don't introduce any consistent issues via this locally as people won't have this flag on.

@AviVahl
Copy link
Contributor

AviVahl commented May 15, 2020

Using wtfnode, we saw some hanging websockets in our setup when we debugged this.

When checking the current code, I found a something odd:
https://github.com/puppeteer/puppeteer/blob/v3.0.4/src/WebSocketTransport.ts#L57
I would expect that line to say:

return new Promise(resolve => this._ws.close(resolve))

and whoever calling close() to await the returned promise.

WDYT? could that be a/the reason for the stale process?

ref: https://github.com/websockets/ws/blob/7.3.0/doc/ws.md#serverclosecallback

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants