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

fix: handle open handles from inside tests #6263

Merged
merged 6 commits into from
Aug 9, 2018

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented May 25, 2018

Summary

@thymikee found this one before the tweet storm yesterday 😅

Test plan

Added test


const stacks = new Set();

const uniqueErrors = formatted.filter(handle => {
Copy link
Member Author

@SimenB SimenB May 25, 2018

Choose a reason for hiding this comment

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

this whole filter is really hairy. ideas?

The thought is to avoid 2 stack traces pointing to the same thing:
image

Copy link
Collaborator

@thymikee thymikee May 25, 2018

Choose a reason for hiding this comment

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

Any difference about Timeout and TIMERWRAP?

Copy link
Member Author

Choose a reason for hiding this comment

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

IDK. TIMEWRAP is a libuv thing, that's what I got from googling it. I don't think it really matters, they point to the same thing

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or maybe we can just ditch the filtering?

Copy link
Member Author

Choose a reason for hiding this comment

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

@cpojer wanted to filter out the dupe

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you just add a comment about the dupes? Should be good enough

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@@ -39,7 +39,7 @@ function promisifyLifeCycleFunction(originalFn, env) {

// We make *all* functions async and run `done` right away if they
// didn't return a promise.
const asyncFn = function(done) {
const asyncJestLifecycle = function(done) {
Copy link
Member Author

Choose a reason for hiding this comment

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

this seemed easiest 🤷‍♂️

Copy link
Collaborator

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

LGTM

@thymikee
Copy link
Collaborator

Some weird errors happening on Node 10

@SimenB
Copy link
Member Author

SimenB commented May 25, 2018

Yeah, node 10.2.0 is messed up. nodejs/node#20921

@SimenB SimenB force-pushed the open-handles-in-test branch 2 times, most recently from 3d28fb1 to 24606f3 Compare May 26, 2018 17:26
@SimenB
Copy link
Member Author

SimenB commented May 27, 2018

I need to fix this for circus

@SimenB SimenB force-pushed the open-handles-in-test branch 3 times, most recently from a92dae2 to fed0667 Compare May 27, 2018 09:23
@SimenB
Copy link
Member Author

SimenB commented May 27, 2018

@aaronabramov I added a second commit attempting to fix this for circus, but I still get some jest internals I don't care about. Current results is this:
image

Ideas on how to detect just stuff created in user code, and avoid the recursive calls circus itself does?

if (error.stack.includes('Runtime.requireModule')) {
if (
error.stack.includes('Runtime.requireModule') ||
(error.stack.includes('callAsyncCircusFn') &&
Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldn't this be || instead of &&?

Copy link
Member Author

@SimenB SimenB May 27, 2018

Choose a reason for hiding this comment

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

I pushed a new commit extracting this into a function where I think the logic (that doesn't really work...) is easier to follow

@aaronabramov
Copy link
Contributor

wouldn't this always be triggered from somewhere in user's code though?
i'm not familiar with the whole error handling process and probably don't understand what's happening :(


// E.g. timeouts might give multiple traces to the same line of code
// This hairy filtering tries to remove entries with duplicate stack traces
const uniqueErrors = formatted.filter(handle => {
Copy link
Contributor

Choose a reason for hiding this comment

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

should we exatract this into a separate module?
i a little worried about introducing so much complex logic right in index.js :P

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, I can extract it

Copy link
Member Author

Choose a reason for hiding this comment

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

done now

@SimenB
Copy link
Member Author

SimenB commented May 27, 2018

wouldn't this always be triggered from somewhere in user's code though?

Basically, we track absolutely everything async happening in the process. But a lot of async stuff is expected such as triggering describes and its (creates promises), so we should filter out those false positives so that we're only left with async stuff the user initiated

@aaronabramov
Copy link
Contributor

@SimenB can we attach an extra flag to it when we trigger them?
like pass an asyncError with circus_internal = true?

@thymikee
Copy link
Collaborator

thymikee commented Aug 8, 2018

@SimenB internal promises should be resolved after #6716 is merged, right?

@SimenB
Copy link
Member Author

SimenB commented Aug 8, 2018

Good point, that might actually solve my issue. That's awesome. I'll rebase after it lands 🙂

@thymikee
Copy link
Collaborator

thymikee commented Aug 8, 2018

@SimenB, just merged #6716

@SimenB
Copy link
Member Author

SimenB commented Aug 9, 2018

Rebased, and looks good locally

@SimenB
Copy link
Member Author

SimenB commented Aug 9, 2018

The dot reporter should include the normal summary as well... The test passes locally, no idea why it fails on CI.

image

@SimenB
Copy link
Member Author

SimenB commented Aug 9, 2018

@SimenB
Copy link
Member Author

SimenB commented Aug 9, 2018

why-is-node-running removes TimerWrap, so I'll just do the same here.

https://github.com/mafintosh/why-is-node-running/blob/8f7908d31bd24b485cce218ce5552b8c75f24010/index.js#L10

@SimenB
Copy link
Member Author

SimenB commented Aug 9, 2018

Green! 🎉

This reverts commit 176a9c1.
@SimenB SimenB merged commit 0099f07 into jestjs:master Aug 9, 2018
@SimenB SimenB deleted the open-handles-in-test branch August 9, 2018 13:48
@thymikee
Copy link
Collaborator

thymikee commented Aug 9, 2018

🎉

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants