FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: fix freebsd10-64 CI failures by Trott · Pull Request #9317 · nodejs/node · GitHub

/ node Public

test: fix freebsd10-64 CI failures - #9317

Closed
Trott wants to merge 3 commits into
nodejs:masterfrom
Trott:freebsd-megafix
ConversationCommits (3)ChecksFiles changed
Closed

test: fix freebsd10-64 CI failures#9317
Trott wants to merge 3 commits into
nodejs:masterfrom
Trott:freebsd-megafix

Conversation

Trott commented Oct 27, 2016 •
edited
Loading

Copy link
Copy Markdown
Member
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 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 added the test Issues and PRs related to the tests. label Oct 27, 2016

Trott commented Oct 27, 2016

Copy link
Copy Markdown
Member Author

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

santigimeno commented Oct 27, 2016 •
edited
Loading

Copy link
Copy Markdown
Member

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 commented Oct 27, 2016

Copy link
Copy Markdown
Member Author

GREEN! CI IS GREEN!

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

jbergstroem left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

Trott mentioned this pull request Oct 27, 2016
Closed

Trott commented Oct 28, 2016

Copy link
Copy Markdown
Member Author

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 commented Oct 28, 2016

Copy link
Copy Markdown
Member

Still LGTM

Trott commented Oct 28, 2016

Copy link
Copy Markdown
Member Author

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

Trott added 3 commits October 28, 2016 17:16
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
squash: remove another timer
23cecf7
squash: common.mustCall()
571a612

Trott commented Oct 29, 2016

Copy link
Copy Markdown
Member Author

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 commented Oct 29, 2016

Copy link
Copy Markdown
Member Author

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
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 commented Oct 29, 2016

Copy link
Copy Markdown
Member Author

Landed in 6ef636c

Trott closed this Oct 29, 2016
evanlucas pushed a commit that referenced this pull request Nov 3, 2016
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 mentioned this pull request Nov 3, 2016
Merged
MylesBorins pushed a commit that referenced this pull request Nov 22, 2016
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 mentioned this pull request Nov 22, 2016
Merged
aqrln added a commit to aqrln/node that referenced this pull request Jul 19, 2017
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 mentioned this pull request Jul 19, 2017
Closed
2 tasks
aqrln added a commit that referenced this pull request Jul 22, 2017
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
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
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 deleted the freebsd-megafix branch January 13, 2022 22:44
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

addaleax addaleax approved these changes
cjihrig cjihrig approved these changes
+3 more reviewers
jbergstroem jbergstroem approved these changes
misterdjules misterdjules approved these changes
gibfahn gibfahn approved these changes
Reviewers whose approvals may not affect merge requirements

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

Back | FazBrowse Home | New Git URL