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: add constructor test to async-hooks #13096

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
8 participants
@Shadowbeetle
Contributor

Shadowbeetle commented May 18, 2017

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)

async-hooks

[refack edit: removed documentation checkbox, as it seems it is not needed for this PR]

Show outdated Hide outdated lib/async_hooks.js
@@ -77,13 +77,13 @@ function fatalError(e) {
class AsyncHook {
constructor({ init, before, after, destroy }) {
if (init && typeof init !== 'function')
if (typeof init !== 'undefined' && typeof init !== 'function')

This comment has been minimized.

@mscdex

mscdex May 18, 2017

Contributor

Why these changes?

@mscdex

mscdex May 18, 2017

Contributor

Why these changes?

This comment has been minimized.

@Shadowbeetle

Shadowbeetle May 18, 2017

Contributor

Because without them it will accept false without throwing, which I assume is not the expected behaviour

@Shadowbeetle

Shadowbeetle May 18, 2017

Contributor

Because without them it will accept false without throwing, which I assume is not the expected behaviour

This comment has been minimized.

@mscdex

mscdex May 18, 2017

Contributor

I see. Can we at least use init !== undefined instead? That is the form we use elsewhere in core.

@mscdex

mscdex May 18, 2017

Contributor

I see. Can we at least use init !== undefined instead? That is the form we use elsewhere in core.

This comment has been minimized.

@Shadowbeetle

Shadowbeetle May 19, 2017

Contributor

Of course

@Shadowbeetle

Shadowbeetle May 19, 2017

Contributor

Of course

Show outdated Hide outdated test/parallel/test-async-wrap-constructor.js
async_hooks.createHook({ init: 1 })
} catch (err) {
assert.equal(err.message, 'init must be a function')
}

This comment has been minimized.

@addaleax

addaleax May 18, 2017

Member

Can you re-phrase these using assert.throws? e.g.

assert.throws(() => {
  async_hooks.createHook({ init: 1 });
}, /^TypeError: init must be a function$/);

(Also, please make sure that make lint passes :))

@addaleax

addaleax May 18, 2017

Member

Can you re-phrase these using assert.throws? e.g.

assert.throws(() => {
  async_hooks.createHook({ init: 1 });
}, /^TypeError: init must be a function$/);

(Also, please make sure that make lint passes :))

This comment has been minimized.

@Shadowbeetle

Shadowbeetle May 18, 2017

Contributor

Thanks! I have updated the commit.

@Shadowbeetle

Shadowbeetle May 18, 2017

Contributor

Thanks! I have updated the commit.

Show outdated Hide outdated test/parallel/test-async-wrap-constructor.js
if (!common.hasCrypto) {
common.skip('missing crypto');
return;
}

This comment has been minimized.

@addaleax

addaleax May 18, 2017

Member

Is this block copy-pased or did you check that it is necessary?

@addaleax

addaleax May 18, 2017

Member

Is this block copy-pased or did you check that it is necessary?

This comment has been minimized.

@Shadowbeetle

Shadowbeetle May 18, 2017

Contributor

Shame on me, completely copy-pasted and unnecessary. I have updated the commit.

@Shadowbeetle

Shadowbeetle May 18, 2017

Contributor

Shame on me, completely copy-pasted and unnecessary. I have updated the commit.

@AndreasMadsen

This comment has been minimized.

Show comment
Hide comment
@AndreasMadsen

AndreasMadsen May 18, 2017

Member

Thanks for taking the time to fix this.

Because without them it will accept false without throwing, which I assume is not the expected behaviour

Could you add those details to the commit message, "test: add constructor test to async-hooks" is a little vague. Since this changes something in the actual async_hooks code you should also prefix the commit message with async_hooks: instead of test:.

Member

AndreasMadsen commented May 18, 2017

Thanks for taking the time to fix this.

Because without them it will accept false without throwing, which I assume is not the expected behaviour

Could you add those details to the commit message, "test: add constructor test to async-hooks" is a little vague. Since this changes something in the actual async_hooks code you should also prefix the commit message with async_hooks: instead of test:.

