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: delete test/pummel/test-repl-empty-maybelocal-crash.js #42720

Conversation

RaisinTen
Copy link
Contributor

@RaisinTen RaisinTen commented Apr 13, 2022

It was disconnecting the runners from the CI server. Not worth having a
resource-intensive test for this kind of an edge cases.

Fixes: #42719
Signed-off-by: Darshan Sen [email protected]

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Apr 13, 2022
@RaisinTen RaisinTen force-pushed the test/mark-test/pummel/test-repl-empty-maybelocal-crash.js-as-flaky-on-freebsd branch from c06b845 to e0091c7 Compare April 13, 2022 14:19
@nodejs-github-bot

This comment was marked as outdated.

@RaisinTen RaisinTen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 13, 2022
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.

If it's a memory issue (and it doesn't only affect FreeBSD), we should probably use common.enoughTestMem

@RaisinTen RaisinTen removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 14, 2022
@nodejs-github-bot

This comment was marked as outdated.

@RaisinTen
Copy link
Contributor Author

RaisinTen commented Apr 14, 2022

If it's a memory issue (and it doesn't only affect FreeBSD), we should probably use common.enoughTestMem

@targos that doesn't seem to work - https://ci.nodejs.org/job/node-test-commit-freebsd/43653/nodes=freebsd12-x64/console. :/

@targos
Copy link
Member

targos commented Apr 14, 2022

Or maybe just delete the test?
I'm not sure it's worth having a resource-intensive test for this kind of edge cases 🤔

It was disconnecting the runners from the CI server. Not worth having a
resource-intensive test for this kind of an edge cases.

Fixes: nodejs#42719
Signed-off-by: Darshan Sen <[email protected]>
@RaisinTen RaisinTen force-pushed the test/mark-test/pummel/test-repl-empty-maybelocal-crash.js-as-flaky-on-freebsd branch from fc18ad6 to d56a3bc Compare April 14, 2022 13:35
@RaisinTen RaisinTen changed the title test: mark test-repl-empty-maybelocal-crash flaky on freebsd test: delete test/pummel/test-repl-empty-maybelocal-crash.js Apr 14, 2022
@nodejs-github-bot
Copy link
Collaborator

@RaisinTen
Copy link
Contributor Author

Agreed, @targos deleted, PTAL

@RaisinTen RaisinTen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 14, 2022
@targos
Copy link
Member

targos commented Apr 14, 2022

I don't know if it's possible, but if we were able to reduce the maximum buffer size limit with a command-line flag, that could be another way to fix it.

@RaisinTen
Copy link
Contributor Author

I don't know if it's possible, but if we were able to reduce the maximum buffer size limit with a command-line flag, that could be another way to fix it.

That won't be possible because Node.js uses

static const size_t kMaxLength = v8::TypedArray::kMaxLength;

for the max buffer length and it's a compile time constant from V8
static constexpr size_t kMaxLength =
internal::kApiSystemPointerSize == 4
? internal::kSmiMaxValue
: static_cast<size_t>(uint64_t{1} << 32);
.

@nodejs-github-bot
Copy link
Collaborator

@richardlau richardlau added the fast-track PRs that do not need to wait for 48 hours to land. label Apr 14, 2022
@github-actions
Copy link
Contributor

Fast-track has been requested by @richardlau. Please 👍 to approve.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@BethGriggs
Copy link
Member

The flake that keeps appearing is #42741. I've resumed CI again, but considering the contents of this PR is removing a different test - I propose landing this as-is.

I'm hoping to include this PR in v18.0.0 to mitigate some of CI instability.

@BethGriggs
Copy link
Member

Hit the flake again, but FreeBSD is also experimental. Landing this now.

BethGriggs pushed a commit that referenced this pull request Apr 14, 2022
It was disconnecting the runners from the CI server. Not worth having a
resource-intensive test for this kind of an edge cases.

Fixes: #42719
Signed-off-by: Darshan Sen <[email protected]>
PR-URL: #42720
Fixes: #42719
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: Stewart X Addison <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@BethGriggs
Copy link
Member

Landed in 19064be

@BethGriggs BethGriggs closed this Apr 14, 2022
@RaisinTen RaisinTen deleted the test/mark-test/pummel/test-repl-empty-maybelocal-crash.js-as-flaky-on-freebsd branch April 15, 2022 01:06
xtx1130 pushed a commit to xtx1130/node that referenced this pull request Apr 25, 2022
It was disconnecting the runners from the CI server. Not worth having a
resource-intensive test for this kind of an edge cases.

Fixes: nodejs#42719
Signed-off-by: Darshan Sen <[email protected]>
PR-URL: nodejs#42720
Fixes: nodejs#42719
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: Stewart X Addison <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Dossar pushed a commit to Dossar/node that referenced this pull request May 26, 2022
It was disconnecting the runners from the CI server. Not worth having a
resource-intensive test for this kind of an edge cases.

Fixes: nodejs#42719
Signed-off-by: Darshan Sen <[email protected]>
PR-URL: nodejs#42720
Fixes: nodejs#42719
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: Stewart X Addison <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Dossar pushed a commit to Dossar/node that referenced this pull request May 26, 2022
It was disconnecting the runners from the CI server. Not worth having a
resource-intensive test for this kind of an edge cases.

Fixes: nodejs#42719
Signed-off-by: Darshan Sen <[email protected]>
PR-URL: nodejs#42720
Fixes: nodejs#42719
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: Stewart X Addison <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
juanarbol pushed a commit that referenced this pull request May 31, 2022
It was disconnecting the runners from the CI server. Not worth having a
resource-intensive test for this kind of an edge cases.

Fixes: #42719
Signed-off-by: Darshan Sen <[email protected]>
PR-URL: #42720
Fixes: #42719
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: Stewart X Addison <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
juanarbol pushed a commit that referenced this pull request Jun 1, 2022
It was disconnecting the runners from the CI server. Not worth having a
resource-intensive test for this kind of an edge cases.

Fixes: #42719
Signed-off-by: Darshan Sen <[email protected]>
PR-URL: #42720
Backport-PR-URL: #42967
Fixes: #42719
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: Stewart X Addison <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@juanarbol juanarbol mentioned this pull request Jun 1, 2022
BethGriggs pushed a commit that referenced this pull request Jun 1, 2022
It was disconnecting the runners from the CI server. Not worth having a
resource-intensive test for this kind of an edge cases.

Fixes: #42719
Signed-off-by: Darshan Sen <[email protected]>
PR-URL: #42720
Backport-PR-URL: #42967
Fixes: #42719
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: Stewart X Addison <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. fast-track PRs that do not need to wait for 48 hours to land. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate flaky crash in test/pummel/test-repl-empty-maybelocal-crash.js on freebsd
8 participants