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

Update jest-leak-detector package to be compatible with the new Node version #8686

Merged
merged 4 commits into from
Aug 19, 2019

Conversation

lh0x00
Copy link
Contributor

@lh0x00 lh0x00 commented Jul 13, 2019

At #8397, we know weak package doesn't work in Node version 12 and we use weak-napi package instead of weak in new version. weak-napi have interface like weak package but it still have some differences suck as:

GC callbacks are invoked at a suitable time (after the actual GC run has finished). This is nice because it means your process won’t potentially crash.

The consequence of this is that the isLeaking function in jest-leak-detector package return value must be converted into a promise to wait a bit time.

Fixes #8397

lh0x00 added a commit to lh0x00/jest that referenced this pull request Jul 13, 2019
@lh0x00 lh0x00 mentioned this pull request Jul 13, 2019
Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

We should remove the conditional require now, which'll necessitate typings.

Since we're changing the signature, that also means this is a breaking change... I see CI fails for node 6, but since it's breaking we'll just drop node 6 anyways

@lh0x00 lh0x00 force-pushed the feature/Issue-8397 branch from be7a7c0 to 05cb207 Compare July 17, 2019 05:34
lh0x00 added a commit to lh0x00/jest that referenced this pull request Jul 17, 2019
@lh0x00
Copy link
Contributor Author

lh0x00 commented Jul 17, 2019

@SimenB @jeysal DT was released type for weak-napi package at @types/weak-napi and I added it. PR was updated.

@SimenB SimenB added this to the Jest 25 milestone Jul 17, 2019
@SimenB
Copy link
Member

SimenB commented Jul 17, 2019

Hmm, something seems to be off with CI - Circle runs on your account instead of Jest's. Not sure how that happened

@lh0x00
Copy link
Contributor Author

lh0x00 commented Jul 17, 2019

how I fix it? :<
I turned off CircleCI in my repo, please trigger run the build again.

@jeysal
Copy link
Contributor

jeysal commented Jul 17, 2019

Noticed Circle doing the same for #8689, perhaps it only worked there because I have the necessary permissions. Maybe it works if you remove your fork from the list of projects that Circle builds.

@lh0x00
Copy link
Contributor Author

lh0x00 commented Jul 17, 2019

@jeysal I have removed it from list in my CircleCI. Please trigger run build agian!

image

@jeysal
Copy link
Contributor

jeysal commented Jul 17, 2019

I don't know if we can rerun if there's no build for this PR in the history. Can you try to

git commit --amend --no-edit && git push -f

@lh0x00 lh0x00 force-pushed the feature/Issue-8397 branch from 05cb207 to d282052 Compare July 17, 2019 07:17
@lh0x00
Copy link
Contributor Author

lh0x00 commented Jul 17, 2019

I did it and waiting to CircleCI re-run

@SimenB
Copy link
Member

SimenB commented Jul 17, 2019

That worked 👌

@SimenB
Copy link
Member

SimenB commented Jul 17, 2019

Either leak detection has improved or it's now wrong... See the node 10 failure (where we run --leak-detector on ourselves). I restarted it for now in case it's transient

@lh0x00
Copy link
Contributor Author

lh0x00 commented Jul 17, 2019

node-10 is working well but I see an error in test-jest-circus and I don't know why CI throw this error!?

@jeysal
Copy link
Contributor

jeysal commented Jul 17, 2019

circus is unrelated, happens on master too

@lh0x00
Copy link
Contributor Author

lh0x00 commented Jul 17, 2019

this PR will be waiting to test on node-12 before, right?

@SimenB
Copy link
Member

SimenB commented Jul 17, 2019

Yeah, we'll merge this when we start landing breaking changes.

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

Thanks!

@lh0x00
Copy link
Contributor Author

lh0x00 commented Jul 17, 2019

many thanks for reviewed! ^^

@jeysal jeysal removed this from the Jest 26 milestone Jan 22, 2020
@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 11, 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