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-http-client-readable #9344

Closed
wants to merge 1 commit into from
Closed

Conversation

Trott
Copy link
Member

@Trott Trott commented Oct 28, 2016

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

test http

Description of change
  • var -> const
  • remove function names where V8 inference is as good or better
  • add function names where there is no V8 inference
  • assert.equal -> strictEqual
  • move assertion from exit handler to response end handler

@Trott Trott added http Issues or PRs related to the http subsystem. test Issues and PRs related to the tests. labels Oct 28, 2016
@Trott
Copy link
Member Author

Trott commented Oct 28, 2016

* var -> const
* remove function names where V8 inference is as good or better
* add function names where there is no V8 inference
* assert.equal -> strictEqual
* move assertion from exit handler to response end handler
@Trott
Copy link
Member Author

Trott commented Oct 29, 2016

Failures are known flaky tests. But one of them was just fixed, so I'm going to rebase and run CI again...

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

@Trott
Copy link
Member Author

Trott commented Nov 1, 2016

@lpinca
Copy link
Member

lpinca commented Nov 1, 2016

add function names where there is no V8 inference

Unrelated questions: is this important for tests? Should this be done for all tests?

Trott added a commit to Trott/io.js that referenced this pull request Nov 1, 2016
* var -> const
* remove function names where V8 inference is as good or better
* add function names where there is no V8 inference
* assert.equal -> strictEqual
* move assertion from exit handler to response end handler

PR-URL: nodejs#9344
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@Trott
Copy link
Member Author

Trott commented Nov 1, 2016

Landed in 7d18a4d

@Trott Trott closed this Nov 1, 2016
@Trott
Copy link
Member Author

Trott commented Nov 1, 2016

add function names where there is no V8 inference

Unrelated questions: is this important for tests? Should this be done for all tests?

I would say there's no reason to make any extra effort to do it for all tests, but that if you're already making an improvement in a test and see something like that, changing it as well is fine. This would be similar to how we deal with var -> const in tests.

evanlucas pushed a commit that referenced this pull request Nov 3, 2016
* var -> const
* remove function names where V8 inference is as good or better
* add function names where there is no V8 inference
* assert.equal -> strictEqual
* move assertion from exit handler to response end handler

PR-URL: #9344
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 22, 2016
* var -> const
* remove function names where V8 inference is as good or better
* add function names where there is no V8 inference
* assert.equal -> strictEqual
* move assertion from exit handler to response end handler

PR-URL: #9344
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Nov 22, 2016
@Trott Trott deleted the thcr branch January 13, 2022 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants