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: fix flaky test-policy-integrity #40763

Closed
wants to merge 1 commit into from
Closed

test: fix flaky test-policy-integrity #40763

wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Nov 9, 2021

Split the test into seven tests so that it doesn't time out.

Fixes: #40694
Fixes: #38088

@Trott Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 9, 2021
@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 Nov 9, 2021
@Trott Trott requested a review from bmeck November 9, 2021 03:43
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 9, 2021
@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member Author

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
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@tniessen
Copy link
Member

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
Copy link
Collaborator

@Trott
Copy link
Member Author

Trott 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?

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
Copy link
Member Author

Trott commented Nov 9, 2021

Looks like it works now. PTAL, folks!

@targos
Copy link
Member

targos commented Nov 9, 2021

Can we make a stress test?

@Trott
Copy link
Member Author

Trott commented Nov 9, 2021

Can we make a stress test?

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/
Test against this PR: https://ci.nodejs.org/job/node-stress-single-test/304/

@targos
Copy link
Member

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
Copy link
Member Author

Trott 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.

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
Copy link
Member

targos commented Nov 9, 2021

It seems like the stress tests don't want to start (both say that "A build with matching parameters is already running").

@Trott
Copy link
Member Author

Trott commented Nov 9, 2021

It seems like the stress tests don't want to start (both say that "A build with matching parameters is already running").

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:
https://ci.nodejs.org/job/node-stress-single-test/303/nodes=win10-vs2019/console

@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member Author

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: https://ci.nodejs.org/job/node-stress-single-test/303/nodes=win10-vs2019/console

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
Copy link
Member

targos commented Nov 9, 2021

It seems like the stress tests don't want to start (both say that "A build with matching parameters is already running").

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: https://ci.nodejs.org/job/node-stress-single-test/303/nodes=win10-vs2019/console

Ah, ok. I was looking at https://ci.nodejs.org/job/node-stress-single-test/303/console

@nodejs-github-bot
Copy link
Collaborator

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 9, 2021
@Trott
Copy link
Member Author

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

@Trott Trott added the fast-track PRs that do not need to wait for 48 hours to land. label Nov 10, 2021
@github-actions
Copy link
Contributor

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

@Trott
Copy link
Member Author

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#40694
Fixes: nodejs#38088

PR-URL: nodejs#40763
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
@Trott
Copy link
Member Author

Trott commented Nov 10, 2021

Landed in fc4a792

@Trott Trott closed this Nov 10, 2021
@Trott Trott deleted the wip branch November 10, 2021 21:35
targos pushed a commit that referenced this pull request Nov 21, 2021
Split the test into seven tests so that it doesn't time out.

Fixes: #40694
Fixes: #38088

PR-URL: #40763
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
danielleadams pushed a commit that referenced this pull request Jan 30, 2022
Split the test into seven tests so that it doesn't time out.

Fixes: #40694
Fixes: #38088

PR-URL: #40763
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
Split the test into seven tests so that it doesn't time out.

Fixes: #40694
Fixes: #38088

PR-URL: #40763
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
@danielleadams danielleadams mentioned this pull request Feb 1, 2022
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
6 participants