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

Revert "http: send Content-Length when possible" #1137

Closed
wants to merge 1 commit into from
Closed

Revert "http: send Content-Length when possible" #1137

wants to merge 1 commit into from

Conversation

piscisaureus
Copy link
Contributor

The "test/parallel/test-http-content-length.js" has never passed on windows.

cc @mscdex @bnoordhuis @tellnes

@bnoordhuis
Copy link
Member

Why doesn't it pass? I don't see anything in the diff that is platform-specific and the test passes on OS X and Linux.

@Fishrock123
Copy link
Contributor

CI error:

=== release test-http-content-length ===
Path: parallel/test-http-content-length
events.js:141
      throw er; // Unhandled 'error' event
            ^
Error: read ECONNRESET
    at exports._errnoException (util.js:734:11)
    at TCP.onread (net.js:538:26)
Command: c:\workspace\iojs+any-pr+multi\nodes\iojs-win2012r2\Release\iojs.exe c:\workspace\iojs+any-pr+multi\nodes\iojs-win2012r2\test\parallel\test-http-content-length.js

@Fishrock123 Fishrock123 mentioned this pull request Mar 13, 2015
Closed
@piscisaureus piscisaureus mentioned this pull request Mar 13, 2015
Closed
@Fishrock123
Copy link
Contributor

See also: #1005 (comment)

@bnoordhuis
Copy link
Member

/cc @tellnes

@piscisaureus
Copy link
Contributor Author

If I change "2" to "3" at
4874182#diff-cc887ceab87442f70205eb5fe715cad5R21, the test passes.
But I can't tell if the test still correct when I do that. @tellnes?

@Fishrock123
Copy link
Contributor

@piscisaureus Looks like it:

4874182#diff-cc887ceab87442f70205eb5fe715cad5R45

First run is 1 at end, 2nd is 2 at end, and it bails.

The increment should be in the conditional or after, the way it is written. Nah it should just expect 3.

@Fishrock123
Copy link
Contributor

CI with the should-be test fix https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/296/

@Fishrock123 Fishrock123 mentioned this pull request Mar 13, 2015
Merged
@piscisaureus
Copy link
Contributor Author

Closing, as reverting won't be necessary: #1137

Fishrock123 added a commit to Fishrock123/node that referenced this pull request Mar 13, 2015
@Fishrock123
test: fix test-http-content-length
906ad71 
Previously the test did not allow the last request to complete.

Fixes: nodejs#1137
Reviewed-By: Brian White <[email protected]>
Reviewed-By: Bert Belder <[email protected]>
Fishrock123 added a commit that referenced this pull request Mar 13, 2015
@Fishrock123
test: fix test-http-content-length
53e200a 
Previously the test did not allow the last request to complete.

Fixes: #1137
PR-URL: #1145
Reviewed-By: Brian White <[email protected]>
Reviewed-By: Bert Belder <[email protected]>
@tellnes
Copy link
Contributor

tellnes commented Mar 16, 2015

Oh, looks like I got the number wrong when I squashed my commits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@piscisaureus @bnoordhuis @Fishrock123 @tellnes