Skip to content

test: http test improvements - #14315

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

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

Conversation

@jasnell

@jasnelljasnell 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-botnodejs-github-bot added the test Issues and PRs related to the tests. label Jul 17, 2017
@jasnell
jasnellforce-pushed the http-test-improvements branch from a03f0e0 to 8f50438CompareJuly 17, 2017 00:55
@jasnelljasnell changed the title Http test improvementstest: http test improvementsJul 17, 2017
@mscdexmscdex added the http Issues or PRs related to the http subsystem. label Jul 17, 2017
benjamingr
benjamingr reviewed Jul 17, 2017
View reviewed changes
Comment threadtest/parallel/test-http-blank-header.js Outdated

@benjamingrbenjamingrJul 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.

heh

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

@benjamingrbenjamingrJul 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.

Why did you add this?

@jasnelljasnellJul 17, 2017

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Just tightening up the tests to be more strict.

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

@benjamingrbenjamingrJul 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.

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
benjamingr reviewed Jul 17, 2017
View reviewed changes
Comment threadtest/parallel/test-http-client-check-http-token.js Outdated

@benjamingrbenjamingrJul 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.

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

benjamingr
benjamingr approved these changes Jul 17, 2017
View reviewed changes

@benjamingrbenjamingr 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.

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

@refackrefack mentioned this pull request Jul 17, 2017
Closed
@refack

refack commented Jul 17, 2017

Copy link
Copy Markdown
Contributor

/cc @nodejs/testing @nodejs/http

@jasnell
jasnellforce-pushed the http-test-improvements branch from efcd158 to 582ce9bCompareJuly 17, 2017 23:27
@jasnell

jasnell commented Jul 17, 2017

Copy link
Copy Markdown
MemberAuthor

@benjamingr ... nits addressed!

@vsemozhetbyt

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 -->

@jasnelljasnell mentioned this pull request Jul 24, 2017
Closed
3 tasks
@jasnell
jasnellforce-pushed the http-test-improvements branch from 582ce9b to 1fccc7bCompareJuly 24, 2017 16:31
@jasnell

jasnell commented Jul 24, 2017

Copy link
Copy Markdown
MemberAuthor

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

@jasnell
test: improvements to various http tests
9062d47
* Add common/countdown utility
* Numerous improvements to http tests
@jasnell
jasnellforce-pushed the http-test-improvements branch from 1fccc7b to 9062d47CompareJuly 24, 2017 21:14
jasnell added a commit that referenced this pull request Jul 24, 2017
@jasnell
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

jasnell commented Jul 24, 2017

Copy link
Copy Markdown
MemberAuthor

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

@jasnelljasnell closed this Jul 24, 2017
@jasnell

jasnell commented Jul 24, 2017

Copy link
Copy Markdown
MemberAuthor

@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

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

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
@jasnell
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>
@jasnelljasnell mentioned this pull request Aug 1, 2017
Closed
3 tasks
addaleax pushed a commit that referenced this pull request Aug 1, 2017
@jasnell@addaleax
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>
@addaleaxaddaleax mentioned this pull request Aug 2, 2017
Merged
@MylesBorins

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
@jasnell@MylesBorins
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>
@MylesBorinsMylesBorins mentioned this pull request Sep 20, 2017
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benjamingrbenjamingrbenjamingr approved these changes

Assignees

No one assigned

Labels

httpIssues or PRs related to the http subsystem.testIssues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@jasnell@refack@vsemozhetbyt@gibfahn@addaleax@MylesBorins@benjamingr@mscdex@nodejs-github-bot