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.

Sign up for GitHub

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

Jump to bottom

refactor http.get expectedCount with common.mustCall #12668

Closed
wants to merge 2 commits into from
Closed

refactor http.get expectedCount with common.mustCall #12668

wants to merge 2 commits into from

Conversation

weewey
Copy link
Contributor

@weewey weewey commented Apr 26, 2017

refactor http.get expectedCount with common.mustCall

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Apr 26, 2017
@vsemozhetbyt
Copy link
Contributor

Linter:

  94:5  error  'expectResponseCount' is assigned a value but never used  no-unused-vars
  95:5  error  'responseCount' is assigned a value but never used        no-unused-vars
  95:5  error  'responseCount' is never reassigned. Use 'const' instead  prefer-const

It seems these variables are not needed anymore.

aqrln
aqrln reviewed Apr 26, 2017
View reviewed changes
test/parallel/test-https-strict.js
@@ -220,5 +219,4 @@ process.on('exit', () => {
assert.strictEqual(server1.requests.length, server1.expectCount);
assert.strictEqual(server2.requests.length, server2.expectCount);
assert.strictEqual(server3.requests.length, server3.expectCount);
assert.strictEqual(responseCount, expectResponseCount);
Copy link
Contributor

Choose a reason for hiding this comment

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

You should probably remove expectResponseCount usages too.

@mscdex mscdex added http Issues or PRs related to the http subsystem. https Issues or PRs related to the https subsystem. and removed http Issues or PRs related to the http subsystem. labels Apr 26, 2017
@weewey
Copy link
Contributor Author

weewey commented Apr 26, 2017

missed out on removing the variables. I have removed expectResponseCount and responseCount.

@vsemozhetbyt
Copy link
Contributor

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

Trott
Trott approved these changes Apr 26, 2017
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM.

@Trott
Copy link
Member

Trott commented Apr 27, 2017

Based on email conversation with @weewey, I believe the third commit with changes to test-https-simple.js is intended for a separate PR.

@weewey
Copy link
Contributor Author

weewey commented Apr 27, 2017

yes @Trott

I committed the changes in the same branch, as a result the commit was appended.

@weewey
Copy link
Contributor Author

weewey commented Apr 28, 2017

@Trott removed.

@Trott
Copy link
Member

Trott commented Apr 28, 2017

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

jasnell pushed a commit that referenced this pull request Apr 28, 2017
@weewey @jasnell
test: use common.mustCall in test-https-strict
f11d4a1 
PR-URL: #12668
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell
Copy link
Member

jasnell commented Apr 28, 2017

Landed in f11d4a1

@jasnell jasnell closed this Apr 28, 2017
@evanlucas evanlucas mentioned this pull request May 1, 2017
Merged
evanlucas pushed a commit that referenced this pull request May 1, 2017
@weewey @evanlucas
test: use common.mustCall in test-https-strict
105e269 
PR-URL: #12668
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: James M Snell <[email protected]>
evanlucas pushed a commit that referenced this pull request May 2, 2017
@weewey @evanlucas
test: use common.mustCall in test-https-strict
07e9393 
PR-URL: #12668
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: James M Snell <[email protected]>
evanlucas pushed a commit that referenced this pull request May 2, 2017
@weewey @evanlucas
test: use common.mustCall in test-https-strict
3e9e6af 
PR-URL: #12668
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@gibfahn
Copy link
Member

gibfahn commented May 15, 2017

@weewey Would you be willing to backport to v6.x? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

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

@aqrln aqrln aqrln left review comments

@jasnell jasnell jasnell approved these changes

@Trott Trott Trott approved these changes

@hiroppy hiroppy hiroppy approved these changes

Assignees
No one assigned
Labels
https Issues or PRs related to the https subsystem. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@weewey @vsemozhetbyt @Trott @jasnell @gibfahn @hiroppy @aqrln @mscdex @nodejs-github-bot