Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34.3k
test: fix flaky test-policy-integrity#40763
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
Trott commented Nov 9, 2021 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
nodejs-github-bot commented Nov 9, 2021
Trott commented Nov 9, 2021
Welp, now we have two tests that timeout. I'll split things up further to see if it helps at all. Hopefully it's not a process that never exits somewhere. |
nodejs-github-bot commented Nov 9, 2021
nodejs-github-bot commented Nov 9, 2021
tniessen commented Nov 9, 2021
Do we know how much longer it takes to run the test than we expect? If a legitimately slow test keeps running into timeouts, maybe we should adjust the timeout instead of the test? |
nodejs-github-bot commented Nov 9, 2021
Trott commented Nov 9, 2021
We did that in 2883992. This is the one test that still times out. I think it's reasonable that launching 1032 processes might take a while on some systems, so I'm OK breaking this test up like this. |
Trott commented Nov 9, 2021
Looks like it works now. PTAL, folks! |
targos commented Nov 9, 2021
Can we make a stress test? |
Trott commented Nov 9, 2021 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Probably. It was failing about 100% of the time previously though, so I'd like to think anything is an improvement. I haven't used the win10-2019 bots in a stress test recently (ever?) but if they're working with that job, then these should do it: Baseline test against master: https://ci.nodejs.org/job/node-stress-single-test/303/ |
targos commented Nov 9, 2021
There's quite a lot of duplication between the new files. It would be nice to have a TODO somewhere to refactor them. |
Trott commented Nov 9, 2021
I'm not opposed to a little bit of that here, but I tend to prefer the tests be self-contained and don't mind duplication in tests the way I might in code in other parts of the codebase. |
targos commented Nov 9, 2021 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
It seems like the stress tests don't want to start (both say that "A build with matching parameters is already running"). |
Trott commented Nov 9, 2021
They've started. The second one will wait for the first one to finish. As of this writing, the first one has 8 failures and no passes. (Since the timeout is so long and the failure rate is ~100% on master, I'm only running it 20 times in each stress test.) Here's the links directly to the console output for the master branch test which should fail 20 out of 20 or something close to that: |
nodejs-github-bot commented Nov 9, 2021
Trott commented Nov 9, 2021
Because the timeout is 12 minutes, that first stress test will take 4 hours just to run the tests (and some additional time was needed for compilation), so it will be a while before the other stress test kicks off. |
targos commented Nov 9, 2021
Ah, ok. I was looking at https://ci.nodejs.org/job/node-stress-single-test/303/console |
nodejs-github-bot commented Nov 9, 2021
Trott commented Nov 10, 2021
Stress test is looking good so far. 100% failure rate on the master branch and a 100%-so-far success rate on this PR branch. This could use another review. @nodejs/testing |
Fast-track has been requested by @Trott. Please 👍 to approve. |
Trott commented Nov 10, 2021
Stress test finished with 100% success. Master branch stress test had 100% failure. |
Split the test into seven tests so that it doesn't time out. Fixes: nodejs#40694Fixes: nodejs#38088 PR-URL: nodejs#40763 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Bradley Farias <[email protected]>
Trott commented Nov 10, 2021
Landed in fc4a792 |
Split the test into seven tests so that it doesn't time out. Fixes: #40694Fixes: #38088 PR-URL: #40763 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Bradley Farias <[email protected]>
Split the test into seven tests so that it doesn't time out. Fixes: #40694Fixes: #38088 PR-URL: #40763 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Bradley Farias <[email protected]>
Split the test into seven tests so that it doesn't time out. Fixes: #40694Fixes: #38088 PR-URL: #40763 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Bradley Farias <[email protected]>
Split the test into seven tests so that it doesn't time out.
Fixes: #40694
Fixes: #38088