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: Refactor test-http2-compat-serverresponse-finished.js #21929

Closed
wants to merge 1 commit into from

Conversation

antsmartian
Copy link
Contributor

@antsmartian antsmartian commented Jul 22, 2018

Just a simple refactor change:

  1. Changed from strictEqual to assert.ok where I left its needed.
  2. Arrow functions.
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

assert.strictEqual(response.socket, undefined);
assert.strictEqual(response.connection, undefined);
response.on('finish', common.mustCall(() => {
assert.ok(!response.socket);
Copy link
Member

@lpinca lpinca Jul 22, 2018

Choose a reason for hiding this comment

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

I see this kind of change being done frequently recently and I think this is not always a good idea. If there are other tests that cover the real value then it's ok, if not we are hiding the real type by coercing to a boolean.

What I mean is that if a regression is introduced where response.socket is set to null (or any other falsy value) instead of undefined, with this change, we will not notice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lpinca I got your point. I even thought about it before I did the changes, but in this context, when the response is finished, its gonna be undefined. So socket and connection are no more valid, I felt it was fine to use ok. But yeah, if some other logic expects the socket to be undefined and we pass null, we gonna cause a regression. Good point.

@antsmartian antsmartian force-pushed the refactor branch 2 times, most recently from e9106e1 to 96a8979 Compare July 22, 2018 11:01
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

I agree with lpinca's point about the assert.ok though.

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM, but please change the assert.ok() calls back. I think it's fine for language-level APIs, but we should be more strict with the APIs we provide for the reasons previously noted.

@maclover7
Copy link
Contributor

ping @antsmartian

@maclover7 maclover7 added the wip Issues and PRs that are still a work in progress. label Jul 28, 2018
@antsmartian
Copy link
Contributor Author

@maclover7 Oops, sorry. Will update the PR in an hour or so.

@antsmartian
Copy link
Contributor Author

antsmartian commented Jul 28, 2018

@maclover7 Ping :)

@maclover7 maclover7 added test Issues and PRs related to the tests. http2 Issues or PRs related to the http2 subsystem. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed wip Issues and PRs that are still a work in progress. labels Jul 31, 2018
@maclover7
Copy link
Contributor

@trivikr
Copy link
Member

trivikr commented Aug 1, 2018

Landed in f2518c4

@trivikr trivikr closed this Aug 1, 2018
trivikr pushed a commit that referenced this pull request Aug 1, 2018
PR-URL: #21929
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Aug 1, 2018
PR-URL: #21929
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@antsmartian antsmartian deleted the refactor branch August 4, 2018 08:39
kjin pushed a commit to kjin/node that referenced this pull request Sep 25, 2018
PR-URL: nodejs#21929
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
kjin pushed a commit to kjin/node that referenced this pull request Oct 16, 2018
PR-URL: nodejs#21929
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this pull request Oct 17, 2018
Backport-PR-URL: #22850
PR-URL: #21929
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Oct 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. http2 Issues or PRs related to the http2 subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants