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.

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

test: improve multiple timers tests #14616

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@jasnell
Member

jasnell commented Aug 4, 2017

General improvements to various timers tests

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

tests

@mscdex mscdex added the timers label Aug 4, 2017

@jasnell

This comment has been minimized.

Show comment
Hide comment

@jasnell jasnell requested review from Trott and addaleax Aug 4, 2017

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Aug 4, 2017

Member

@jasnell not before Monday. ;)

Member

addaleax commented Aug 4, 2017

@jasnell not before Monday. ;)

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 4, 2017

Member

No worries! :-) enjoy your weekend!

Member

jasnell commented Aug 4, 2017

No worries! :-) enjoy your weekend!

@refack

refack approved these changes Aug 7, 2017

non blocking nits

console.error('[FAIL] Interval fired %d/%d times.', nbIntervalFired, N);
throw new Error('Test timed out. keepOpen was not canceled.');
}, TEST_DURATION);
const keepOpen =

This comment has been minimized.

@refack

refack Aug 7, 2017

Member

nit: This is an uncommon wrapping, IMHO

const keepOpen = setTimeout(
  common.mustNotCall('Test timed out. keepOpen was not canceled.'),
  TEST_DURATION
);

is more "regular".

Alltought

const keepOpenMsg = 'Test timed out. keepOpen was not canceled.'
const keepOpen = setTimeout(common.mustNotCall(keepOpenMsg), TEST_DURATION);

is usually my preference.

@refack

refack Aug 7, 2017

Member

nit: This is an uncommon wrapping, IMHO

const keepOpen = setTimeout(
  common.mustNotCall('Test timed out. keepOpen was not canceled.'),
  TEST_DURATION
);

is more "regular".

Alltought

const keepOpenMsg = 'Test timed out. keepOpen was not canceled.'
const keepOpen = setTimeout(common.mustNotCall(keepOpenMsg), TEST_DURATION);

is usually my preference.

@@ -36,18 +36,14 @@ const assert = require('assert');
}
{
let ncalled = 0;
let ncalled = 3;

This comment has been minimized.

@refack

refack Aug 7, 2017

Member

nit: camelCase nCalled

@refack

refack Aug 7, 2017

Member

nit: camelCase nCalled

jasnell added a commit that referenced this pull request Aug 8, 2017

test: improve multiple timers tests
PR-URL: #14616
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 8, 2017

Member

Landed in 7192e91

Member

jasnell commented Aug 8, 2017

Landed in 7192e91

@jasnell jasnell closed this Aug 8, 2017

addaleax added a commit that referenced this pull request Aug 10, 2017

test: improve multiple timers tests
PR-URL: #14616
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>

icarter09 added a commit to icarter09/node that referenced this pull request Aug 12, 2017

test: improve multiple timers tests
PR-URL: nodejs#14616
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>

@addaleax addaleax referenced this pull request Aug 13, 2017

Merged

v8.4.0 proposal #14811

MylesBorins added a commit that referenced this pull request Sep 19, 2017

test: improve multiple timers tests
PR-URL: #14616
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>

MylesBorins added a commit that referenced this pull request Sep 19, 2017

test: improve multiple timers tests
PR-URL: #14616
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>

@MylesBorins MylesBorins referenced this pull request Sep 20, 2017

Merged

v6.11.4 proposal #15506

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment