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: fix freebsd10-64 CI failures #9317

Closed
wants to merge 3 commits into from
Closed

test: fix freebsd10-64 CI failures #9317

wants to merge 3 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Oct 27, 2016 •
edited
Loading

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test

Description of change

Remove unneeded timers from some tests and move others from parallel
testing to sequential testing.

This is to resolve test failures on freebsd10-64 on CI. The failures
are all due to timers firing later than expected. Timers firing later
than they are set for can happen on resource-constrained hosts and is
not a bug.

In general, it may be wise to put tests that depend on timing into
sequential testing rather than parallel testing, as the timing can
be affected by other simultaneously-running test processes.

I ran this change set (with one fewer files moved to sequential, so not quite this change set) through CI six times and there were no failures on freebsd10-64. I did the same with current master and two of the six runs failed with tests involved in this PR. (I'll probably do at least that many again now that it's a PR.)

/cc @nodejs/testing @nodejs/platform-freebsd @misterdjules

@Trott Trott added timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. build Issues and PRs related to build files or the CI. test Issues and PRs related to the tests. freebsd Issues and PRs related to the FreeBSD platform. labels Oct 27, 2016
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 27, 2016
@Trott
Copy link
Member Author

Trott commented Oct 27, 2016

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

@santigimeno
Copy link
Member

santigimeno commented Oct 27, 2016 •
edited
Loading

The changes LGTM.
One minor comment wrt the dgram tests. Due to the unreliable nature of UDP, those tests can be flaky. I would add an interval to send messages until one is received. (it can be done in a separate PR).

@Trott
Copy link
Member Author

Trott commented Oct 27, 2016

GREEN! CI IS GREEN!

@srl295 srl295 mentioned this pull request Oct 27, 2016
Closed
4 tasks
jbergstroem
jbergstroem approved these changes Oct 27, 2016
View reviewed changes
Copy link
Member

@jbergstroem jbergstroem left a comment

Choose a reason for hiding this comment

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

LGTM

@Trott Trott mentioned this pull request Oct 27, 2016
Closed
@Trott
Copy link
Member Author

Trott commented Oct 28, 2016

Running CI on this over and over to confirm there aren't other timer issues lurking in other tests, I found another timer to be removed in test/parallel/test-tls-server-failed-handshake-emits-clienterror.js. PTAL.

@santigimeno
Copy link
Member

Still LGTM

@Trott
Copy link
Member Author

Trott commented Oct 28, 2016

CI with the additional change: https://ci.nodejs.org/job/node-test-pull-request/4726/

Trott added 3 commits October 28, 2016 17:16
@Trott
test: fix freebsd10-64 CI failures
8efee55 
Remove unneeded timers from some tests and move others from parallel
testing to sequential testing.

This is to resolve test failures on freebsd10-64 on CI. The failures
are all due to timers firing later than expected. Timers firing later
than they are set for can happen on resource-constrained hosts and is
not a bug.

In general, it may be wise to put tests that depend on timing into
sequential testing rather than parallel testing, as the timing can
be affected by other simultaneously-running test processes.

Fixes: nodejs#8041
Fixes: nodejs#9227
@Trott
squash: remove another timer
23cecf7
@Trott
squash: common.mustCall()
571a612
@Trott
Copy link
Member Author

Trott commented Oct 29, 2016

Freebsd failure on last ci was Jenkins disconnect related.

Had to rebase and force push to resolve a merge conflict with master anyway, so let's run CI again!

CI: https://ci.nodejs.org/job/node-test-commit/5850/

@Trott
Copy link
Member Author

Trott commented Oct 29, 2016

Additional failing test on FreeBSD is unrelated. Will open a separate PR for it.

Trott added a commit to Trott/io.js that referenced this pull request Oct 29, 2016
@Trott
test: fix freebsd10-64 CI failures
6ef636c 
Remove unneeded timers from some tests and move others from parallel
testing to sequential testing.

This is to resolve test failures on freebsd10-64 on CI. The failures
are all due to timers firing later than expected. Timers firing later
than they are set for can happen on resource-constrained hosts and is
not a bug.

In general, it may be wise to put tests that depend on timing into
sequential testing rather than parallel testing, as the timing can
be affected by other simultaneously-running test processes.

