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

big fix for CI #40684

Merged
merged 0 commits into from
Nov 1, 2021
Merged

big fix for CI #40684

merged 0 commits into from
Nov 1, 2021

Conversation

Trott
Copy link
Member

@Trott Trott commented Nov 1, 2021

This incorporates #40670 to fix the datetime test issue that is blocking CI along with a fix/workaround for test-benchmark-buffer which is blocking that PR from landing. Putting them together in one PR to get them both landed and hopefully get CI unstuck.

@Trott Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 1, 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. tools Issues and PRs related to the tools directory. labels Nov 1, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 1, 2021
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@targos
Copy link
Member

targos commented Nov 1, 2021

I think someone from build should still investigate why that windows host suddenly became slow

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

github-actions bot commented Nov 1, 2021

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

@Trott
Copy link
Member Author

Trott commented Nov 1, 2021

I think someone from build should still investigate why that windows host suddenly became slow

It's an Azure host so maybe someone at Microsoft can help? Hate to ping because I know they all have other things going on, but uh... @joaocgreis @bnb @bzoz This would be the win10-vs2019-COMPILED_BY-vs2019 jobs.

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@Trott
Copy link
Member Author

Trott commented Nov 1, 2021

It's an Azure host so maybe someone at Microsoft can help? Hate to ping because I know they all have other things going on, but uh... @joaocgreis @bnb @bzoz This would be the win10-vs2019-COMPILED_BY-vs2019 jobs.

A good example to look at might be https://ci.nodejs.org/job/node-test-binary-windows-native-suites/10883.

@Trott
Copy link
Member Author

Trott commented Nov 1, 2021

Argh, the test-policy-integrity pummel test is likewise timing out on a win10-COMPILED_BY-vs2019 machine. I'll bump up the pummel timeouts to 6x (from 4x).

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@targos
Copy link
Member

targos commented Nov 1, 2021

@Trott did you mean to push before rerunning CI?

@Trott
Copy link
Member Author

Trott commented Nov 1, 2021

@Trott did you mean to push before rerunning CI?

🤦 It's late here....

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@targos
Copy link
Member

targos commented Nov 1, 2021

Well, now the job is killed by Jenkins because it takes more than 10 minutes:

11:54:58 ok 791 pummel/test-net-timeout
11:54:58   ---
11:54:58   duration_ms: 5.233
11:54:58   ...
12:04:58 Build timed out (after 10 minutes). Marking the build as failed.
12:04:59 Terminate batch job (Y/N)?

@tniessen tniessen mentioned this pull request Nov 1, 2021
Closed
@Trott
Copy link
Member Author

Trott commented Nov 1, 2021 •
edited
Loading

Well, now the job is killed by Jenkins because it takes more than 10 minutes:

11:54:58 ok 791 pummel/test-net-timeout
11:54:58   ---
11:54:58   duration_ms: 5.233
11:54:58   ...
12:04:58 Build timed out (after 10 minutes). Marking the build as failed.
12:04:59 Terminate batch job (Y/N)?

I've increased the timeout for node-test-binary-windows-js-suites on Jenkins to 15 minutes (900 seconds up from 600 seconds). @nodejs/build Is there somewhere I should document this? (I'll open an issue in the build repo at least.)

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@Trott Trott mentioned this pull request Nov 1, 2021
Closed
@Trott
Copy link
Member Author

Trott commented Nov 1, 2021

@tniessen (or another Collaborator): This could use a second 👍 on the fast track request comment at #40684 (comment).

@tniessen
Copy link
Member

tniessen commented Nov 1, 2021

@Trott 👍

@Trott
Copy link
Member Author

Trott commented Nov 1, 2021

I've increased the timeout for node-test-binary-windows-js-suites on Jenkins to 15 minutes (900 seconds up from 600 seconds).

Of course, if the problem is that there's a race condition and pummel/test-policy-integrity will never finish, then that won't help....

If that's the case, I'll insert some skip-if-on-Windows code into the test and open an issue for it. I hate to do that, but it is an experimental feature at least.

@Trott
Copy link
Member Author

Trott commented Nov 1, 2021

test-policy-integrity still failed on the Windows 10/VS2019 host after 12 minutes so I'm marking it flaky, which is unfortunate because it seems to fail approximately 100% of the time. But it's an experimental feature and we need to be able to land working code.

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@Trott
Copy link
Member Author

Trott commented Nov 1, 2021

image

@Trott
Copy link
Member Author

Trott commented Nov 1, 2021

I wonder if someone did something to fix the slow-running issue. test-policy-integrity timed out (more than 12 minutes) in https://ci.nodejs.org/job/node-test-pull-request/40619/ but took less than a minute to run on https://ci.nodejs.org/job/node-test-pull-request/40620/.

@Trott
Copy link
Member Author

Trott commented Nov 1, 2021

I wonder if someone did something to fix the slow-running issue. test-policy-integrity timed out (more than 12 minutes) in https://ci.nodejs.org/job/node-test-pull-request/40619/ but took less than a minute to run on https://ci.nodejs.org/job/node-test-pull-request/40620/.

(Regardless, let's land this, and we can revert or undo the greater time allowances if the fix is across everything and robust. It's not harming anything.)

