test: http2 client settings errors

[trivikr]

This code change adds tests for checking errors in client.settings()

Refs: #14985

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, http2

[apapirovski]

test.type does not appear to be defined in this test. I think it should be replaced with a plain string that says session. (Maybe the message isn't even necessary to be honest since it only emits on session.)

[trivikr]

Great catch @apapirovski
I'd copied this template from other tests which checks for errors which had types. I've replace ${test.type} with plain string session

[apapirovski]

I might be missing something but is there a reason the server needs to be started on each test run rather than having a single server for the whole test?

[trivikr]

The sessionError is thrown on the server.
A single server was used originally, but I was not able to override the error thrown on that server. So a new server was created per test.

[apapirovski]

Would it be possible to just do server.once('sessionError', fn) inside runTest and have all the code around creating a server and initiating it outside of runTest? If not, no worries.

[trivikr]

server.once('sessionError', fn) worked!
Thanks for the suggestion. I've updated the commit.

[apapirovski]

I'm not sure I'm following what's going on with the changes in this file. Is there a reason everything needs to be scoped within the stream handler? This looks like a lot of churn and I'm having trouble following why.

[trivikr]

I originally started working on this file and formatted the code using Prettier.
I created test-http2-client-settings-errors.js after coming across the template used by other tests which checked for errors.
I've scoped method assertSettings within stream handler, as it's only used within that block.

[apapirovski]

My concern is just that it's a bit easier to follow the file history (git blame) when changes aren't made just for the sake of changing code style, unless the clarity is a significant problem. That said, this is a nit so if no one else cares then this is fine. Thanks for the explanation 👍

[trivikr]

I agree that code change shouldn't be made just for changing code style.
I added changed in test-http2-session-settings.js as the formatting was already done. I'll remove it from the commit if more people raise it in the reviews.
Btw, I created an issue at #16115 to check if we can move to Javascript formatter like Prettier.

[apapirovski]

LGTM. Just nits above. 👍

[trivikr]

Can someone create CI link for this pull request?

[joyeecheung]

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

[trivikr]

Fixed linting errors and rebased from upstream/master

[joyeecheung]

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

[apapirovski]

Looks like on slower systems this test is failing because sessionError doesn't emit until after the next test started. To be honest, instead of trying to fix (nextTick or setImmediate would likely do it), I would recommend just getting rid of the current sessionError listener because we know those errors are forwarded there and that behaviour is tested elsewhere. Instead I would just add (at the top level scope, not within each test runner):

server.on('sessionError', () => {}); // not being tested

[trivikr]

Moved sessionError to top level scope as tests are failing on slower systems.

[jasnell]

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

[mcollina]

Failures are unrelated.

[mcollina]

LGTM

[mcollina]

Landing this.

[mcollina]

Landed as c3eeb28.