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: add case to test-http-methods.js #14773

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@okyantoro
Contributor

okyantoro commented Aug 11, 2017

Add more case to check existence of DELETE and PUT methods.

Refs: #14544

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test http

test: add case to test-http-methods.js
Add more case to check existence of DELETE and PUT methods.

Refs: #14544

@aqrln aqrln added the http label Aug 11, 2017

@aqrln

This comment has been minimized.

Show comment
Hide comment
@aqrln

aqrln Aug 11, 2017

Member

Hi and thanks for your contribution! This looks good, and is certainly an improvement to the test, but I wonder if it would be even better to check that all request methods Node's HTTP parser supports are properly exposed. That said, you or anyone else may do it as a separate PR, so LGTM either way.

Member

aqrln commented Aug 11, 2017

Hi and thanks for your contribution! This looks good, and is certainly an improvement to the test, but I wonder if it would be even better to check that all request methods Node's HTTP parser supports are properly exposed. That said, you or anyone else may do it as a separate PR, so LGTM either way.

@aqrln

aqrln approved these changes Aug 11, 2017

@okyantoro

This comment has been minimized.

Show comment
Hide comment
@okyantoro

okyantoro Aug 11, 2017

Contributor

Hi @aqrln @tniessen
I just updated the test case. to cover all methods in HTTP parser. I built array and just use deep equal to check the existence of the methods. Is it okay?

Contributor

okyantoro commented Aug 11, 2017

Hi @aqrln @tniessen
I just updated the test case. to cover all methods in HTTP parser. I built array and just use deep equal to check the existence of the methods. Is it okay?

@aqrln

@okyantoro yes, this approach is perfectly okay. I left a bunch of comments, sorry if it's a bit overwhelming, and let me know if you need any help. Thanks!

Show outdated Hide outdated test/parallel/test-http-methods.js
Show outdated Hide outdated test/parallel/test-http-methods.js
Show outdated Hide outdated test/parallel/test-http-methods.js
Show outdated Hide outdated test/parallel/test-http-methods.js
@okyantoro

This comment has been minimized.

Show comment
Hide comment
@okyantoro

okyantoro Aug 12, 2017

Contributor

@aqrln I just updated it. Thanks

Contributor

okyantoro commented Aug 12, 2017

@aqrln I just updated it. Thanks

@lpinca

lpinca approved these changes Aug 12, 2017

aqrln added a commit that referenced this pull request Aug 14, 2017

test: cover all HTTP methods that parser supports
Cover all request methods that Node's HTTP parser supports in
parallel/test-http-methods.

PR-URL: #14773
Refs: #14544
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@aqrln

This comment has been minimized.

Show comment
Hide comment
@aqrln

aqrln Aug 14, 2017

Member

Landed in 1268737. Thanks, and hope to see you contributing again!

Member

aqrln commented Aug 14, 2017

Landed in 1268737. Thanks, and hope to see you contributing again!

@aqrln aqrln closed this Aug 14, 2017

@okyantoro okyantoro deleted the okyantoro:test-http-methods branch Aug 14, 2017

addaleax added a commit that referenced this pull request Aug 14, 2017

test: cover all HTTP methods that parser supports
Cover all request methods that Node's HTTP parser supports in
parallel/test-http-methods.

PR-URL: #14773
Refs: #14544
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@addaleax addaleax referenced this pull request Aug 14, 2017

Merged

v8.4.0 proposal #14811

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

test: cover all HTTP methods that parser supports
Cover all request methods that Node's HTTP parser supports in
parallel/test-http-methods.

PR-URL: #14773
Refs: #14544
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@MylesBorins MylesBorins referenced this pull request Sep 20, 2017

Merged

v6.11.4 proposal #15506

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment