FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: http test improvements by jasnell · Pull Request #14315 · nodejs/node · GitHub

/ node Public

test: http test improvements - #14315

Closed
jasnell wants to merge 1 commit into
nodejs:masterfrom
jasnell:http-test-improvements
ConversationCommits (1)ChecksFiles changed
Closed

test: http test improvements#14315
jasnell wants to merge 1 commit into
nodejs:masterfrom
jasnell:http-test-improvements

Conversation

jasnell commented Jul 17, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

Generalized http test suite improvements...

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

test

nodejs-github-bot added the test Issues and PRs related to the tests. label Jul 17, 2017
jasnell force-pushed the http-test-improvements branch from a03f0e0 to 8f50438 Compare July 17, 2017 00:55
jasnell changed the title Http test improvements test: http test improvements Jul 17, 2017
mscdex added the http Issues or PRs related to the http subsystem. label Jul 17, 2017
benjamingr reviewed Jul 17, 2017
View reviewed changes
Comment thread test/parallel/test-http-blank-header.js Outdated

benjamingr Jul 17, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

heh

benjamingr reviewed Jul 17, 2017
View reviewed changes
Comment thread test/parallel/test-http-client-agent.js Outdated

benjamingr Jul 17, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Why did you add this?

jasnell Jul 17, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Just tightening up the tests to be more strict.

benjamingr reviewed Jul 17, 2017
View reviewed changes
Comment thread test/parallel/test-http-client-check-http-token.js Outdated

benjamingr Jul 17, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

this can be a shorthand. { method, path: '/'}. Also this test structure is a bit strange - and I'm not sure that in this case the code is clearer after the refactoring. Personally I would prefer a for... of but it's not a big deal.

benjamingr reviewed Jul 17, 2017
View reviewed changes
Comment thread test/parallel/test-http-client-check-http-token.js Outdated

benjamingr Jul 17, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It might be good to add a Symbol to the list of expectedFails while we're touching these

benjamingr approved these changes Jul 17, 2017
View reviewed changes

benjamingr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Left a few nits - overall nice work with the countdown.
This is also a pretty big diff.

refack mentioned this pull request Jul 17, 2017
Closed

refack commented Jul 17, 2017

Copy link
Copy Markdown
Contributor

/cc @nodejs/testing @nodejs/http

jasnell force-pushed the http-test-improvements branch from efcd158 to 582ce9b Compare July 17, 2017 23:27

jasnell commented Jul 17, 2017

Copy link
Copy Markdown
Member Author

@benjamingr ... nits addressed!

vsemozhetbyt commented Jul 20, 2017

Copy link
Copy Markdown
Contributor

It seems this should be added before the code example in README.md to pass linting:

<!-- eslint-disable strict, required-modules -->

jasnell mentioned this pull request Jul 24, 2017
Closed
3 tasks
jasnell force-pushed the http-test-improvements branch from 582ce9b to 1fccc7b Compare July 24, 2017 16:31

jasnell commented Jul 24, 2017

Copy link
Copy Markdown
Member Author

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

test: improvements to various http tests
9062d47 
* Add common/countdown utility
* Numerous improvements to http tests
jasnell force-pushed the http-test-improvements branch from 1fccc7b to 9062d47 Compare July 24, 2017 21:14
jasnell added a commit that referenced this pull request Jul 24, 2017
test: improvements to various http tests
b0a8a7c 
* Add common/countdown utility
* Numerous improvements to http tests

PR-URL: #14315
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

jasnell commented Jul 24, 2017

Copy link
Copy Markdown
Member Author

Landed in b0a8a7c, squashed and a lint issue fixed in the process.

jasnell closed this Jul 24, 2017

jasnell commented Jul 24, 2017

Copy link
Copy Markdown
Member Author

@nodejs/codeandlearn ... As a future exercise for code-n-learn participants, there are a non-trivial number of existing tests that can be modified to use the new common/countdown module... for instance, to shutdown a server after a given number of test requests have completed.

gibfahn commented Jul 25, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

Seems like there should be some more people in @nodejs/codeandlearn (I know @Trott and @addaleax have been heavily involved, and I'm sure there are others who are interested).

addaleax commented Jul 27, 2017

Copy link
Copy Markdown
Member

This doesn’t land cleanly on 8.x; if you can, please follow the guide and raise a backport PR.

jasnell added a commit to jasnell/node that referenced this pull request Aug 1, 2017
test: improvements to various http tests
4cddc05 
* Add common/countdown utility
* Numerous improvements to http tests

PR-URL: nodejs#14315
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
jasnell mentioned this pull request Aug 1, 2017
Closed
3 tasks
addaleax pushed a commit that referenced this pull request Aug 1, 2017
test: improvements to various http tests
b1fef05 
* Add common/countdown utility
* Numerous improvements to http tests

Backport-PR-URL: #14583
Backport-Reviewed-By: Anna Henningsen <anna@addaleax.net>

PR-URL: #14315
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
addaleax mentioned this pull request Aug 2, 2017
Merged

MylesBorins commented Aug 16, 2017

Copy link
Copy Markdown
Contributor

This does not land cleanly in LTS. Please feel free to manually backport by following the guide. Please also feel free to replace do-not-land if it is being backported

MylesBorins pushed a commit that referenced this pull request Sep 19, 2017
test: improvements to various http tests
c88f99f 
* Add common/countdown utility
* Numerous improvements to http tests

PR-URL: #14315
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
MylesBorins mentioned this pull request Sep 20, 2017
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

benjamingr benjamingr approved these changes

Assignees

No one assigned

Labels

http Issues or PRs related to the http 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

Back | FazBrowse Home | New Git URL