Skip to content

test: Http2Stream server stream session destroy tests in one file#15624

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
Closed
trivikr wants to merge 1 commit into from
Closed

test: Http2Stream server stream session destroy tests in one file #15624

trivikr wants to merge 1 commit into from

Conversation

@trivikr
Copy link
Member

@trivikrtrivikr commented Sep 26, 2017

This PR includes all server stream session destroy tests in one file

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

@nodejs-github-botnodejs-github-bot added the test Issues and PRs related to the tests. label Sep 26, 2017
@hiroppyhiroppy added the http2 Issues or PRs related to the http2 subsystem. label Sep 26, 2017
mcollina
mcollina approved these changes Sep 26, 2017
View reviewed changes
Copy link
Member

@mcollinamcollina left a comment

Choose a reason for hiding this comment

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

LGTM

cjihrig
cjihrig approved these changes Sep 26, 2017
View reviewed changes
test/parallel/test-http2-server-stream-session-destroy.js Outdated
Copy link
Contributor

@cjihrigcjihrigSep 26, 2017

Choose a reason for hiding this comment

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

Can you name this something else since it's not actually JSON?

Copy link
MemberAuthor

@trivikrtrivikrSep 26, 2017

Choose a reason for hiding this comment

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

Done!
Renamed it to errorObj

@trivikrtrivikrforce-pushed the test-http2-server-destroy branch from c3887df to a7609e2CompareSeptember 26, 2017 15:08
@BridgeAR
Copy link
Member

BridgeAR commented Sep 28, 2017

@trivikr this needs a rebase

@trivikr
Copy link
MemberAuthor

trivikr commented Sep 29, 2017

@BridgeAR rebase complete!

@hiroppy
Copy link
Member

hiroppy commented Sep 29, 2017

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

@trivikr
Copy link
MemberAuthor

trivikr commented Sep 29, 2017

@abouthiroppy The error in CI is the CONFLICT in 3-way merge https://ci.nodejs.org/job/node-test-commit/12680/console

This conflict is removal of first line // Flags: --expose-http2 from test files
I remember resolving this conflict while doing rebase in my private branch. The conflict is no longer shown in commits of this PR.

@trivikrtrivikrforce-pushed the test-http2-server-destroy branch 3 times, most recently from a7609e2 to 48e5d2dCompareSeptember 30, 2017 08:05
@trivikrtrivikrforce-pushed the test-http2-server-destroy branch from 48e5d2d to 3ffd096CompareSeptember 30, 2017 08:07
@trivikr
Copy link
MemberAuthor

trivikr commented Sep 30, 2017•
edited
Loading

@abouthiroppy Instead of merge, I performed rebase as explained in the documentation
If you trigger new CI and it's successful, it will be safe to merge this PR to master.

mcollina
mcollina approved these changes Sep 30, 2017
View reviewed changes
Copy link
Member

@mcollinamcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@jasnell
Copy link
Member

jasnell commented Oct 1, 2017

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

@trivikr
Copy link
MemberAuthor

trivikr commented Oct 1, 2017

The CI failed again. As per the console output, the failure was on linux and aix.
The test command runs successfully in my workspace: ./configure && make -j4 test on macOS
The command is given in test instructions

The CI documentation states that sometimes "flaky" tests on specific platforms might fail. Is this one of those failures?

@gibfahn
Copy link
Member

gibfahn commented Oct 1, 2017

The test failures on AIX and Centos 32-bit x64 Linux were unrelated to this PR, CI can be considered to have passed.

@gireeshpunathil
Copy link
Member

gireeshpunathil commented Oct 1, 2017

failure in AIX:

not ok 1760 sequential/test-child-process-pass-fd --- duration_ms: 1.640 severity: fail stack: |- events.js:182 throw er; // Unhandled 'error' event ^ Error: spawn /home/iojs/build/workspace/node-test-commit-aix/nodes/aix61-ppc64/out/Release/node EAGAIN at _errnoException (util.js:1018:13) at Process.ChildProcess._handle.onexit (internal/child_process.js:202:19) at onErrorNT (internal/child_process.js:390:16) at _combinedTickCallback (internal/process/next_tick.js:138:11) at process._tickCallback (internal/process/next_tick.js:180:9) at Function.Module.runMain (module.js:643:11) at startup (bootstrap_node.js:187:16) at bootstrap_node.js:605:3

While this does not hint any possibility of being related to this PR, I don't remember seeing this failure earlier, it is not listed as flaky either. @gibfahn - are you familiar with this failure, or do we have a tracker for this?

cenOS failure:

not ok 1688 inspector/test-break-when-eval --- duration_ms: 15.642 severity: fail stack: |- [test] Connecting to a child Node process [test] Testing /json/list [err] Debugger listening on ws://127.0.0.1:60225/1304e900-59e2-4300-b971-d7f781769510 [err] For help see https://nodejs.org/en/docs/inspector [err] [test] Setting up a debugger [err] Debugger attached. [err] [test] Breaking in the code [out] Ready! [out] [test] Step over console statement and test output [out] 0 3 [out] Timed out waiting for matching notification (Console output matching [0,3])) 1

No idea about this, but looks to be evidently unrelated.

@gibfahn
Copy link
Member

gibfahn commented Oct 1, 2017

@gireeshpunathil AIX failure is #15656

BridgeAR pushed a commit that referenced this pull request Oct 1, 2017
@trivikr@BridgeAR
test: consolidate http2 tests in one file
2adf680
PR-URL: #15624 Refs: #14985 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
@BridgeAR
Copy link
Member

BridgeAR commented Oct 1, 2017

Landed in 2adf680

@trivikr congraulations to your first commits to Node.js! 🎉
Just as a note - I changed the commit message because it was above 50 characters.

@BridgeARBridgeAR closed this Oct 1, 2017
@trivikrtrivikr deleted the test-http2-server-destroy branch October 1, 2017 23:45
@trivikr
Copy link
MemberAuthor

trivikr commented Oct 1, 2017

Thanks @BridgeAR
I'm working with NodeJS for more than a year, and happy to finally contribute to it. Writing tests was just the beginning, identifying and fixing bugs comes next in #15676 and will hopefully be able to add features in future :-)

@MylesBorins
Copy link
Contributor

MylesBorins commented Oct 3, 2017

This doesn't appear to land cleanly on v8.x. Would someone familiar with backport be willing to take a look?

addaleax pushed a commit to addaleax/ayo that referenced this pull request Oct 4, 2017
@trivikr@addaleax
test: consolidate http2 tests in one file
4b0c8d7
PR-URL: nodejs/node#15624 Refs: nodejs/node#14985 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
@MylesBorins
Copy link
Contributor

MylesBorins commented Oct 7, 2017

Quick ping re: backport

@trivikrtrivikr mentioned this pull request Oct 7, 2017
Closed
@trivikr
Copy link
MemberAuthor

trivikr commented Oct 7, 2017

v8.x backport PR created at #16072

trivikr added a commit to trivikr/node that referenced this pull request Oct 16, 2017
@trivikr
test: consolidate http2 tests in one file
fadf481
PR-URL: nodejs#15624 Refs: nodejs#14985 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 23, 2017
@trivikr@MylesBorins
test: consolidate http2 tests in one file
65c5d0e
PR-URL: #15624 Refs: #14985 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollinamcollinamcollina approved these changes

@jasnelljasnelljasnell approved these changes

@cjihrigcjihrigcjihrig approved these changes

@BridgeARBridgeARBridgeAR approved these changes

Assignees

No one assigned

Labels

http2Issues or PRs related to the http2 subsystem.testIssues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

11 participants

@trivikr@BridgeAR@hiroppy@jasnell@gibfahn@gireeshpunathil@MylesBorins@mcollina@cjihrig@lance@nodejs-github-bot