Show outdated Hide outdated test/parallel/test-async-wrap-constructor.js
const assert = require('assert');
const async_hooks = require('async_hooks');
assert.throws(() => {

This comment has been minimized.

@refack

refack May 18, 2017

Member

@Shadowbeetle you gave false as a previously failing argument. Could you add a set of tests with false?
This could be done in a double loop:

for (let badArg of [1, false, true, null, undefined, 'hello']) {
  for (let field of ['init', 'before', 'after', 'destroy']) {
    assert.throws(() => {
      async_hooks.createHook({ [field]: badArg });
    }, /^TypeError: before must be a function$/);
  }
}
@refack

refack May 18, 2017

Member

@Shadowbeetle you gave false as a previously failing argument. Could you add a set of tests with false?
This could be done in a double loop:

for (let badArg of [1, false, true, null, undefined, 'hello']) {
  for (let field of ['init', 'before', 'after', 'destroy']) {
    assert.throws(() => {
      async_hooks.createHook({ [field]: badArg });
    }, /^TypeError: before must be a function$/);
  }
}

This comment has been minimized.

@Shadowbeetle

Shadowbeetle May 19, 2017

Contributor

@refack Thanks, I have included this with a minor change in the commit.

@Shadowbeetle

Shadowbeetle May 19, 2017

Contributor

@refack Thanks, I have included this with a minor change in the commit.

@refack

refack approved these changes May 18, 2017

@refack

refack approved these changes May 19, 2017

💯

@refack

This comment has been minimized.

Show comment
Hide comment
@@ -0,0 +1,12 @@
'use strict';
require('../common');
const assert = require('assert');

This comment has been minimized.

@refack

refack May 19, 2017

Member

Could you add a couple of lines describing what this is testing
https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md

@refack

refack May 19, 2017

Member

Could you add a couple of lines describing what this is testing
https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md

Show outdated Hide outdated test/parallel/test-async-wrap-constructor.js
for (const field of ['init', 'before', 'after', 'destroy']) {
assert.throws(() => {
async_hooks.createHook({ [field]: badArg });
}, new RegExp(`^TypeError: ${field} must be a function$`), );

This comment has been minimized.

@refack

refack May 19, 2017

Member

I think the dangling , is a lint error

@refack

refack May 19, 2017

Member

I think the dangling , is a lint error

@Shadowbeetle

This comment has been minimized.

Show comment
Hide comment
@Shadowbeetle

Shadowbeetle May 19, 2017

Contributor

Sorry, an extra space sneaked in that broke the lint

Contributor

Shadowbeetle commented May 19, 2017

Sorry, an extra space sneaked in that broke the lint

@AndreasMadsen

This comment has been minimized.

Show comment
Hide comment
@AndreasMadsen

AndreasMadsen May 19, 2017

Member

@Shadowbeetle Sorry if what I said was poorly explained, but we need to conform to the commit message guidelines, you can read them here https://github.com/nodejs/node/blob/master/CONTRIBUTING.md#commit-message-guidelines

Something like this should be fine:

async_hooks: add constructor check to AsyncHook

This fixes the async_hooks.AsyncHook constructor such that it throws an error
when provided with falsy values other than undefined.
Member

AndreasMadsen commented May 19, 2017

@Shadowbeetle Sorry if what I said was poorly explained, but we need to conform to the commit message guidelines, you can read them here https://github.com/nodejs/node/blob/master/CONTRIBUTING.md#commit-message-guidelines

Something like this should be fine:

async_hooks: add constructor check to AsyncHook

This fixes the async_hooks.AsyncHook constructor such that it throws an error
when provided with falsy values other than undefined.
async_hooks: add constructor check to async-hooks
This fixes the async_hooks.AsyncHook constructor such that it throws an error
when provided with falsy values other than undefined.
@Shadowbeetle

This comment has been minimized.

Show comment
Hide comment
@Shadowbeetle

Shadowbeetle May 19, 2017

Contributor

@AndreasMadsen I see, sorry I should have read the guidelines more thoroughly. Updated the commit message.

Contributor

Shadowbeetle commented May 19, 2017

@AndreasMadsen I see, sorry I should have read the guidelines more thoroughly. Updated the commit message.

@AndreasMadsen

This comment has been minimized.

Show comment
Hide comment

AndreasMadsen added a commit that referenced this pull request May 21, 2017

async_hooks: add constructor check to async-hooks
This fixes the async_hooks.AsyncHook constructor such that it throws an
error when provided with falsy values other than undefined.

PR-URL: #13096
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
@AndreasMadsen

This comment has been minimized.

Show comment
Hide comment
@AndreasMadsen

AndreasMadsen May 21, 2017

Member

@Shadowbeetle Thanks for your contribution, the commit landed in 6fb27af

PS: I fixed the commit message linebreak and test comment on merge.

Member

AndreasMadsen commented May 21, 2017

@Shadowbeetle Thanks for your contribution, the commit landed in 6fb27af

PS: I fixed the commit message linebreak and test comment on merge.

@refack

This comment has been minimized.

Show comment
Hide comment
@refack

refack May 21, 2017

Member

Congrats @Shadowbeetle on your first contribution 🥇
And @AndreasMadsen on your first land 😉

Member

refack commented May 21, 2017

Congrats @Shadowbeetle on your first contribution 🥇
And @AndreasMadsen on your first land 😉

@AndreasMadsen

This comment has been minimized.

Show comment
Hide comment
@AndreasMadsen

AndreasMadsen May 21, 2017

Member

@refack Thanks. I have landed a few commits, but it has been a while :)

Member

AndreasMadsen commented May 21, 2017

@refack Thanks. I have landed a few commits, but it has been a while :)

@Shadowbeetle

This comment has been minimized.

Show comment
Hide comment
@Shadowbeetle

Shadowbeetle May 22, 2017

Contributor

thanks to everybody for all the help

Contributor

Shadowbeetle commented May 22, 2017

thanks to everybody for all the help

@jasnell jasnell referenced this pull request May 28, 2017

Closed

8.0.0 Release Proposal #12220

@gibfahn gibfahn referenced this pull request Jun 15, 2017

Closed

Auditing for 6.11.1 #230

2 of 3 tasks complete
@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Jul 17, 2017

Member

This should likely be part of a larger sync hooks backport

Member

MylesBorins commented Jul 17, 2017

This should likely be part of a larger sync hooks backport

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