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.

Sign up for GitHub

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

Jump to bottom

test: increase timeout in vm-timeout-escape-queuemicrotask #31966

Closed
wants to merge 1 commit into from
Closed

test: increase timeout in vm-timeout-escape-queuemicrotask #31966

wants to merge 1 commit into from

Conversation

lundibundi
Copy link
Member

@lundibundi lundibundi commented Feb 26, 2020 •
edited
Loading

It looks like under high load the loop isn't even started and therefore
successfully finishes without 'escaping'. After increasing the timeout
during parallel run of the test failure rate decreased from 15/1000 to
0/1000.

This will not fix the issue but should at least decrease its failure rate. Though imo the test is inherently flaky and we probably won't be able to fix this completely.
Refs: #25529

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

/cc @nodejs/testing

@lundibundi
test: increase timeout in vm-timeout-escape-queuemicrotask
3d27482 
It looks like under high load the loop isn't even started and therefore
successfully finishes without 'escaping'. After increasing the timeout
during parallel run of the test failure rate decreased from 15/1000 to
0/1000.
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Feb 26, 2020
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/29370/

@rustyconover rustyconover mentioned this pull request Feb 26, 2020
Closed
2 tasks
@Trott
Copy link
Member

Trott commented Feb 26, 2020 •
edited
Loading

Since this is only failing on Raspberry Pi devices, perhaps another solution to consider is to increase the multiplier in platformTimeout() for armv6 and/or armv7 devices? Then it only slows down the test on Raspberry Pi.

Just a suggestion. Not blocking the change proposed here if it's preferred/easier/whatever.

@Trott
Copy link
Member

Trott commented Feb 26, 2020 •
edited
Loading

Since this is only failing on Raspberry Pi devices, perhaps another solution to consider is to increase the multiplier in platformTimeout() for armv6 and/or armv7 devices? Then it only slows down the test on Raspberry Pi.

Just a suggestion. Not blocking the change proposed here if it's preferred/easier/whatever.

Maybe even consider adding an options object for platformTimeout() that allows individual tests to specify custom multipliers? Then the tests can still be super fast on fast machines without straining the limits of slower machines, but also without having to tune a single multiplier for all test cases. This one needs a bigger multiplier? OK, fine, have a bigger multiplier that only affects this test. That kind of thing.

@Trott
Copy link
Member

Trott commented Feb 26, 2020

Though imo the test is inherently flaky and we probably won't be able to fix this completely.

As a known-issue test, it is indeed inherently flaky. If the issue ever gets fixed, it will be flaky in the other direction which is probably preferable: It will usually fail if there's a bug, but will always pass in the bug's absence. Since that failure will (typically) be cross-platform, we can probably safely assume it would be caught in CI.

In other words, if the known issue ever gets fixed, the test is (probably) fine. But as it is now, yeah, there might be no reasonable way to completely de-flake it short of fixing the bug so it's not a known-issue test anymore.

@addaleax
Copy link
Member

Realistically, this bug is just never going to be fixed without major changes to V8 and worker.terminate() is a reasonable equivalent for most use cases. I’d be good with just deleting the test, tbh.

@jasnell
Copy link
Member

jasnell commented Feb 26, 2020

I'm definitely +1 on removing the test

jasnell
jasnell approved these changes Feb 26, 2020
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM but prefer removing it.

@jasnell
Copy link
Member

jasnell commented Feb 26, 2020 •
edited
Loading

Also, I'd like to see if we can fast track this because it is repeatedly causing failures for me in CI and it's painful. Please 👍 if you agree to fast track

@jasnell jasnell mentioned this pull request Feb 26, 2020
Closed
3 tasks
@Trott
Copy link
Member

Trott commented Feb 27, 2020

I'm OK with fast-tracking this. Someone can submit a subsequent PR to remove if that's preferred.

@Trott Trott added the fast-track PRs that do not need to wait for 48 hours to land. label Feb 27, 2020
Trott pushed a commit that referenced this pull request Feb 27, 2020
@lundibundi @Trott
test: increase timeout in vm-timeout-escape-queuemicrotask
468bfd3 
It looks like under high load the loop isn't even started and therefore
successfully finishes without 'escaping'. After increasing the timeout
during parallel run of the test failure rate decreased from 15/1000 to
0/1000.

PR-URL: #31966
Refs: #25529
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@Trott
Copy link
Member

Trott commented Feb 27, 2020

Landed in 468bfd3

@Trott Trott closed this Feb 27, 2020
@lundibundi lundibundi mentioned this pull request Feb 27, 2020
Closed
2 tasks
codebytere pushed a commit that referenced this pull request Feb 27, 2020
@lundibundi @codebytere
test: increase timeout in vm-timeout-escape-queuemicrotask
49c959d 
It looks like under high load the loop isn't even started and therefore
successfully finishes without 'escaping'. After increasing the timeout
during parallel run of the test failure rate decreased from 15/1000 to
0/1000.

PR-URL: #31966
Refs: #25529
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@codebytere codebytere mentioned this pull request Feb 29, 2020
Merged
codebytere pushed a commit that referenced this pull request Mar 15, 2020
@lundibundi @codebytere
test: increase timeout in vm-timeout-escape-queuemicrotask
2900807 
It looks like under high load the loop isn't even started and therefore
successfully finishes without 'escaping'. After increasing the timeout
during parallel run of the test failure rate decreased from 15/1000 to
0/1000.

PR-URL: #31966
Refs: #25529
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere pushed a commit that referenced this pull request Mar 17, 2020
@lundibundi @codebytere
test: increase timeout in vm-timeout-escape-queuemicrotask
3adcac0 
It looks like under high load the loop isn't even started and therefore
successfully finishes without 'escaping'. After increasing the timeout
during parallel run of the test failure rate decreased from 15/1000 to
0/1000.

PR-URL: #31966
Refs: #25529
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@codebytere codebytere mentioned this pull request Mar 17, 2020
Merged
codebytere pushed a commit that referenced this pull request Mar 30, 2020
@lundibundi @codebytere
test: increase timeout in vm-timeout-escape-queuemicrotask
086e14d 
It looks like under high load the loop isn't even started and therefore
successfully finishes without 'escaping'. After increasing the timeout
during parallel run of the test failure rate decreased from 15/1000 to
0/1000.

PR-URL: #31966
Refs: #25529
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lpinca lpinca lpinca approved these changes

@addaleax addaleax addaleax approved these changes

@Trott Trott Trott approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
fast-track PRs that do not need to wait for 48 hours to land. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@lundibundi @nodejs-github-bot @Trott @addaleax @jasnell @lpinca