test: add case to test-http-methods.js #14773
Conversation
Add more case to check existence of DELETE and PUT methods. Refs: nodejs#14544
|
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. |
There was a problem hiding this comment.
@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!
test/parallel/test-http-methods.js
Outdated
| // This test ensures all http methods from HTTP parser are exposed | ||
| // to http library | ||
|
|
||
| var methods = [ |
test/parallel/test-http-methods.js
Outdated
| 'MKCALENDAR', | ||
| 'LINK', | ||
| 'UNLINK' | ||
| ]; | ||
| assert(Array.isArray(http.METHODS)); | ||
| assert(http.METHODS.length > 0); |
There was a problem hiding this comment.
This check is now redundant, you may safely remove it.
test/parallel/test-http-methods.js
Outdated
| assert(http.METHODS.includes('POST')); | ||
| assert(http.METHODS.includes('PUT')); | ||
| assert.deepStrictEqual(util._extend([], http.METHODS), http.METHODS.sort()); | ||
| assert.equal(methods.length, http.METHODS.length); |
There was a problem hiding this comment.
This is covered by the next assert statement, so I'd say this is redundant. By the way, please use strictEqual() instead of equal() in such cases :)
test/parallel/test-http-methods.js
Outdated
| assert(http.METHODS.includes('PUT')); | ||
| assert.deepStrictEqual(util._extend([], http.METHODS), http.METHODS.sort()); | ||
| assert.equal(methods.length, http.METHODS.length); | ||
| assert.deepStrictEqual(methods.sort(), http.METHODS.sort()); |
There was a problem hiding this comment.
Previously this test asserted that the http.METHODS array is alphabetically sorted. So as not to change the test expectations, you should either drop .sort() from http.METHODS, or drop both .sort()s and make the array in this test sorted beforehand. I'd prefer the latter way, but one might find the former one conveying the intention more clearly, so I'm fine with both.
There was a problem hiding this comment.
Aside from that, can you please swap the order of arguments here? According to the function's signature, first argument should be an actual value and second one an expected value.
|
@aqrln I just updated it. Thanks |
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>
|
Landed in 1268737. Thanks, and hope to see you contributing again! |
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>
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>
Add more case to check existence of DELETE and PUT methods.
Refs: #14544
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
test http