Skip to content
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

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

trivikr
Copy link
Member

@trivikr trivikr 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-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Sep 26, 2017
@hiroppy hiroppy 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

@mcollina mcollina 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
server.on(
'stream',
common.mustCall((stream) => {
const errorJSON = {
Copy link
Contributor

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
Member Author

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

@BridgeAR
Copy link
Member

@trivikr this needs a rebase

@trivikr
Copy link
Member Author

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
Member Author

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.

@trivikr trivikr force-pushed the test-http2-server-destroy branch 3 times, most recently from a7609e2 to 48e5d2d Compare September 30, 2017 08:05
@trivikr
Copy link
Member Author

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

@mcollina mcollina 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
Member Author

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

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.

@BridgeAR BridgeAR closed this Oct 1, 2017
@trivikr trivikr deleted the test-http2-server-destroy branch October 1, 2017 23:45
@trivikr
Copy link
Member Author

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

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

Quick ping re: backport

@trivikr trivikr mentioned this pull request Oct 7, 2017
Closed
@trivikr
Copy link
Member Author

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 free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BridgeAR BridgeAR BridgeAR approved these changes

@cjihrig cjihrig cjihrig approved these changes

@jasnell jasnell jasnell approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
http2 Issues or PRs related to the http2 subsystem. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.