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 destroy server before shutdown #15597

Closed
wants to merge 1 commit into from
Closed

test: Http2Stream destroy server before shutdown #15597

wants to merge 1 commit into from

Conversation

trivikr
Copy link
Member

@trivikr trivikr commented Sep 25, 2017

This PR includes a test case for the handling of stream.session.shutdown() when session is already destroyed.
This is my first PR in node, will add more tests for http2 after getting this reviewed/merged.

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 25, 2017
@mscdex mscdex added the http2 Issues or PRs related to the http2 subsystem. label Sep 25, 2017
lpinca
lpinca reviewed Sep 25, 2017
View reviewed changes
test/parallel/test-http2-server-destroy-before-shutdown.js Outdated
'use strict';

const common = require('../common');
if (!common.hasCrypto) common.skip('missing crypto');
Copy link
Member

Choose a reason for hiding this comment

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

Nit: can you please put the body on the next line? I think it is the dominant style throughout the code base.

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!

@lpinca
Copy link
Member

lpinca commented Sep 25, 2017

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

apapirovski
apapirovski reviewed Sep 25, 2017
View reviewed changes
test/parallel/test-http2-server-destroy-before-shutdown.js Outdated
() => stream.session.shutdown(),
common.expectsError({
code: 'ERR_HTTP2_INVALID_SESSION',
message: /^The session has been destroyed$/
Copy link
Member

Choose a reason for hiding this comment

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

This probably doesn't need to be a RegExp.

Also, for future reference, common.expectsError can take () => stream.session.shutdown() as the first argument and the error object as the 2nd. (No need for assert.throws.)

Copy link
Member

Choose a reason for hiding this comment

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

For example:

common.expectsError(
  () => stream.session.shutdown(),
  {
    code: 'ERR_HTTP2_INVALID_SESSION',
    type: Error,
    message: 'The session has been destroyed'
  }
);

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!

jasnell
jasnell approved these changes Sep 25, 2017
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM with the suggested change to use common.expectsError

@trivikr trivikr mentioned this pull request Sep 25, 2017
Closed
3 tasks
@trivikr
Copy link
Member Author

trivikr commented Sep 25, 2017

When the commits in this PR are squashed before merge, the commit message must be:

test: Http2Stream destroy server before shutdown
Refs: #14985

@lpinca
Copy link
Member

lpinca commented Sep 25, 2017 •
edited
Loading

@trivikr if you want you can squash now, force push and save us some work :)

@trivikr trivikr force-pushed the http2-test-destroy-before-shutdown branch from 16810b8 to 0cef487 Compare September 25, 2017 20:03
@trivikr
Copy link
Member Author

trivikr commented Sep 25, 2017

@lpinca Done with squash and force push!

@apapirovski
Copy link
Member

apapirovski commented Sep 25, 2017 •
edited
Loading

Btw I think you need to update your git user.name and user.email

$ git config --global user.name "J. Random User"
$ git config --global user.email "j.random.user@example.com"

See https://github.com/nodejs/node/blob/master/CONTRIBUTING.md#setting-up-your-local-environment

Right now your commits don't seem to be associating properly.

@trivikr trivikr force-pushed the http2-test-destroy-before-shutdown branch from 0cef487 to e27cb36 Compare September 25, 2017 21:22
@trivikr
Copy link
Member Author

trivikr commented Sep 25, 2017

Thanks @apapirovski
There was a typo in my email ID. I've updated the commit with email ID associated with my github account.

@BridgeAR
Copy link
Member

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

@trivikr trivikr force-pushed the http2-test-destroy-before-shutdown branch from e27cb36 to 48fa0d8 Compare September 29, 2017 03:12
@trivikr
Copy link
Member Author

trivikr commented Sep 29, 2017

@BridgeAR rebase complete!
make -j4 test passes after rebase

@trivikr trivikr force-pushed the http2-test-destroy-before-shutdown branch from 285c729 to 48fa0d8 Compare September 30, 2017 08:10
@trivikr trivikr force-pushed the http2-test-destroy-before-shutdown branch from 48fa0d8 to 45ffc5f Compare September 30, 2017 08:11
@trivikr
Copy link
Member Author

trivikr commented Sep 30, 2017 •
edited
Loading

@BridgeAR I removed the merge commit and performed rebase as per the documentation
If you create new CI and it's successful, this PR will be safe to merge to master.

@jasnell
Copy link
Member

jasnell commented Oct 1, 2017

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

@BridgeAR
Copy link
Member

BridgeAR commented Oct 1, 2017

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

@BridgeAR
Copy link
Member

BridgeAR commented Oct 2, 2017

Landed in 5dd6583

@BridgeAR BridgeAR closed this Oct 2, 2017
BridgeAR pushed a commit that referenced this pull request Oct 2, 2017
@trivikr @BridgeAR
test: Http2Stream destroy server before shutdown
5dd6583 
PR-URL: #15597
Refs: #14985
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@trivikr trivikr deleted the http2-test-destroy-before-shutdown branch October 2, 2017 06:44
MylesBorins pushed a commit that referenced this pull request Oct 3, 2017
@trivikr @MylesBorins
test: Http2Stream destroy server before shutdown
1359cb9 
PR-URL: #15597
Refs: #14985
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins MylesBorins mentioned this pull request Oct 3, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Oct 3, 2017
@trivikr @MylesBorins
test: Http2Stream destroy server before shutdown
71897de 
PR-URL: #15597
Refs: #14985
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
addaleax pushed a commit to addaleax/ayo that referenced this pull request Oct 4, 2017
@trivikr @addaleax
test: Http2Stream destroy server before shutdown
8a9f1d5 
PR-URL: nodejs/node#15597
Refs: nodejs/node#14985
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Oct 11, 2017
@trivikr @MylesBorins
test: Http2Stream destroy server before shutdown
7842f63 
PR-URL: #15597
Refs: #14985
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@apapirovski apapirovski apapirovski left review comments

@BridgeAR BridgeAR BridgeAR approved these changes

@lpinca lpinca lpinca approved these changes

@jasnell jasnell jasnell 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.
8 participants
@trivikr @lpinca @apapirovski @BridgeAR @jasnell @mscdex @MylesBorins @nodejs-github-bot