-
Notifications
You must be signed in to change notification settings - Fork 30k
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
Conversation
@miladfarca Could you rebase this to get rid of the merge commits? 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. |
@sam-github Top of the comment includes the original nodejs issue |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this 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.
There was a problem hiding this 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.
@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. |
They will only go away in case you push force your local rebased branch. So what you can do is:
As a side note: it's likely better to create a feature branch instead of using I guess in this specific case we could go ahead and land this PR without a CI run. Depending on what other collaborators think. |
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
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). |
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. |
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>
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>
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>
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