Skip to content

test: check types for http request and response - #7003

Merged
bnoordhuis merged 1 commit into
nodejs:masterfrom
bnoordhuis:test-http-maps
Jun 29, 2016
Merged

test: check types for http request and response#7003
bnoordhuis merged 1 commit into
nodejs:masterfrom
bnoordhuis:test-http-maps

Conversation

@bnoordhuis

@bnoordhuisbnoordhuis commented May 26, 2016•
edited
Loading

Copy link
Copy Markdown
Member

Add a basic regression test that checks if the map for IncomingMessage
and OutgoingMessage objects is stable over time.

The test is not exhaustive in that it doesn't try to establish whether
the transition path is the same on every request, it just checks that
objects in their final states have the same map.

To be investigated why the first (and only the first) ServerRequest
object ends up with a deprecated map, regardless of the number of
iterations.

Refs: #6294
CI: https://ci.nodejs.org/job/node-test-pull-request/2810/

@bnoordhuisbnoordhuis added http Issues or PRs related to the http subsystem. test Issues and PRs related to the tests. labels May 26, 2016
@bnoordhuisbnoordhuis mentioned this pull request May 26, 2016
Closed
@jasnell

jasnell commented Jun 28, 2016

Copy link
Copy Markdown
Member

New CI: https://ci.nodejs.org/job/node-test-pull-request/3113/
@nodejs/http

jasnell
jasnell reviewed Jun 28, 2016
View reviewed changes
Comment threadtest/parallel/test-http-same-map.js Outdated

@jasnelljasnellJun 28, 2016

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.

I believe we're moving away from using common.PORT... correct @Trott ?

@mscdexmscdexJun 28, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

FWIW I'd say that is the plan.

@TrottTrottJun 28, 2016

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.

I wouldn't stop something from landing over it, but yes, I would say that there is a preference for server.listen(0) over server.listen(common.PORT) where possible.

@mscdexmscdexJun 29, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

IMHO this should be a blocker for landing any PR, otherwise we could easily end up in the same situation again with cascading test failures if we start being lax about it.

@TrottTrottJun 29, 2016

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.

IMHO this should be a blocker for landing any PR, otherwise we could easily end up in the same situation again with cascading test failures if we start being lax about it.

That's reasonable (with the note that there are currently situations that require common.PORT or something like it, but you know that and no doubt mean to exclude those situations).

@jasnell

jasnell commented Jun 28, 2016

Copy link
Copy Markdown
Member

LGTM if CI is green

@bnoordhuis

bnoordhuis commented Jun 29, 2016

Copy link
Copy Markdown
MemberAuthor

Dropped usage of common.PORT and included style changes that make eslint happy again (it doesn't like the natives syntax anymore; it didn't complain before.)

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

@bnoordhuis

bnoordhuis commented Jun 29, 2016

Copy link
Copy Markdown
MemberAuthor

Le sigh, one more fix-up for the Windows buildbots. They didn't like omitting the bind address.

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

@cjihrig

cjihrig commented Jun 29, 2016

Copy link
Copy Markdown
Contributor

CI is green. LGTM

@jasnell

jasnell commented Jun 29, 2016

Copy link
Copy Markdown
Member

LGTM

@bnoordhuis
test: check types for http request and response
dac16d8
Add a basic regression test that checks if the map for IncomingMessage
and OutgoingMessage objects is stable over time.
The test is not exhaustive in that it doesn't try to establish whether
the transition path is the same on every request, it just checks that
objects in their final states have the same map.
To be investigated why the first (and only the first) ServerRequest
object ends up with a deprecated map, regardless of the number of
iterations.
PR-URL: nodejs#7003
Refs: nodejs#6294
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@bnoordhuis
bnoordhuis deleted the test-http-maps branch June 29, 2016 16:15
@bnoordhuis
bnoordhuis merged commit dac16d8 into nodejs:masterJun 29, 2016
Fishrock123 pushed a commit that referenced this pull request Jul 5, 2016
@bnoordhuis@Fishrock123
test: check types for http request and response
658ab3d
Add a basic regression test that checks if the map for IncomingMessage
and OutgoingMessage objects is stable over time.
The test is not exhaustive in that it doesn't try to establish whether
the transition path is the same on every request, it just checks that
objects in their final states have the same map.
To be investigated why the first (and only the first) ServerRequest
object ends up with a deprecated map, regardless of the number of
iterations.
PR-URL: #7003
Refs: #6294
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Fishrock123Fishrock123 mentioned this pull request Jul 5, 2016
Merged
@MylesBorinsMylesBorins mentioned this pull request Jul 11, 2016
Closed
MylesBorins pushed a commit that referenced this pull request Jul 11, 2016
@bnoordhuisMyles Borins
test: check types for http request and response
90f1c3d
Add a basic regression test that checks if the map for IncomingMessage
and OutgoingMessage objects is stable over time.
The test is not exhaustive in that it doesn't try to establish whether
the transition path is the same on every request, it just checks that
objects in their final states have the same map.
To be investigated why the first (and only the first) ServerRequest
object ends up with a deprecated map, regardless of the number of
iterations.
PR-URL: #7003
Refs: #6294
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
@bnoordhuis@MylesBorins
test: check types for http request and response
76e65d9
Add a basic regression test that checks if the map for IncomingMessage
and OutgoingMessage objects is stable over time.
The test is not exhaustive in that it doesn't try to establish whether
the transition path is the same on every request, it just checks that
objects in their final states have the same map.
To be investigated why the first (and only the first) ServerRequest
object ends up with a deprecated map, regardless of the number of
iterations.
PR-URL: #7003
Refs: #6294
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins

MylesBorins commented Jul 12, 2016

Copy link
Copy Markdown
Contributor

I'm backing this out of v4.x as it was causing failures on windows

--> https://ci.nodejs.org/job/node-test-binary-windows/2865/RUN_SUBSET=2,VS_VERSION=vcbt2015,label=win10/tapTestReport/test.tap-115/

not ok 115 parallel/test-http-same-map
# events.js:141
# throw er; // Unhandled 'error' event
# ^
# # Error: connect EADDRNOTAVAIL :::60977
# at Object.exports._errnoException (util.js:907:11)
# at exports._exceptionWithHostPort (util.js:930:20)
# at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1077:14)

Will dig more into this another time

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

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.

6 participants

@bnoordhuis@jasnell@cjihrig@MylesBorins@mscdex@Trott