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 test-timers-unrefd-interval-still-fires #4561

Closed
wants to merge 1 commit into from
Closed

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 10 additions & 11 deletions test/parallel/test-timers-unrefd-interval-still-fires.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,26 +1,25 @@
'use strict';
/*
* This test is a regression test for joyent/node#8900.
*
* Node.js 0.10.34 contains the bug that this test detects.
* Node.js 0.10.35 contains the fix.
*
* Due to the introduction of ES6-isms, comment out `'use strict'` and
* `require('../common');` to run the test on Node.js 0.10.x.
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't be necessary. If testing is needed, you could just undo the timers fix: https://github.com/nodejs/node-v0.x-archive/pull/8906/files#diff-0a5d4868b2b9b17cf9e2c11f1bd1311eL296

Copy link
Member Author

Choose a reason for hiding this comment

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

Not strictly necessary, but awfully nice. I'd rather nvm use 0.10.34 and then test as opposed to undo a fix (and hope that the code is still in basically the same place so that I don't have to do any git archaeology or greps in the code base to find it), recompile, and test.

*/
const common = require('../common');
require('../common');

const TEST_DURATION = common.platformTimeout(100);
const N = 3;
Copy link
Contributor

Choose a reason for hiding this comment

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

This gives a proper error if the test times out.

var nbIntervalFired = 0;

const keepOpen = setTimeout(() => {
console.error('[FAIL] Interval fired %d/%d times.', nbIntervalFired, N);
throw new Error('Test timed out. keepOpen was not canceled.');
}, TEST_DURATION);
const keepOpen = setInterval(function() {}, 9999);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really see how this is any less arbitrary. I guess we can up the duration to like 1s if it's really not solvable any other way, but 9s seems like overkill.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's less arbitrary because it is a setInterval() and not a setTimeout(). It could just as well be a net.createServer() or anything else that will keep the test running forever if not canceled.


const timer = setInterval(() => {
const timer = setInterval(function() {
++nbIntervalFired;
if (nbIntervalFired === N) {
clearInterval(timer);
timer._onTimeout = () => {
throw new Error('Unrefd interval fired after being cleared.');
};
clearTimeout(keepOpen);
clearInterval(keepOpen);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

}
}, 1);

Expand Down