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

Adjust test-http-content-length and docs to consider countdown #17201

Closed
wants to merge 3 commits into from
Closed

Adjust test-http-content-length and docs to consider countdown #17201

wants to merge 3 commits into from

Conversation

Bamieh
Copy link
Contributor

@Bamieh Bamieh commented Nov 21, 2017

Refactored the test suite test-http-content-length to use countdown and referred to the countdown usage in the docs; as per issue #17169

P.S. These small refactors are really good for new contributors to get more personal with the code base. @jasnell I am willing to contribute more to these refactors if needed.

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)

doc, test

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. test Issues and PRs related to the tests. labels Nov 21, 2017
jasnell
jasnell reviewed Nov 21, 2017
View reviewed changes
doc/guides/writing-tests.md Outdated
@@ -133,11 +133,11 @@ platforms.

### The *common* API

Make use of the helpers from the `common` module as much as possible.
Make use of the helpers from the `common` module as much as possible. Please refer to the [Common API Documentation](https://github.com/nodejs/node/tree/master/test/common) for full details about the helpers.
Copy link
Member

Choose a reason for hiding this comment

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

just a nit: can you line wrap at <= 80 chars :-)

Copy link
Contributor Author

@Bamieh Bamieh Nov 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@jasnell Sure 😄
image

@mscdex mscdex added the http Issues or PRs related to the http subsystem. label Nov 21, 2017
@Trott
Copy link
Member

Trott commented Nov 22, 2017

Should this be two separate commits? One for the doc and one for the test?

@Bamieh
Copy link
Contributor Author

Bamieh commented Nov 22, 2017

@Trott the changes are minor so i figured its okay to put them together. i can split them if thats a requirement.

@maclover7
Copy link
Contributor

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

@addaleax
Copy link
Member

@Bamieh Yeah, splitting them would be ideal, but generally that can also be done by whoever lands this

@Bamieh
Copy link
Contributor Author

Bamieh commented Dec 1, 2017

@addaleax I will definitely do this in future pull requests, but should i split them right now to save time for the person who's gonna land them?

@maclover7
Copy link
Contributor

@Bamieh Would you be able to split the commits? After that I think this should be ready to land :)

@maclover7 maclover7 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 4, 2017
@Bamieh Bamieh force-pushed the refactor-test-http-content-length-and-docs branch from def969d to fef3cf6 Compare December 8, 2017 13:56
@Bamieh
Copy link
Contributor Author

Bamieh commented Dec 8, 2017

@maclover7 I merged the commits into two, one for the fix, another for the documentation:

@maclover7
Copy link
Contributor

@Bamieh it looks like there's still doc edits in the first commit -- are you able to move those to the second commit?

@apapirovski apapirovski removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 9, 2017
@maclover7 maclover7 added the wip Issues and PRs that are still a work in progress. label Dec 12, 2017
@maclover7
Copy link
Contributor

Ping @Bamieh

@Bamieh
Copy link
Contributor Author

Bamieh commented Dec 12, 2017 •
edited
Loading

@maclover7 sorry i got distracted. ready now.

@Bamieh Bamieh force-pushed the refactor-test-http-content-length-and-docs branch from fef3cf6 to f758516 Compare December 12, 2017 20:31
@Bamieh Bamieh force-pushed the refactor-test-http-content-length-and-docs branch from f758516 to 879f939 Compare December 12, 2017 20:44
@maclover7
Copy link
Contributor

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

@maclover7 maclover7 added 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 Dec 14, 2017
maclover7 pushed a commit that referenced this pull request Dec 17, 2017
@Bamieh @maclover7
doc: add countdown module to writing tests guide
cf76176 
PR-URL: #17201
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
maclover7 pushed a commit that referenced this pull request Dec 17, 2017
@Bamieh @maclover7
test: update test-http-content-length to use countdown
97cee72 
PR-URL: #17201
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 8, 2018
@Bamieh @MylesBorins
doc: add countdown module to writing tests guide
92d08d9 
PR-URL: #17201
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 8, 2018
@Bamieh @MylesBorins
test: update test-http-content-length to use countdown
c626f8a 
PR-URL: #17201
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
@Bamieh @MylesBorins
doc: add countdown module to writing tests guide
d1af106 
PR-URL: #17201
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
@Bamieh @MylesBorins
test: update test-http-content-length to use countdown
46f8a9e 
PR-URL: #17201
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 10, 2018
Merged
boingoing pushed a commit to nodejs/node-chakracore that referenced this pull request Jan 18, 2018
@Bamieh @boingoing
doc: add countdown module to writing tests guide
9b4495e 
PR-URL: nodejs/node#17201
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
boingoing pushed a commit to nodejs/node-chakracore that referenced this pull request Jan 18, 2018
@Bamieh @boingoing
test: update test-http-content-length to use countdown
a783c4f 
PR-URL: nodejs/node#17201
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 23, 2018
@Bamieh @MylesBorins
doc: add countdown module to writing tests guide
cd88918 
PR-URL: #17201
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 23, 2018
@Bamieh @MylesBorins
test: update test-http-content-length to use countdown
005e367 
PR-URL: #17201
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 23, 2018
@Bamieh @MylesBorins
doc: add countdown module to writing tests guide
d56b589 
PR-URL: #17201
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 23, 2018
@Bamieh @MylesBorins
test: update test-http-content-length to use countdown
ec1e260 
PR-URL: #17201
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
gibfahn pushed a commit that referenced this pull request Jan 24, 2018
@Bamieh @gibfahn
doc: add countdown module to writing tests guide
2f35920 
PR-URL: #17201
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
gibfahn pushed a commit that referenced this pull request Jan 24, 2018
@Bamieh @gibfahn
test: update test-http-content-length to use countdown
6ed5773 
PR-URL: #17201
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Feb 11, 2018
@Bamieh @MylesBorins
doc: add countdown module to writing tests guide
fa393d5 
PR-URL: #17201
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 11, 2018
@Bamieh @MylesBorins
test: update test-http-content-length to use countdown
97f764c 
PR-URL: #17201
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 12, 2018
@Bamieh @MylesBorins
doc: add countdown module to writing tests guide
505280f 
PR-URL: #17201
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 12, 2018
@Bamieh @MylesBorins
test: update test-http-content-length to use countdown
cb16791 
PR-URL: #17201
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 13, 2018
@Bamieh @MylesBorins
doc: add countdown module to writing tests guide
d0b89a1 
PR-URL: #17201
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 13, 2018
@Bamieh @MylesBorins
test: update test-http-content-length to use countdown
31c5db6 
PR-URL: #17201
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

@maclover7 maclover7 maclover7 approved these changes

@evanlucas evanlucas evanlucas approved these changes

@jasnell jasnell jasnell approved these changes

Assignees

@maclover7 maclover7

Labels
doc Issues and PRs related to the documentations. 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.