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

http2: add tests for push stream error handling #15281

Closed

Conversation

Projects
None yet
5 participants
@apapirovski
Copy link
Member

commented Sep 8, 2017

Some test cases for pushStream edge behaviours:

  • Adds tests that cover errors for wrong arguments, as well as tests for error codes from nghttp2.

  • Also a test for the method head branch of pushStream (should close writeable stream immediately).

  • Fixes pushStream to emit NGHTTP2_ERR_STREAM_ID_NOT_AVAILABLE on session rather than
    stream.

Let me know if there's anything I can change. Thanks!

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)

http2, test

@apapirovski

This comment has been minimized.

Copy link
Member Author

commented Sep 13, 2017

@mcollina would you mind reviewing this? Thanks!

@mcollina
Copy link
Member

left a comment

LGTM with some nits addressed.

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

common.skip('missing crypto');
const assert = require('assert');
const http2 = require('http2');

This comment has been minimized.

Copy link
@mcollina

mcollina Sep 13, 2017

Member

can you add a quick description of the test here?

const req = client.request(headers);

let data = '';

This comment has been minimized.

Copy link
@mcollina

mcollina Sep 13, 2017

Member

can you please do req.setEncoding('utf8')?

common.skip('missing crypto');
const assert = require('assert');
const http2 = require('http2');

This comment has been minimized.

Copy link
@mcollina

mcollina Sep 13, 2017

Member

Can you add a quick description of this test here?

http2: add tests for push stream error handling
Add tests that cover errors for wrong arguments, as well as
tests for error codes from nghttp2. Fix pushStream to emit
NGHTTP2_ERR_STREAM_ID_NOT_AVAILABLE on session rather than
stream.

@apapirovski apapirovski force-pushed the apapirovski:patch-http2-pushstream-tests branch from 2451031 to 64109b3 Sep 13, 2017

@apapirovski

This comment has been minimized.

Copy link
Member Author

commented Sep 13, 2017

Thanks @mcollina. Feedback addressed and also rebased just in case.

@mcollina

This comment has been minimized.

Copy link
Member

commented Sep 13, 2017

@mcollina

This comment has been minimized.

Copy link
Member

commented Sep 13, 2017

Landed as 1aca135

@mcollina mcollina closed this Sep 13, 2017

mcollina added a commit that referenced this pull request Sep 13, 2017

http2: add tests for push stream error handling
Add tests that cover errors for wrong arguments, as well as
tests for error codes from nghttp2. Fix pushStream to emit
NGHTTP2_ERR_STREAM_ID_NOT_AVAILABLE on session rather than
stream.

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

@apapirovski apapirovski deleted the apapirovski:patch-http2-pushstream-tests branch Sep 15, 2017

addaleax added a commit to addaleax/ayo that referenced this pull request Sep 17, 2017

http2: add tests for push stream error handling
Add tests that cover errors for wrong arguments, as well as
tests for error codes from nghttp2. Fix pushStream to emit
NGHTTP2_ERR_STREAM_ID_NOT_AVAILABLE on session rather than
stream.

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

jasnell added a commit that referenced this pull request Sep 20, 2017

http2: add tests for push stream error handling
Add tests that cover errors for wrong arguments, as well as
tests for error codes from nghttp2. Fix pushStream to emit
NGHTTP2_ERR_STREAM_ID_NOT_AVAILABLE on session rather than
stream.

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

Qard pushed a commit to Qard/ayo that referenced this pull request Sep 21, 2017

http2: add tests for push stream error handling
Add tests that cover errors for wrong arguments, as well as
tests for error codes from nghttp2. Fix pushStream to emit
NGHTTP2_ERR_STREAM_ID_NOT_AVAILABLE on session rather than
stream.

PR-URL: nodejs/node#15281
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.