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.

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

test: Http2Stream redundant call to shutdown and call passing only callback #15612

Closed
wants to merge 1 commit into from

Conversation

@trivikr
Copy link
Member

trivikr commented Sep 25, 2017

This PR includes test cases for the handling of:

  • redundant call of stream.session.shutdown()
  • calling stream.session.shutdown() with only callback (no options)

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

@mscdex mscdex added the http2 label Sep 25, 2017
Copy link
Member

apapirovski left a comment

Thanks for the work @trivikr! I think in general it's better to do these as one test since the 1st test is strictly a subset of the 2nd test. If the first mustCall doesn't fire in the 2nd test then the results are equivalent.

I would also suggest expanding this to cover this[kState].destroyed || this[kState].destroying behaviour (throws). (Feel free to add comments to annotate the different steps.)

stream.session.shutdown(common.mustCall(() => {
assert.strictEqual(
stream.session.shutdown(),
undefined

This comment has been minimized.

Copy link
@apapirovski

apapirovski Sep 25, 2017

Member

I think it's better if this also checks that it doesNotThrow.

This comment has been minimized.

Copy link
@trivikr

trivikr Sep 25, 2017

Author Member

assert.strictEqual will ensure that no error is thrown
Tested on repl

server.on('stream', common.mustCall((stream) => {
stream.session.shutdown(common.mustCall(() => {
assert.strictEqual(
stream.session.shutdown(),

This comment has been minimized.

Copy link
@apapirovski

apapirovski Sep 25, 2017

Member

I would suggest adding common.mustNotCall() so that it's clear that the callback only executes once.

This comment has been minimized.

Copy link
@trivikr

trivikr Sep 25, 2017

Author Member

Added!


// Test blank return when a stream.session.shutdown is called twice
server.on('stream', common.mustCall((stream) => {
stream.session.shutdown(common.mustCall(() => {

This comment has been minimized.

Copy link
@apapirovski

apapirovski Sep 25, 2017

Member

I would also run another stream.session.shutdown(common.mustNotCall()) outside of the callback. If there are two calls in a row, it shouldn't throw or execute a callback a 2nd time. (This would cover a case where someone moves the shuttingDown setting into a nextTick or event or something, or just removes it period.)

This comment has been minimized.

Copy link
@trivikr

trivikr Sep 25, 2017

Author Member

Added!

@trivikr

This comment has been minimized.

Copy link
Member Author

trivikr commented Sep 25, 2017

I agree to comment by @apapirovski #15612 (review) that the test test-http2-server-shutdown-redundant.js is a subset of test in test-http2-server-shutdown-only-callback.js
I've kept them separate as they serve specific purpose.

The test to cover this[kState].destroyed || this[kState].destroying was added in my first PR with NodeJS at #15597

@apapirovski

This comment has been minimized.

Copy link
Member

apapirovski commented Sep 25, 2017

Yeah, my bad re: the 2nd point. Should've realized, haha. :)

I'm still not sure I understand what test-http2-server-shutdown-only-callback.js does that the other test doesn't. It seems to me like a naming and comment problem rather than a distinct test. I could be misunderstanding though...

trivikr pushed a commit to trivikr/node that referenced this pull request Sep 25, 2017
@trivikr

This comment has been minimized.

Copy link
Member Author

trivikr commented Sep 25, 2017

I've removed the test test-http2-server-shutdown-only-callback.js and added a comment in test-http2-server-shutdown-redundant.js that it also tests only callback use case.

@trivikr

This comment has been minimized.

Copy link
Member Author

trivikr commented Sep 25, 2017

When the commits are squashed before merge, the commit message should be:

test: Http2Stream redundant shutdown and single callback

Refs: #14985
@apapirovski

This comment has been minimized.

Copy link
Member

apapirovski commented Sep 25, 2017

Thanks @trivikr! 👍 I realize these were sort of nits but just to explain my reasoning:

There are currently over 1200+ tests and it's growing all the time, so as much as possible it's good to avoid highly specialized test cases if the same can be accomplished by a single slightly longer test case that goes through a few stages (as long as it doesn't compromise the test result). That way we can avoid having duplicate code and unnecessary amount of buildup and teardown of tests. (If you look at the node CI, it runs all the tests on some truly ancient/slow systems.)

See some of these other http2 tests:
https://github.com/nodejs/node/blob/master/test/parallel/test-http2-compat-serverresponse-end.js
https://github.com/nodejs/node/blob/master/test/parallel/test-http2-compat-serverresponse-destroy.js

@trivikr

This comment has been minimized.

Copy link
Member Author

trivikr commented Sep 25, 2017

I completely agree with @apapirovski
The command make test takes around 20 minutes to run on MacBook Pro (Retina, 13-inch, Early 2015)

It's difficult to run single test also. I went through Makefile and came up with a python command to runs single test

node/Makefile

Line 204 in 4f4d55d

$(PYTHON) tools/test.py --mode=release -J \

Questions:

  • Is easing development by providing an ability to debug/run single tests being tracked somewhere? If the support is already available, can it be updated in writing tests guide
  • Does NodeJS plan to move a easier/faster test framework like Jest?

The above issues weren't blocking me for this task, so I didn't create issues.
We can create issues/tasks and follow up to get more developers onboard.

@bnoordhuis

This comment has been minimized.

Copy link
Member

bnoordhuis commented Sep 25, 2017

Is easing development by providing an ability to debug/run single tests being tracked somewhere?

It's documented in CONTRIBUTING.md, if that is what you mean. You are welcome to add it to the 'writing tests' guide though.

Does NodeJS plan to move a easier/faster test framework like Jest?

No.

@trivikr trivikr force-pushed the trivikr:http2-test-stream-shutdown branch from 328081e to e853dc4 Sep 25, 2017
@trivikr

This comment has been minimized.

Copy link
Member Author

trivikr commented Sep 25, 2017

Squashed all commits into a single one, and force pushed it my private branch.

@lpinca
lpinca approved these changes Sep 26, 2017
@trivikr

This comment has been minimized.

Copy link
Member Author

trivikr commented Sep 28, 2017

What's the procedure to merge these changes to master? Do we have wait for certain time for approvals before merging?

@BridgeAR

This comment has been minimized.

@trivikr trivikr force-pushed the trivikr:http2-test-stream-shutdown branch from e853dc4 to 1498dbb Sep 29, 2017
@trivikr

This comment has been minimized.

Copy link
Member Author

trivikr commented Sep 29, 2017

@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Sep 29, 2017

@trivikr ... yes, PRs generally must sit for at least 48 hours during the week and 72 hours over the weekend before they can be landed. Exception is given only to trivial doc updates. Also, things must go through a CI test run.

@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Sep 29, 2017

Oh, and I see you added a merge commit. Generally, we always use rebase and avoid using merge commits. Before I get this landed, can I ask you to drop the merge commit and rebase from master?

@trivikr trivikr force-pushed the trivikr:http2-test-stream-shutdown branch from 695540e to 63c865c Sep 30, 2017
@trivikr

This comment has been minimized.

Copy link
Member Author

trivikr commented Sep 30, 2017

Dropped the merge commit and performed git rebase as suggested by @jasnell and documentation

@jasnell

This comment has been minimized.

@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Oct 1, 2017

Landed in 27d8202

@BridgeAR BridgeAR closed this Oct 1, 2017
BridgeAR added a commit that referenced this pull request Oct 1, 2017
PR-URL: #15612
Refs: #14985
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@trivikr

This comment has been minimized.

Copy link
Member Author

trivikr commented Oct 1, 2017

Thanks @BridgeAR
This is my first PR to NodeJS - a small step to contribute more to this awesome project :-)

@trivikr trivikr deleted the trivikr:http2-test-stream-shutdown branch Oct 1, 2017
@apapirovski

This comment has been minimized.

Copy link
Member

apapirovski commented Oct 1, 2017

Congrats @trivikr! Thanks for contributing 🎉

MylesBorins added a commit that referenced this pull request Oct 3, 2017
PR-URL: #15612
Refs: #14985
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins MylesBorins mentioned this pull request Oct 3, 2017
MylesBorins added a commit that referenced this pull request Oct 3, 2017
PR-URL: #15612
Refs: #14985
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
addaleax added a commit to addaleax/ayo that referenced this pull request Oct 4, 2017
PR-URL: nodejs/node#15612
Refs: nodejs/node#14985
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins added a commit that referenced this pull request Oct 11, 2017
PR-URL: #15612
Refs: #14985
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Luigi Pinca <luigipinca@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
Projects
None yet
10 participants
You can’t perform that action at this time.