test: http2 client settings errors #16096

trivikr · 2017-10-09T03:12:03Z

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 · 2017-10-09T20:21:37Z

test/parallel/test-http2-client-settings-errors.js

+
+    const errorMustCall = common.expectsError(test.error, 2);
+    const errorMustNotCall = common.mustNotCall(
+      `${test.error.code} should emit on ${test.type}`


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

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 · 2017-10-09T20:23:58Z

test/parallel/test-http2-client-settings-errors.js

+runTest(tests.shift());
+
+function runTest(test) {
+  const server = http2.createServer(common.mustNotCall());


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?

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.

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.

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

apapirovski · 2017-10-09T20:27:55Z

test/parallel/test-http2-session-settings.js

-      client.settings({
-        maxHeaderListSize: 1
-      });
+server.on(


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.

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.

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 👍

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 · 2017-10-13T17:31:57Z

Can someone create CI link for this pull request?

joyeecheung · 2017-10-14T14:01:36Z

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

trivikr · 2017-10-14T15:29:49Z

Fixed linting errors and rebased from upstream/master

joyeecheung · 2017-10-14T15:49:23Z

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

apapirovski · 2017-10-14T16:28:59Z

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

Refs: nodejs#14985

trivikr · 2017-10-14T20:49:10Z

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

jasnell · 2017-10-15T22:21:23Z

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

mcollina · 2017-10-17T08:13:51Z

Failures are unrelated.

mcollina

LGTM

mcollina · 2017-10-17T08:19:23Z

Landing this.

mcollina · 2017-10-17T08:27:30Z

Landed as c3eeb28.

Refs: #14985 PR-URL: #16096 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Refs: nodejs/node#14985 PR-URL: nodejs/node#16096 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Refs: #14985 PR-URL: #16096 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 9, 2017

mscdex added the http2 Issues or PRs related to the http2 subsystem. label Oct 9, 2017

apapirovski reviewed Oct 9, 2017

View reviewed changes

refack self-assigned this Oct 10, 2017

trivikr force-pushed the test-http2-session-settings branch from ffdc72d to efa4ab1 Compare October 10, 2017 00:45

apapirovski approved these changes Oct 10, 2017

View reviewed changes

trivikr force-pushed the test-http2-session-settings branch from efa4ab1 to b16419c Compare October 10, 2017 01:56

jasnell approved these changes Oct 12, 2017

View reviewed changes

trivikr force-pushed the test-http2-session-settings branch from b16419c to 262beed Compare October 14, 2017 15:29

test: http2 client settings errors

a1bf7a0

Refs: nodejs#14985

trivikr force-pushed the test-http2-session-settings branch from 262beed to a1bf7a0 Compare October 14, 2017 20:47

mcollina approved these changes Oct 17, 2017

View reviewed changes

mcollina assigned mcollina and unassigned refack Oct 17, 2017

mcollina closed this Oct 17, 2017

mcollina pushed a commit that referenced this pull request Oct 17, 2017

test: http2 client settings errors

c3eeb28

Refs: #14985 PR-URL: #16096 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 18, 2017

test: http2 client settings errors

375bd94

Refs: nodejs/node#14985 PR-URL: nodejs/node#16096 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

MylesBorins pushed a commit that referenced this pull request Oct 23, 2017

test: http2 client settings errors

50b4965

Refs: #14985 PR-URL: #16096 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

MylesBorins added the dont-land-on-v6.x label Nov 16, 2017

trivikr deleted the test-http2-session-settings branch February 8, 2018 05:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: http2 client settings errors #16096

test: http2 client settings errors #16096

trivikr commented Oct 9, 2017

apapirovski Oct 9, 2017 •

edited

Loading

trivikr Oct 10, 2017

apapirovski Oct 9, 2017

trivikr Oct 10, 2017

apapirovski Oct 10, 2017

trivikr Oct 10, 2017

apapirovski Oct 9, 2017

trivikr Oct 10, 2017

apapirovski Oct 10, 2017 •

edited

Loading

trivikr Oct 10, 2017

apapirovski left a comment

trivikr commented Oct 13, 2017

joyeecheung commented Oct 14, 2017

trivikr commented Oct 14, 2017

joyeecheung commented Oct 14, 2017

apapirovski commented Oct 14, 2017 •

edited

Loading

trivikr commented Oct 14, 2017

jasnell commented Oct 15, 2017

mcollina commented Oct 17, 2017

mcollina left a comment

mcollina commented Oct 17, 2017

mcollina commented Oct 17, 2017

test: http2 client settings errors #16096

test: http2 client settings errors #16096

Conversation

trivikr commented Oct 9, 2017

Checklist

Affected core subsystem(s)

apapirovski Oct 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apapirovski Oct 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apapirovski left a comment

Choose a reason for hiding this comment

trivikr commented Oct 13, 2017

joyeecheung commented Oct 14, 2017

trivikr commented Oct 14, 2017

joyeecheung commented Oct 14, 2017

apapirovski commented Oct 14, 2017 • edited Loading

trivikr commented Oct 14, 2017

jasnell commented Oct 15, 2017

mcollina commented Oct 17, 2017

mcollina left a comment

Choose a reason for hiding this comment

mcollina commented Oct 17, 2017

mcollina commented Oct 17, 2017

apapirovski Oct 9, 2017 •

edited

Loading

apapirovski Oct 10, 2017 •

edited

Loading

apapirovski commented Oct 14, 2017 •

edited

Loading