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.

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

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

Closed
wants to merge 1 commit into from

Conversation

santigimeno
Copy link
Member

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

@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

Ouch, sorry about that. Updated.

@Trott
Copy link
Member

Trott commented Jan 30, 2016

@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

@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?

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

@Trott done

@Trott
Copy link
Member

Trott commented Feb 19, 2016

@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
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
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
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
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
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
Labels
http Issues or PRs related to the http subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants