Skip to content

test: refactor test-http-destroyed-socket-write2#4970

Closed
santigimeno wants to merge 1 commit intonodejs:masterfrom
santigimeno:fix_one
Closed

test: refactor test-http-destroyed-socket-write2#4970
santigimeno wants to merge 1 commit intonodejs:masterfrom
santigimeno:fix_one

Conversation

@santigimeno
Copy link
Member

@santigimeno santigimeno commented Jan 30, 2016

Remove the limit of requests to be sent (128) as in some conditions it
was reached without the error event being fired, causing the
test to fail.
Remove the initial timeout.
Remove some variables used to check the validity of the test and replace
them with common.mustCall and common.fail calls.

The error I was getting from time to time in OS X was:

=== release test-http-destroyed-socket-write2 ===                    
Path: parallel/test-http-destroyed-socket-write2
assert.js:89
  throw new assert.AssertionError({
  ^
AssertionError: false == true
    at test (/Users/sgimeno/node/node/test/parallel/test-http-destroyed-socket-write2.js:89:5)
    at Immediate.write [as _onImmediate] (/Users/sgimeno/node/node/test/parallel/test-http-destroyed-socket-write2.js:29:7)
    at processImmediate [as _immediateCallback] (timers.js:392:17)
Command: out/Release/node /Users/sgimeno/node/node/test/parallel/test-http-destroyed-socket-write2.js

@JungMinu JungMinu added the test Issues and PRs related to the tests. label Jan 30, 2016
@r-52 r-52 added the http Issues or PRs related to the http subsystem. label Jan 30, 2016
@r-52
Copy link
Contributor

r-52 commented Jan 30, 2016

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

@Trott
Copy link
Member

Trott commented Jan 30, 2016

Looks like there's a minor lint issue to fix:

./node tools/eslint/bin/eslint.js src lib test tools/eslint-rules \
    --rulesdir tools/eslint-rules --quiet

/usr/home/iojs/build/workspace/node-test-linter/test/parallel/test-http-destroyed-socket-write2.js
  44:1  error  Trailing spaces not allowed  no-trailing-spaces

��� 1 problem (1 error, 0 warnings)

@santigimeno
Copy link
Member Author

santigimeno commented Jan 30, 2016

Ouch, sorry about that. Updated.

@Trott
Copy link
Member

Trott commented Jan 30, 2016

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

@Trott
Copy link
Member

Trott commented Jan 30, 2016

Because of its history of flakiness, it might be a good idea to run a stress test for this on AIX and Windows.

It shouldn't stop this from landing, but it might also be worth experimenting after this lands to see if EPIPE and/or ECONNABORTED can be removed. If not, maybe the test can be modified to run until it receives an ECONNRESET since that's what the test was written to check for.

@Trott
Copy link
Member

Trott commented Jan 30, 2016

I don't think we have AIX in CI yet.

I kicked off a stress test on Windows 10. https://ci.nodejs.org/job/node-stress-single-test/395/nodes=win10/

@jasnell
Copy link
Member

jasnell commented Feb 1, 2016

LGTM if CI is green.

@r-52
Copy link
Contributor

r-52 commented Feb 5, 2016

LGTM

@santigimeno santigimeno mentioned this pull request Feb 18, 2016
Closed
@Trott
Copy link
Member

Trott commented Feb 18, 2016

@santigimeno I'd like to land this, but I want to make sure we have a pretty recent CI run so I'd like to run it again. Can you rebase against master and force push?

@santigimeno
test: refactor test-http-destroyed-socket-write2
7809959 
Remove the limit of requests to be sent (128) as in some conditions it
was reached without the `error` event being fired, causing the test to
fail.
Remove the initial timeout.
Remove some variables used to check the validity of the test and replace
them with `common.mustCall` and `common.fail` calls.
@santigimeno
Copy link
Member Author

santigimeno commented Feb 19, 2016

@Trott done

@Trott
Copy link
Member

Trott commented Feb 19, 2016

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

@Trott
Copy link
Member

Trott commented Feb 19, 2016

CI is green. LGTM.

Would be interested to know if this fixes the issue for @thealphanerd too.

Trott pushed a commit to Trott/io.js that referenced this pull request Feb 19, 2016
@santigimeno @Trott
test: refactor test-http-destroyed-socket-write2
b920d45 
Remove the limit of requests to be sent (128) as in some conditions it
was reached without the `error` event being fired, causing the test to
fail.

Remove the initial timeout.

Remove some variables used to check the validity of the test and replace
them with `common.mustCall` and `common.fail` calls.

PR-URL: nodejs#4970
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Roman Klauke <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@Trott
Copy link
Member

Trott commented Feb 19, 2016

Landed in b920d45

@Trott Trott closed this Feb 19, 2016
rvagg pushed a commit that referenced this pull request Feb 21, 2016
@santigimeno @rvagg
test: refactor test-http-destroyed-socket-write2
138ee98 
Remove the limit of requests to be sent (128) as in some conditions it
was reached without the `error` event being fired, causing the test to
fail.

Remove the initial timeout.

Remove some variables used to check the validity of the test and replace
them with `common.mustCall` and `common.fail` calls.

PR-URL: #4970
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Roman Klauke <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 22, 2016
@santigimeno
test: refactor test-http-destroyed-socket-write2
3c7a3be 
Remove the limit of requests to be sent (128) as in some conditions it
was reached without the `error` event being fired, causing the test to
fail.

Remove the initial timeout.

Remove some variables used to check the validity of the test and replace
them with `common.mustCall` and `common.fail` calls.

PR-URL: #4970
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Roman Klauke <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 22, 2016
@santigimeno
test: refactor test-http-destroyed-socket-write2
1647113 
Remove the limit of requests to be sent (128) as in some conditions it
was reached without the `error` event being fired, causing the test to
fail.

Remove the initial timeout.

Remove some variables used to check the validity of the test and replace
them with `common.mustCall` and `common.fail` calls.

PR-URL: #4970
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Roman Klauke <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 2, 2016
@santigimeno
test: refactor test-http-destroyed-socket-write2
f44c7ac 
Remove the limit of requests to be sent (128) as in some conditions it
was reached without the `error` event being fired, causing the test to
fail.

Remove the initial timeout.

Remove some variables used to check the validity of the test and replace
them with `common.mustCall` and `common.fail` calls.

PR-URL: #4970
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Roman Klauke <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
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

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

@santigimeno @r-52 @Trott @jasnell @MylesBorins @JungMinu

Comments