Fixes: nodejs#8041
Fixes: nodejs#9227
PR-URL: nodejs#9317
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Julien Gilli <jgilli@nodejs.org>
Reviewed-By: Johan Bergstrom <bugs@bergstroem.nu>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
@Trott
Copy link
Member Author

Trott commented Oct 29, 2016

Landed in 6ef636c

@Trott Trott closed this Oct 29, 2016
evanlucas pushed a commit that referenced this pull request Nov 3, 2016
@Trott @evanlucas
test: fix freebsd10-64 CI failures
411b133 
Remove unneeded timers from some tests and move others from parallel
testing to sequential testing.

This is to resolve test failures on freebsd10-64 on CI. The failures
are all due to timers firing later than expected. Timers firing later
than they are set for can happen on resource-constrained hosts and is
not a bug.

In general, it may be wise to put tests that depend on timing into
sequential testing rather than parallel testing, as the timing can
be affected by other simultaneously-running test processes.

Fixes: #8041
Fixes: #9227
PR-URL: #9317
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Julien Gilli <jgilli@nodejs.org>
Reviewed-By: Johan Bergstrom <bugs@bergstroem.nu>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
@evanlucas evanlucas mentioned this pull request Nov 3, 2016
Merged
MylesBorins pushed a commit that referenced this pull request Nov 22, 2016
@Trott
test: fix freebsd10-64 CI failures
ce05b70 
Remove unneeded timers from some tests and move others from parallel
testing to sequential testing.

This is to resolve test failures on freebsd10-64 on CI. The failures
are all due to timers firing later than expected. Timers firing later
than they are set for can happen on resource-constrained hosts and is
not a bug.

In general, it may be wise to put tests that depend on timing into
sequential testing rather than parallel testing, as the timing can
be affected by other simultaneously-running test processes.

Fixes: #8041
Fixes: #9227
PR-URL: #9317
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Julien Gilli <jgilli@nodejs.org>
Reviewed-By: Johan Bergstrom <bugs@bergstroem.nu>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Nov 22, 2016
Merged
aqrln added a commit to aqrln/node that referenced this pull request Jul 19, 2017
@aqrln
test: move timing-dependent tests to sequential
c87fb3b 
Move test-http-server-keep-alive-timeout-slow-server and
test-http-server-keep-alive-timeout-slow-client-headers from parallel to
sequential to (hopefully) resolve test flakiness on freebsd10-64.

Refs: nodejs#14033
Refs: nodejs#9317
@aqrln aqrln mentioned this pull request Jul 19, 2017
Closed
2 tasks
aqrln added a commit that referenced this pull request Jul 22, 2017
@aqrln
test: move timing-dependent tests to sequential
239ebf0 
Move test-http-server-keep-alive-timeout-slow-server and
test-http-server-keep-alive-timeout-slow-client-headers from parallel to
sequential to resolve test flakiness on freebsd10-64.

Fixes: #14033
Refs: #9317
PR-URL: #14377
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
addaleax pushed a commit that referenced this pull request Jul 22, 2017
@aqrln @addaleax
test: move timing-dependent tests to sequential
9e9ce23 
Move test-http-server-keep-alive-timeout-slow-server and
test-http-server-keep-alive-timeout-slow-client-headers from parallel to
sequential to resolve test flakiness on freebsd10-64.

Fixes: #14033
Refs: #9317
PR-URL: #14377
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Fishrock123 pushed a commit that referenced this pull request Jul 24, 2017
@aqrln @Fishrock123
test: move timing-dependent tests to sequential
953736c 
Move test-http-server-keep-alive-timeout-slow-server and
test-http-server-keep-alive-timeout-slow-client-headers from parallel to
sequential to resolve test flakiness on freebsd10-64.

Fixes: #14033
Refs: #9317
PR-URL: #14377
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@Trott Trott deleted the freebsd-megafix branch January 13, 2022 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gibfahn gibfahn gibfahn approved these changes

@cjihrig cjihrig cjihrig approved these changes

@addaleax addaleax addaleax approved these changes

@misterdjules misterdjules misterdjules approved these changes

@jbergstroem jbergstroem jbergstroem approved these changes

Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI. freebsd Issues and PRs related to the FreeBSD platform. test Issues and PRs related to the tests. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@Trott @santigimeno @jbergstroem @misterdjules @addaleax @cjihrig @gibfahn @MylesBorins @nodejs-github-bot