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

test: refactor async-hooks/test-httparser tests #14818

Closed
wants to merge 2 commits into from
Closed

test: refactor async-hooks/test-httparser tests #14818

wants to merge 2 commits into from

Conversation

Runite618
Copy link
Contributor

My first contribution to open source.

By advice from Rich Trott I have modified both the test-httparser.request.js changing the string literal on line 22 and then also the file test-http.response.js removing unused arguments from the method. Pull request contains two commits.

Screenshot is attached of the tests that have all been passed.

2017-08-14 3

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

@Runite618
test: refactor async-hooks/test-httparser tests
a8655e7 
CRLF variable was defined but only used on line 22 so the variable
was deleted and placed inside line 22 as a string literal. This
was in file test-httpparser.request.js

On line 46 there's a function declared that takes 3 arguments but
none of them are ever used so removed. This is in file
test-httpparser.response.js
@Runite618
test: refactor asyncs-hooks/test-httparser tests
a9c22d9 
Modification of the file test-httpparser.request.js to have the
string literals \r\n to be included in the string literal
of line 22 and not concatenated to remove errors and hopefully
pass the tests.
@nodejs-github-bot nodejs-github-bot added async_hooks Issues and PRs related to the async hooks subsystem. test Issues and PRs related to the tests. labels Aug 14, 2017
@vsemozhetbyt
Copy link
Contributor

Thank you, @Runite618!
CI: https://ci.nodejs.org/job/node-test-pull-request/9646/

@aqrln aqrln changed the title First contribution test: refactor async-hooks/test-httparser tests Aug 14, 2017
@aqrln
Copy link
Contributor

aqrln commented Aug 14, 2017

Thanks for your contribution, @Runite618! 🎉
FWIW, the checkbox is more of a reminder to run tests before opening a PR, you don't need to include a screenshot of successfully passed tests to prove it :D

@Runite618
Copy link
Contributor Author

Oh sorry it said include tests so I thought it meant add a screenshot lol.

@aqrln
Copy link
Contributor

aqrln commented Aug 14, 2017

@Runite618 no, the "tests and/or benchmarks are included" checkbox only applies to those PRs that must include new tests or benchmarks. Speaking of tests, these would be PRs that introduce new features, fix bugs or merely introduce new tests to increase coverage. If you are unsure about something, or you have a question, #node-dev on IRC is a great place to ask, so don't hesitate doing so. Asking any relevant questions in this thread is perfectly okay too :)

Trott
Trott approved these changes Aug 14, 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. Only CI failures is a known flaky (about to be fixed, or at least worked around). So CI is good.

@refack
Copy link
Contributor

refack commented Aug 14, 2017

@Runite618 welcome, and thanks.

@tniessen
Copy link
Member

Landed in d4374ad, thank you! 🎉

@tniessen tniessen closed this Aug 16, 2017
tniessen pushed a commit that referenced this pull request Aug 16, 2017
@Runite618 @tniessen
test: refactor async-hooks/test-httparser tests
d4374ad 
CRLF variable was defined but only used on line 22 so the variable
was deleted and placed inside line 22 as a string literal. This
was in file test-httpparser.request.js

On line 46 there's a function declared that takes 3 arguments but
none of them are ever used so removed. This is in file
test-httpparser.response.js

PR-URL: #14818
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MSLaguana pushed a commit to nodejs/node-chakracore that referenced this pull request Aug 21, 2017
@Runite618 @MSLaguana
test: refactor async-hooks/test-httparser tests
221e72d 
CRLF variable was defined but only used on line 22 so the variable
was deleted and placed inside line 22 as a string literal. This
was in file test-httpparser.request.js

On line 46 there's a function declared that takes 3 arguments but
none of them are ever used so removed. This is in file
test-httpparser.response.js

PR-URL: nodejs/node#14818
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 10, 2017
@Runite618 @MylesBorins
test: refactor async-hooks/test-httparser tests
8004625 
CRLF variable was defined but only used on line 22 so the variable
was deleted and placed inside line 22 as a string literal. This
was in file test-httpparser.request.js

On line 46 there's a function declared that takes 3 arguments but
none of them are ever used so removed. This is in file
test-httpparser.response.js

PR-URL: #14818
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Sep 10, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Sep 12, 2017
@Runite618 @MylesBorins
test: refactor async-hooks/test-httparser tests
4dd095c 
CRLF variable was defined but only used on line 22 so the variable
was deleted and placed inside line 22 as a string literal. This
was in file test-httpparser.request.js

On line 46 there's a function declared that takes 3 arguments but
none of them are ever used so removed. This is in file
test-httpparser.response.js

PR-URL: #14818
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vsemozhetbyt vsemozhetbyt vsemozhetbyt approved these changes

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

@aqrln aqrln aqrln approved these changes

@tniessen tniessen tniessen approved these changes

@lpinca lpinca lpinca approved these changes

@Trott Trott Trott approved these changes

@jasnell jasnell jasnell approved these changes

@refack refack refack approved these changes

Assignees
No one assigned
Labels
async_hooks Issues and PRs related to the async hooks 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.