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: refactor parallel/test-timer-close #10517

Closed
wants to merge 1 commit into from
Closed

test: refactor parallel/test-timer-close #10517

wants to merge 1 commit into from
+7 −12

Conversation

BethGriggs
Copy link
Member

@BethGriggs BethGriggs commented Dec 29, 2016 •
edited
Loading

Refactor and simplify parallel/test-timer-close.js. Add comment to
describe the test case.

Checklist
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test

Description of change

Refactor and simplify test-timer-close.js.

@nodejs-github-bot nodejs-github-bot added test Issues and PRs related to the tests. lts-watch-v6.x labels Dec 29, 2016
@gibfahn
Copy link
Member

gibfahn commented Dec 29, 2016 •
edited
Loading

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

cc/ @bnoordhuis as you raised the original issue which this test checks (#1287)

gibfahn
gibfahn approved these changes Dec 29, 2016
View reviewed changes
Copy link
Member

@gibfahn gibfahn left a comment

Choose a reason for hiding this comment

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

CI is green, LGTM

@gibfahn gibfahn self-assigned this Dec 29, 2016
@gibfahn gibfahn mentioned this pull request Dec 29, 2016
Closed
@mscdex mscdex added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Dec 29, 2016
jasnell
jasnell reviewed Dec 29, 2016
View reviewed changes
test/parallel/test-timer-close.js

t.close(onclose);
t.close(onclose);
const t = new (process.binding('timer_wrap').Timer)();
Copy link
Member

Choose a reason for hiding this comment

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

hmm... can't say that I'm really a fan of this style (even tho I know it was used before this change)... I prefer separating out the call to process.binding...

const Timer = process.binding('timer_wrap').Timer;
const t = new Timer();

jasnell
jasnell approved these changes Dec 29, 2016
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

Minor nit but otherwise LGTM

gibfahn
gibfahn approved these changes Dec 29, 2016
View reviewed changes
Copy link
Member

@gibfahn gibfahn left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Still LGTM (post @jasnell's nit)

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

EDIT: Looks like FreeBSD CI failures were unrelated to this test.

@BethGriggs BethGriggs force-pushed the pr-test-timer-close branch from aa1919d to 4a78216 Compare January 3, 2017 10:29
@gibfahn
Copy link
Member

gibfahn commented Jan 3, 2017

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

@BethGriggs
test: refactor test-timer-close
3a480ed 
Refactor and simplify parallel/test-timer-close.js. Add comment to
describe the test case.
@BethGriggs BethGriggs force-pushed the pr-test-timer-close branch from 4a78216 to 3a480ed Compare January 3, 2017 12:11
@gibfahn
Copy link
Member

gibfahn commented Jan 3, 2017 •
edited
Loading

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

EDIT: This failure on FreeBSD seems unrelated:

not ok 688 parallel/test-net-connect-local-error
  ---
  duration_ms: 0.186
  severity: fail
  stack: |-
    
    assert.js:85
      throw new assert.AssertionError({
      ^
    AssertionError: undefined == 12446
        at Socket.onError (/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd11-x64/test/parallel/test-net-connect-local-error.js:13:10)
        at Socket.<anonymous> (/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd11-x64/test/common.js:443:15)
        at emitOne (events.js:96:13)
        at Socket.emit (events.js:188:7)
        at emitErrorNT (net.js:1281:8)
        at _combinedTickCallback (internal/process/next_tick.js:74:11)
        at process._tickCallback (internal/process/next_tick.js:98:9)
  ...

@gibfahn
Copy link
Member

gibfahn commented Jan 3, 2017

Landed in 5a51955

@gibfahn gibfahn closed this Jan 3, 2017
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 18, 2017
@BethGriggs @italoacasas
test: refactor test-timer-close
c115f02 
Refactor and simplify parallel/test-timer-close.js. Add comment to
describe the test case.

PR-URL: nodejs#10517
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 19, 2017
@BethGriggs @italoacasas
test: refactor test-timer-close
264c935 
Refactor and simplify parallel/test-timer-close.js. Add comment to
describe the test case.

PR-URL: nodejs#10517
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@italoacasas italoacasas mentioned this pull request Jan 25, 2017
Closed
targos pushed a commit that referenced this pull request Jan 28, 2017
@BethGriggs @targos
test: refactor test-timer-close
012702a 
Refactor and simplify parallel/test-timer-close.js. Add comment to
describe the test case.

PR-URL: #10517
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@italoacasas italoacasas mentioned this pull request Jan 29, 2017
Merged
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
@BethGriggs @italoacasas
test: refactor test-timer-close
eba9add 
Refactor and simplify parallel/test-timer-close.js. Add comment to
describe the test case.

PR-URL: nodejs#10517
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
@BethGriggs @italoacasas
test: refactor test-timer-close
69f3bb3 
Refactor and simplify parallel/test-timer-close.js. Add comment to
describe the test case.

PR-URL: nodejs#10517
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@MylesBorins
Copy link
Contributor

This does not land cleanly in LTS. Added dont-land label. Please feel free to manually backport

@BethGriggs BethGriggs deleted the pr-test-timer-close branch February 21, 2018 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gibfahn gibfahn gibfahn approved these changes

@cjihrig cjihrig cjihrig approved these changes

@Fishrock123 Fishrock123 Fishrock123 approved these changes

@santigimeno santigimeno santigimeno approved these changes

@jasnell jasnell jasnell approved these changes

Assignees

@gibfahn gibfahn

Labels
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
@BethGriggs @gibfahn @MylesBorins @jasnell @santigimeno @Fishrock123 @cjihrig @mscdex @nodejs-github-bot