Trott pushed a commit that referenced this pull request Nov 1, 2021
@PiotrRybak @Trott
test: fix test-datetime-change-notify after daylight change
747ef34 
Add standard timezone name for Dublin without daylight saving

PR-URL: #40684
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Voltrex <mohammadkeyvanzade94@gmail.com>
Trott added a commit that referenced this pull request Nov 1, 2021
@Trott
test,tools: increase timeout for benchmark tests
cbcf86f 
test-benchmark-buffer is consistently timing out on a single Windows
host in CI. Rather than try to figure out if we need to scale the
timeout up for a certain memory limit or chip speed or something else,
let's increase the timeout for benchmark tests in general.

PR-URL: #40684
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Voltrex <mohammadkeyvanzade94@gmail.com>
Trott added a commit that referenced this pull request Nov 1, 2021
@Trott
test,tools: increase pummel/benchmark test timeout from 4x to 6x
2883992 
The win10-COMPILED_BY-vs2019 hosts in CI are very slow and timing out on
certain tests in pummel and (previously) benchmark. Increase timeout
from 4x to 6x.

PR-URL: #40684
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Voltrex <mohammadkeyvanzade94@gmail.com>
@Trott Trott closed this Nov 1, 2021
@Trott
Copy link
Member Author

Trott commented Nov 1, 2021

Landed in 86099a3...b2aff85

@Trott Trott merged commit b2aff85 into nodejs:master Nov 1, 2021
@Trott Trott deleted the big-fix branch November 1, 2021 16:49
@Mesteery Mesteery added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 1, 2021
@richardlau richardlau mentioned this pull request Nov 1, 2021
Closed
This was referenced Nov 2, 2021
Open
Open
Open
targos pushed a commit that referenced this pull request Nov 4, 2021
@PiotrRybak @targos
test: fix test-datetime-change-notify after daylight change
50c6666 
Add standard timezone name for Dublin without daylight saving

PR-URL: #40684
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Voltrex <mohammadkeyvanzade94@gmail.com>
targos pushed a commit that referenced this pull request Nov 4, 2021
@Trott @targos
test,tools: increase timeout for benchmark tests
908f644 
test-benchmark-buffer is consistently timing out on a single Windows
host in CI. Rather than try to figure out if we need to scale the
timeout up for a certain memory limit or chip speed or something else,
let's increase the timeout for benchmark tests in general.

PR-URL: #40684
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Voltrex <mohammadkeyvanzade94@gmail.com>
targos pushed a commit that referenced this pull request Nov 4, 2021
@Trott @targos
test,tools: increase pummel/benchmark test timeout from 4x to 6x
aef809f 
The win10-COMPILED_BY-vs2019 hosts in CI are very slow and timing out on
certain tests in pummel and (previously) benchmark. Increase timeout
from 4x to 6x.

PR-URL: #40684
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Voltrex <mohammadkeyvanzade94@gmail.com>
targos pushed a commit that referenced this pull request Nov 4, 2021
@Trott @targos
test: mark test-policy-integrity flaky on Windows
8226690 
Ref: #40694

PR-URL: #40684
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Voltrex <mohammadkeyvanzade94@gmail.com>
targos pushed a commit that referenced this pull request Nov 6, 2021
@PiotrRybak @targos
test: fix test-datetime-change-notify after daylight change
8fa1c61 
Add standard timezone name for Dublin without daylight saving

PR-URL: #40684
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Voltrex <mohammadkeyvanzade94@gmail.com>
targos pushed a commit that referenced this pull request Nov 6, 2021
@Trott @targos
test,tools: increase timeout for benchmark tests
f731f5f 
test-benchmark-buffer is consistently timing out on a single Windows
host in CI. Rather than try to figure out if we need to scale the
timeout up for a certain memory limit or chip speed or something else,
let's increase the timeout for benchmark tests in general.

PR-URL: #40684
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Voltrex <mohammadkeyvanzade94@gmail.com>
targos pushed a commit that referenced this pull request Nov 6, 2021
@Trott @targos
test,tools: increase pummel/benchmark test timeout from 4x to 6x
1844463 
The win10-COMPILED_BY-vs2019 hosts in CI are very slow and timing out on
certain tests in pummel and (previously) benchmark. Increase timeout
from 4x to 6x.

PR-URL: #40684
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Voltrex <mohammadkeyvanzade94@gmail.com>
targos pushed a commit that referenced this pull request Nov 6, 2021
@Trott @targos
test: mark test-policy-integrity flaky on Windows
5020f63 
Ref: #40694

PR-URL: #40684
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Voltrex <mohammadkeyvanzade94@gmail.com>
@RaisinTen RaisinTen mentioned this pull request Nov 7, 2021
Merged
@targos targos mentioned this pull request Nov 8, 2021
Merged
@BethGriggs BethGriggs mentioned this pull request Nov 26, 2021
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VoltrexKeyva VoltrexKeyva VoltrexKeyva approved these changes

@tniessen tniessen tniessen approved these changes

@targos targos targos approved these changes

Assignees
No one assigned
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. tools Issues and PRs related to the tools directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Trott @nodejs-github-bot @targos @tniessen @devsnek @VoltrexKeyva @Mesteery