test: fix test-timers-unrefd-interval-still-fires by Trott · Pull Request #4561 · nodejs/node · GitHub

Remove arbitrary timeout duration. This is a functionality test and not
a performance benchmark. Rely on test runner timeout.

Confirmed that the test (with ES6-isms removed) hangs/times out in Node
0.10.34 (which has the bug that this test is supposed to catch) and
passes in 0.10.35 (which has the fix). So that is good.

Fixes: #4559
Refs: #3550
R=@thealphanerd
R=@Fishrock123
R=@misterdjules

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

Stress test with current implementation: https://ci.nodejs.org/job/node-stress-single-test/307/nodes=smartos14-64/console

Stress test with version in this PR: https://ci.nodejs.org/job/node-stress-single-test/311/nodes=smartos14-64/console

I am going to repeat what I said in the previous PR: if 5 1ms timers takes so long to run there are other underlying problems. whatever.

We aren't parallelizing in CI still, I think? I'm not really sure how these machines could have that much load on them... cc @nodejs/build these machines aren't shared with anyone else, right?

This logic should not be removed. It allows the test to exit "early" if the test passed, rather than waiting for the entire keepOpen interval.

It still exits early thanks to the clearInterval(keepOpen) call. The removed code above is not necessary to exit early.

Remove arbitrary timeout duration. This is a functionality test and not a performance benchmark. Rely on test runner timeout. Confirmed that the test (with ES6-isms removed) hangs/times out in Node 0.10.34 (which has the bug that this test is supposed to catch) and passes in 0.10.35 (which has the fix). So that is good. Fixes: nodejs#4559

Rebased against master and force-pushed. The diff is now one or two lines smaller. PTAL

These lines give a proper error for if too many timeouts occur.

You're not wrong, but at the same time:

Without that code, the test times out if the clearInterval() somehow doesn't work. So it's not strictly necessary.
Presumably other tests check to confirm that clearInterval() works, so tacking it on to this test is kinda sorta scope creep.
If the test is for one thing and one thing only, it's a whole lot easier to figure out what's wrong when the test fails.
Minor point, but the access to a _-prefixed property is kind of a code smell.

But you're not wrong on your point that the removed code provides a proper error if clearInterval() does somehow fail. Happy to put it back in if it's a deal-breaker.

shrug I have a suspicion there are not necessarily other tests which check that clearInterval() works with unref timers.

-1, we don't do this for other tests? un-es6 other tests?

For tests written for regressions in specific versions of Node.js that don't support certain ES6 features, it may be beneficial to un-ES6. Otherwise, we operate on faith that the test is still testing what it is supposed to test. Of course, the downside is being handcuffed on new features in that test. So, you know, I can see both sides...

For tests written for regressions in specific versions of Node.js that don't support certain ES6 features, it may be beneficial to un-ES6.

This isn't very solid, where do we the draw the line? Should we just disable ES6 in tests?

This isn't very solid, where do we the draw the line? Should we just disable ES6 in tests?

I don't know that we'd need to get too strict about it, but if you want to draw a line, you do it like this: Presumably, each test has a version in which it should first succeed. It should fail in the version immediately prior to that. That test should be kept runnable in those versions.

So tests that get written today for stuff that's broken (or features that don't exist) in version 5.4.1 can totally use arrow functions. But tests that were written for bugs in Node 0.10.4? Not so much.

Given the state of the test suite, I'd be inclined to treat that as a guideline, though.

Closing because consensus is not coalescing around this.

So, interestingly, these are the tests that fail if you make an error like this test is supposed to catch, but in regular timers:

