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

test: skip test-worker-prof on all platforms. #28175

Closed
wants to merge 1 commit into from
Closed

test: skip test-worker-prof on all platforms. #28175

wants to merge 1 commit into from

Conversation

miladfarca
Copy link
Contributor

There was a problem with V8 GC which potentially could
crash this test on all platforms. Issue is fixed on V8 master. SKIPPING until
the V8 patch is backported to 7.5 and 7.6.

Bug:
https://bugs.chromium.org/p/v8/issues/detail?id=9333

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jun 11, 2019
@sam-github
Copy link
Contributor

sam-github commented Jun 11, 2019

@miladfarca Could you rebase this to get rid of the merge commits? git fetch -a; git rebase upstream/master; git push --force-with-leaase should do it.

Also, could you link to a nodejs/node issue in the .status file? That allows someone who is later looking at the skipped files to find the source bug and its state - the v8 issue won't indicate when the bug fix has landed in node master (I don't think). Once the status file is linked to a nodejs/node github issue, you won't need the explanatory text, either, since it will be in the issue.

@miladfarca
Copy link
Contributor Author

@sam-github Top of the comment includes the original nodejs issue https://github.com/nodejs/node/issues/26401, should remove the comments?

@mhdawson
Copy link
Member

As a note, while this can fail on all platforms it seems to fail very regularly on PPC resulting in yellow builds. I'm in agreement that skipping until we get the fix in will be good for CI hygiene.

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor Author

@miladfarca miladfarca left a comment

Choose a reason for hiding this comment

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

@sam-github I have done a rebase but the merge commits aren't going away.

test/parallel/parallel.status Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@targos targos left a comment

Choose a reason for hiding this comment

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

This test also reliably times out on my computer (Fedora 30). I think it started after the latest kernel update.

@miladfarca
Copy link
Contributor Author

@targos It would timeout on any machine if V8 GC thinks heap needs a cleanup, not architecture dependent, thats why it was passing and failing irregularly on our end.

@BridgeAR
Copy link
Member

@miladfarca

@sam-github I have done a rebase but the merge commits aren't going away

They will only go away in case you push force your local rebased branch. So what you can do is:

git fetch upstream
git checkout master
git rebase -i upstream/master
git push --force-with-lease

As a side note: it's likely better to create a feature branch instead of using master for future pull requests :) The steps above should work in case https://github.com/nodejs/node is called upstream.

I guess in this specific case we could go ahead and land this PR without a CI run. Depending on what other collaborators think.

@sam-github
Copy link
Contributor

Agreed, this can land without CI, since the only thing it does is add one SKIPed test.

Due to a bug in V8 GC, this test case has to potential to fail
on all platforms. V8 master has been patched with a fix and waiting
for it to be backported to V8 7.5 and 7.6. Skipping the test until
it is backported. Bug can be tracked here:

https://bugs.chromium.org/p/v8/issues/detail?id=9333
@sam-github
Copy link
Contributor

I force pushed @miladfarca 's branch (well, master...), squashed the fixup, and rewrote the commit description to fit in the 50 chars (cause of Travis CI failure, I assume).

@sam-github
Copy link
Contributor

@addaleax
Copy link
Member

addaleax commented Jun 14, 2019

Landed in b614840

That being said:

Edit: Just saw that this has been approved for merging into V8 7.5. I think we still want to have the fix on Node v10.x, so we should open a PR for that, but I guess that can wait until after we’ve updated our V8 version.

@addaleax addaleax closed this Jun 14, 2019
addaleax pushed a commit that referenced this pull request Jun 14, 2019
Due to a bug in V8 GC, this test case has to potential to fail
on all platforms. V8 master has been patched with a fix and waiting
for it to be backported to V8 7.5 and 7.6. Skipping the test until
it is backported. Bug can be tracked here:

https://bugs.chromium.org/p/v8/issues/detail?id=9333

PR-URL: #28175
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann (רפאל פלחי) <refack@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@addaleax addaleax added v8 engine Issues and PRs related to the V8 dependency. worker Issues and PRs related to Worker support. labels Jun 14, 2019
BridgeAR pushed a commit that referenced this pull request Jun 17, 2019
Due to a bug in V8 GC, this test case has to potential to fail
on all platforms. V8 master has been patched with a fix and waiting
for it to be backported to V8 7.5 and 7.6. Skipping the test until
it is backported. Bug can be tracked here:

https://bugs.chromium.org/p/v8/issues/detail?id=9333

PR-URL: #28175
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann (רפאל פלחי) <refack@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Jun 17, 2019
targos pushed a commit that referenced this pull request Jun 18, 2019
Due to a bug in V8 GC, this test case has to potential to fail
on all platforms. V8 master has been patched with a fix and waiting
for it to be backported to V8 7.5 and 7.6. Skipping the test until
it is backported. Bug can be tracked here:

https://bugs.chromium.org/p/v8/issues/detail?id=9333

PR-URL: #28175
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann (רפאל פלחי) <refack@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests. v8 engine Issues and PRs related to the V8 dependency. worker Issues and PRs related to Worker support.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants