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: more AsyncWrap constructor args validation tests #16025

Closed
wants to merge 2 commits into from
Closed

test: more AsyncWrap constructor args validation tests #16025

wants to merge 2 commits into from

Conversation

bwhitty
Copy link
Contributor

@bwhitty bwhitty commented Oct 6, 2017 •
edited by BridgeAR
Loading

Fixes a line of uncovered code in the AsyncWrap class's constructor.
Specifically this covers validtion of the 'promiseResolve' argument.

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)

test

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 6, 2017
@bwhitty bwhitty force-pushed the async-wrap-constructor-tests branch from 09e5cbc to 06f4599 Compare October 6, 2017 19:12
@bwhitty bwhitty changed the title more AsyncWrap constructor args validation tests test: more AsyncWrap constructor args validation tests Oct 6, 2017
BridgeAR
BridgeAR reviewed Oct 6, 2017
View reviewed changes
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

LGTM if CI is green

@bwhitty
test: more AsyncWrap constructor validation tests
34b993f 
Fixes a line of uncovered code in the AsyncWrap class's constructor.
Specifically this covers validtion of the 'promiseResolve' argument.
@bwhitty bwhitty force-pushed the async-wrap-constructor-tests branch from 06f4599 to 34b993f Compare October 6, 2017 23:15
@Trott Trott added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Oct 7, 2017
@refack refack self-assigned this Oct 10, 2017
@joyeecheung
Copy link
Member

CI: https://ci.nodejs.org/job/node-test-pull-request/10624/

@lance
Copy link
Member

lance commented Oct 13, 2017

Appears to be a linting error.

not ok 17 - /usr/home/iojs/build/workspace/node-test-linter/test/parallel/test-async-wrap-constructor.js 
  ---
  message: Line 10 exceeds the maximum line length of 80.
  severity: error
  data:
    line: 10
    column: 1
    ruleId: max-len
  ...
+ exit 1

@bwhitty can you change the offending line to be fewer than 80 characters?

Maybe something like.

  for (const field of
    ['init', 'before', 'after', 'destroy', 'promiseResolve']) {

You can validate your changes without having to run the full test suite by running

$ make lint-js

Thanks for the contribution!

refack
refack approved these changes Oct 14, 2017
View reviewed changes
test/parallel/test-async-wrap-constructor.js Outdated
@@ -7,7 +7,7 @@ const assert = require('assert');
const async_hooks = require('async_hooks');

for (const badArg of [0, 1, false, true, null, 'hello']) {
for (const field of ['init', 'before', 'after', 'destroy']) {
for (const field of ['init', 'before', 'after', 'destroy', 'promiseResolve']) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Another passible way to reduce line length:

  const hookNames = ['init', 'before', 'after', 'destroy', 'promiseResolve'];
  for (const field of hookNames) {

@bwhitty
Copy link
Contributor Author

bwhitty commented Oct 16, 2017

My bad, should have ran the linter locally first! Line length issue fixed!

@joyeecheung
Copy link
Member

CI: https://ci.nodejs.org/job/node-test-pull-request/10736/

@BridgeAR BridgeAR assigned BridgeAR and unassigned refack Oct 18, 2017
BridgeAR pushed a commit that referenced this pull request Oct 19, 2017
@bwhitty @BridgeAR
test: more AsyncWrap constructor validation tests
ba999e1 
Fixes a line of uncovered code in the AsyncWrap class's constructor.
Specifically this covers validtion of the 'promiseResolve' argument.

PR-URL: #16025
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
@BridgeAR
Copy link
Member

Landed in ba999e1

Thanks for the PR, and congratulations on becoming a Node.js Contributor 🎉 !

@BridgeAR BridgeAR closed this Oct 19, 2017
MylesBorins pushed a commit that referenced this pull request Oct 23, 2017
@bwhitty @MylesBorins
test: more AsyncWrap constructor validation tests
2e2c09e 
Fixes a line of uncovered code in the AsyncWrap class's constructor.
Specifically this covers validtion of the 'promiseResolve' argument.

PR-URL: #16025
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 26, 2017
@bwhitty @addaleax
test: more AsyncWrap constructor validation tests
8e8f6f4 
Fixes a line of uncovered code in the AsyncWrap class's constructor.
Specifically this covers validtion of the 'promiseResolve' argument.

PR-URL: nodejs/node#16025
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
@bwhitty @addaleax
test: more AsyncWrap constructor validation tests
359d7e9 
Fixes a line of uncovered code in the AsyncWrap class's constructor.
Specifically this covers validtion of the 'promiseResolve' argument.

PR-URL: nodejs/node#16025
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@refack refack refack approved these changes

@jasnell jasnell jasnell approved these changes

@benjamingr benjamingr benjamingr approved these changes

@lpinca lpinca lpinca approved these changes

@cjihrig cjihrig cjihrig approved these changes

@tniessen tniessen tniessen approved these changes

@hiroppy hiroppy hiroppy approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

Assignees

@BridgeAR BridgeAR

Labels
code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.