/usr/local/opt/python/bin/python2.7 tools/test.py --mode=release message parallel sequential -J
=== release test-signal-handler ===                                            
Path: parallel/test-signal-handler
process.pid: 26907
running process...1
assert.js:89
  throw new assert.AssertionError({
  ^
AssertionError: 1 == 0
    at process.<anonymous> (/Users/Jeremiah/Documents/node/test/parallel/test-signal-handler.js:48:10)
    at emitOne (events.js:82:20)
    at process.emit (events.js:169:7)
Command: out/Release/node /Users/Jeremiah/Documents/node/test/parallel/test-signal-handler.js
=== release test-stream-transform-objectmode-falsey-value ===                  
Path: parallel/test-stream-transform-objectmode-falsey-value
assert.js:89
  throw new assert.AssertionError({
  ^
AssertionError: [ -1 ] deepEqual [ -1, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ]
    at process.<anonymous> (/Users/Jeremiah/Documents/node/test/parallel/test-stream-transform-objectmode-falsey-value.js:15:10)
    at emitOne (events.js:82:20)
    at process.emit (events.js:169:7)
Command: out/Release/node /Users/Jeremiah/Documents/node/test/parallel/test-stream-transform-objectmode-falsey-value.js
=== release test-timers-unrefd-interval-still-fires ===                        
Path: parallel/test-timers-unrefd-interval-still-fires
[FAIL] Interval fired 1/3 times.
/Users/Jeremiah/Documents/node/test/parallel/test-timers-unrefd-interval-still-fires.js:13
  throw new Error('Test timed out. keepOpen was not canceled.');
  ^

Error: Test timed out. keepOpen was not canceled.
    at null._onTimeout (/Users/Jeremiah/Documents/node/test/parallel/test-timers-unrefd-interval-still-fires.js:13:9)
    at Timer.listOnTimeout (timers.js:92:15)
Command: out/Release/node /Users/Jeremiah/Documents/node/test/parallel/test-timers-unrefd-interval-still-fires.js
=== release test-timers-zero-timeout ===                   
Path: parallel/test-timers-zero-timeout
assert.js:89
  throw new assert.AssertionError({
  ^
AssertionError: 1 == 3
    at process.<anonymous> (/Users/Jeremiah/Documents/node/test/parallel/test-timers-zero-timeout.js:37:12)
    at emitOne (events.js:82:20)
    at process.emit (events.js:169:7)
Command: out/Release/node /Users/Jeremiah/Documents/node/test/parallel/test-timers-zero-timeout.js
=== release test-child-process-fork-dgram ===                                  
Path: parallel/test-child-process-fork-dgram
Command: out/Release/node /Users/Jeremiah/Documents/node/test/parallel/test-child-process-fork-dgram.js
--- TIMEOUT ---
=== release test-http-zero-length-write ===                                    
Path: parallel/test-http-zero-length-write
Command: out/Release/node /Users/Jeremiah/Documents/node/test/parallel/test-http-zero-length-write.js
--- TIMEOUT ---
=== release test-net-connect-options-ipv6 ===              
Path: parallel/test-net-connect-options-ipv6
Command: out/Release/node /Users/Jeremiah/Documents/node/test/parallel/test-net-connect-options-ipv6.js
--- TIMEOUT ---
[01:23|% 100|+ 994|-   7]: Done                            
make: *** [test] Error 1

So, interestingly, these are the tests that fail if you make an error like this test is supposed to catch, but in regular timers:

@Fishrock123 Can you elaborate a bit on this? I'm not sure what you mean. Do you mean that if the fix (that this test was written for) is removed from the source code, all these other tests fail as well? Which then suggests that this test might be entirely unnecessary?

@Trott No, this what will happen if you apply the problem to regular (not unrefed) timeouts.

@Fishrock123 I don't understand. I suspect I'm misunderstanding something elementary in your comment.

Here's how I'm thinking about it: This test confirms that unrefed timers still fire.

When you say "apply the problem to regular (not unrefed) timeouts", I'm not sure what the problem is that would apply to non-unrefed timers. Because surely non-unrefed timers should fire or else almost every timer test would fail. So you can't mean "Here are the tests that fail if timers don't fire." That doesn't seem likely to be what you mean. So what am I misunderstanding?

Fix: #5351 Refs: #4561 PR-URL: #5352 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Minwoo Jung <jmwsoft@gmail.com>

Trott added timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. test Issues and PRs related to the tests. labels Jan 7, 2016

Trott mentioned this pull request Jan 7, 2016

test: test-timers-unrefd-interval-still-fires.js flaky on smartOS #4559

Closed

Fishrock123 reviewed Jan 7, 2016
View reviewed changes

jasnell added the lts-watch-v4.x label Jan 7, 2016

Trott mentioned this pull request Jan 9, 2016

test: fix flaky unrefed timers test #4599

Closed

Trott force-pushed the fix-unrefd branch from 105e38b to 13529be Compare January 11, 2016 17:32

Fishrock123 reviewed Jan 11, 2016
View reviewed changes

Trott closed this Jan 16, 2016

jasnell removed the lts-watch-v4.x label Jan 18, 2016

Fishrock123 mentioned this pull request Feb 21, 2016

test: increase timeouts on some unref timers tests #5352

Closed

Trott deleted the fix-unrefd branch January 13, 2022 22:31

test: fix test-timers-unrefd-interval-still-fires by Trott · Pull Request #4561 · nodejs/node · GitHub

Uh oh!

Uh oh!

test: fix test-timers-unrefd-interval-still-fires - #4561

test: fix test-timers-unrefd-interval-still-fires#4561Trott wants to merge 1 commit intonodejs:masternodejs/node:masterfrom Trott:fix-unrefdTrott/io.js:fix-unrefdCopy head branch name to clipboard

Conversation

Trott commented Jan 7, 2016

Uh oh!

Trott commented Jan 7, 2016

Uh oh!

Fishrock123 commented Jan 7, 2016

Uh oh!

Fishrock123 Jan 7, 2016

Choose a reason for hiding this comment

Uh oh!

Trott Jan 7, 2016

Choose a reason for hiding this comment

Uh oh!

Trott commented Jan 11, 2016

Uh oh!

Fishrock123 Jan 11, 2016

Choose a reason for hiding this comment

Uh oh!

Trott Jan 11, 2016

Choose a reason for hiding this comment

Uh oh!

Fishrock123 Jan 12, 2016

Choose a reason for hiding this comment

Uh oh!

Fishrock123 commented Jan 11, 2016

Uh oh!

Trott commented Jan 11, 2016

Uh oh!

Fishrock123 commented Jan 12, 2016

Uh oh!

Trott commented Jan 13, 2016

Uh oh!

Trott commented Jan 16, 2016

Uh oh!

Fishrock123 commented Jan 18, 2016

Uh oh!

Trott commented Jan 18, 2016

Uh oh!

Fishrock123 commented Jan 18, 2016

Uh oh!

Trott commented Jan 19, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

test: fix test-timers-unrefd-interval-still-fires#4561
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:fix-unrefd