test: add constructor test to async-hooks

[Shadowbeetle]

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]

[mscdex]

Why these changes?

[Shadowbeetle]

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

[mscdex]

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

[Shadowbeetle]

Of course

[addaleax]

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 :))

[Shadowbeetle]

Thanks! I have updated the commit.

[addaleax]

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

[Shadowbeetle]

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

[AndreasMadsen]

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: .

[refack]

@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$/);
  }
}

[Shadowbeetle]

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

[refack]

💯

[refack]

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

[refack]

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]

I think the dangling , is a lint error

[Shadowbeetle]

Sorry, an extra space sneaked in that broke the lint

[AndreasMadsen]

@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.

[Shadowbeetle]

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

[AndreasMadsen]

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

[AndreasMadsen]

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

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

[refack]

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

[AndreasMadsen]

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

[Shadowbeetle]

thanks to everybody for all the help

[MylesBorins]

This should likely be part of a larger sync hooks backport