FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: refactor parallel/test-timer-close by BethGriggs · Pull Request #10517 · nodejs/node · GitHub

/ node Public

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

Closed
BethGriggs wants to merge 1 commit into
nodejs:masterfrom
BethGriggs:pr-test-timer-close
ConversationCommits (1)ChecksFiles changed
Closed

test: refactor parallel/test-timer-close#10517
BethGriggs wants to merge 1 commit into
nodejs:masterfrom
BethGriggs:pr-test-timer-close

Conversation

BethGriggs commented Dec 29, 2016 •
edited
Loading

Copy link
Copy Markdown
Member

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 added test Issues and PRs related to the tests. lts-watch-v6.x labels Dec 29, 2016

gibfahn commented Dec 29, 2016 •
edited
Loading

Copy link
Copy Markdown
Member

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

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

gibfahn approved these changes Dec 29, 2016
View reviewed changes

gibfahn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

CI is green, LGTM

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

jasnell Dec 29, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 approved these changes Dec 29, 2016
View reviewed changes

jasnell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Minor nit but otherwise LGTM

gibfahn approved these changes Dec 29, 2016
View reviewed changes

gibfahn left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 force-pushed the pr-test-timer-close branch from aa1919d to 4a78216 Compare January 3, 2017 10:29

gibfahn commented Jan 3, 2017

Copy link
Copy Markdown
Member

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

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

gibfahn commented Jan 3, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

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 commented Jan 3, 2017

Copy link
Copy Markdown
Member

Landed in 5a51955

gibfahn closed this Jan 3, 2017
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 18, 2017
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
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 mentioned this pull request Jan 25, 2017
Closed
targos pushed a commit that referenced this pull request Jan 28, 2017
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 mentioned this pull request Jan 29, 2017
Merged
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
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
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 commented Mar 8, 2017

Copy link
Copy Markdown
Contributor

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

BethGriggs deleted the pr-test-timer-close branch February 21, 2018 15:36
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
santigimeno santigimeno approved these changes
cjihrig cjihrig approved these changes
+2 more reviewers
Fishrock123 Fishrock123 approved these changes
gibfahn gibfahn approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

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

Back | FazBrowse Home | New Git URL