Skip to content

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

Closed
piscisaureus wants to merge 1 commit intonodejs:v1.xfrom
piscisaureus:revert-4874
Closed

Revert "http: send Content-Length when possible"#1137
piscisaureus wants to merge 1 commit intonodejs:v1.xfrom
piscisaureus:revert-4874

Conversation

@piscisaureus
Copy link
Contributor

@piscisaureus piscisaureus commented Mar 13, 2015

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

cc @mscdex @bnoordhuis @tellnes

@bnoordhuis
Copy link
Member

bnoordhuis commented Mar 13, 2015

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

Fishrock123 commented Mar 13, 2015

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

Fishrock123 commented Mar 13, 2015

See also: #1005 (comment)

@bnoordhuis
Copy link
Member

bnoordhuis commented Mar 13, 2015

/cc @tellnes

@piscisaureus
Copy link
Contributor Author

piscisaureus commented Mar 13, 2015

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

Fishrock123 commented Mar 13, 2015

@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

Fishrock123 commented Mar 13, 2015

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

piscisaureus commented Mar 13, 2015

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 <mscdex@mscdex.net>
Reviewed-By: Bert Belder <bertbelder@gmail.com>
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 <mscdex@mscdex.net>
Reviewed-By: Bert Belder <bertbelder@gmail.com>
@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