Skip to content

Conversation

@Trott
Copy link
Member

@TrottTrott commented Oct 27, 2016

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

@TrottTrott 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-botnodejs-github-bot added the test Issues and PRs related to the tests. label Oct 27, 2016
@Trott
Copy link
MemberAuthor

@santigimeno
Copy link
Member

santigimeno commented Oct 27, 2016

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
MemberAuthor

GREEN! CI IS GREEN!

@srl295srl295 mentioned this pull request Oct 27, 2016
4 tasks
Copy link
Member

@jbergstroemjbergstroem 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
Copy link
MemberAuthor

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
MemberAuthor

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

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#8041Fixes: nodejs#9227
@Trott
Copy link
MemberAuthor

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
MemberAuthor

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
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#8041Fixes: nodejs#9227 PR-URL: nodejs#9317 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Julien Gilli <[email protected]> Reviewed-By: Johan Bergstrom <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]>
@Trott
Copy link
MemberAuthor

Landed in 6ef636c

@TrottTrott closed this Oct 29, 2016
evanlucas pushed a commit that referenced this pull request Nov 3, 2016
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: #8041Fixes: #9227 PR-URL: #9317 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Julien Gilli <[email protected]> Reviewed-By: Johan Bergstrom <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]>
@evanlucasevanlucas mentioned this pull request Nov 3, 2016
MylesBorins pushed a commit that referenced this pull request Nov 22, 2016
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: #8041Fixes: #9227 PR-URL: #9317 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Julien Gilli <[email protected]> Reviewed-By: Johan Bergstrom <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Nov 22, 2016
aqrln added a commit to aqrln/node that referenced this pull request Jul 19, 2017
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 added a commit that referenced this pull request Jul 22, 2017
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>
addaleax pushed a commit that referenced this pull request Jul 22, 2017
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Jul 24, 2017
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>
@TrottTrott deleted the freebsd-megafix branch January 13, 2022 22:44
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

buildIssues and PRs related to build files or the CI.freebsdIssues and PRs related to the FreeBSD platform.testIssues and PRs related to the tests.timersIssues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants

@Trott@santigimeno@jbergstroem@misterdjules@addaleax@cjihrig@gibfahn@MylesBorins@nodejs-github-bot