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: add an exception test to http-write-head #11034

Closed
wants to merge 1 commit into from
Closed

test: add an exception test to http-write-head #11034

wants to merge 1 commit into from

Conversation

hiroppy
Copy link
Member

@hiroppy hiroppy commented Jan 27, 2017

Add an exception test.
Make use of Arrow function.

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 nodejs-github-bot added the test Issues and PRs related to the tests. label Jan 27, 2017
cjihrig
cjihrig approved these changes Jan 27, 2017
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM, but this could use some common.mustCall() in several places while you're in here.

@hiroppy hiroppy force-pushed the feature/test-http-write-head branch from e3f0c7d to 485dac5 Compare January 27, 2017 16:21
@mscdex mscdex added the http Issues or PRs related to the http subsystem. label Jan 27, 2017
@hiroppy
Copy link
Member Author

hiroppy commented Jan 28, 2017

Changed to use common.mustCall().

@cjihrig
Copy link
Contributor

cjihrig commented Jan 29, 2017

You could also add common.mustCall() on lines 43, 46, and 47.

@hiroppy
test: add an exception test to http-write-head
2ec24e0 
Add an exception test.
Add `common.mustCall()`.
Make use of Arrow function.

a
@hiroppy hiroppy force-pushed the feature/test-http-write-head branch from 485dac5 to 2ec24e0 Compare January 29, 2017 03:19
@hiroppy
Copy link
Member Author

hiroppy commented Jan 29, 2017

Added common.mustCall() on lines 43, 46, and 47.

jasnell pushed a commit that referenced this pull request Jan 30, 2017
@hiroppy @jasnell
test: add an exception test to http-write-head
49e5f61 
* Add an exception test.
* Add `common.mustCall()`.
* Make use of Arrow function.

PR-URL: #11034
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@jasnell
Copy link
Member

jasnell commented Jan 30, 2017

Landed in 49e5f61

@jasnell jasnell closed this Jan 30, 2017
evanlucas pushed a commit that referenced this pull request Jan 31, 2017
@hiroppy @evanlucas
test: add an exception test to http-write-head
e367b74 
* Add an exception test.
* Add `common.mustCall()`.
* Make use of Arrow function.

PR-URL: #11034
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@italoacasas italoacasas mentioned this pull request Jan 31, 2017
Merged
jasnell pushed a commit that referenced this pull request Mar 8, 2017
@hiroppy @jasnell
test: add an exception test to http-write-head
6cc0db8 
* Add an exception test.
* Add `common.mustCall()`.
* Make use of Arrow function.

PR-URL: #11034
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@jasnell
Copy link
Member

jasnell commented Mar 8, 2017

Test fails in v4, will need a backport PR if it is to land there. Landed in v6

MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
@hiroppy @MylesBorins
test: add an exception test to http-write-head
22d4ed2 
* Add an exception test.
* Add `common.mustCall()`.
* Make use of Arrow function.

PR-URL: #11034
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig cjihrig approved these changes

@lpinca lpinca lpinca approved these changes

@jasnell jasnell jasnell 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.
6 participants
@hiroppy @cjihrig @jasnell @lpinca @mscdex @nodejs-github-bot