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

parallel/test-http-connect broken on master #18940

Closed
Fishrock123 opened this issue Feb 22, 2018 · 7 comments
Closed

parallel/test-http-connect broken on master #18940

Fishrock123 opened this issue Feb 22, 2018 · 7 comments
Labels
confirmed-bug Issues with confirmed bugs. http Issues or PRs related to the http subsystem. test Issues and PRs related to the tests.

Comments

@Fishrock123
Copy link
Contributor

  • Version: master (2b7f920)
  • Platform: all
  • Subsystem: http, test
=== release test-http-connect ===
Path: parallel/test-http-connect
assert.js:74
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: 2 strictEqual 1
    at Server.server.on.common.mustCall (/Users/Jeremiah/Documents/node/test/parallel/test-http-connect.js:37:10)
    at Server.<anonymous> (/Users/Jeremiah/Documents/node/test/common/index.js:476:15)
    at Server.emit (events.js:129:13)
    at onParserExecuteCommon (_http_server.js:536:14)
    at onParserExecute (_http_server.js:483:3)

cc @BridgeAR who says he's on it.

@Fishrock123 Fishrock123 added confirmed-bug Issues with confirmed bugs. http Issues or PRs related to the http subsystem. test Issues and PRs related to the tests. labels Feb 22, 2018
@BridgeAR
Copy link
Member

It seems like 51be03c broke the test in combination with other PRs that landed earlier.

@lpinca can you please take a look?

@BridgeAR
Copy link
Member

The CI was green for the PR: https://ci.nodejs.org/job/node-test-pull-request/13276/

@BridgeAR
Copy link
Member

To fix it I am just going to open a PR to change it to 2 right now as I can not look into it any further for the next couple hours.

@Fishrock123
Copy link
Contributor Author

Can we revert the commits in question?

@addaleax
Copy link
Member

@BridgeAR Yeah, it’s just 51be03c interacting with 281d00e.

I don’t think we need to do anything besides changing the number to 2 (and maybe recommending make test before pushing to master, if that’s not already in the collaborator guide).

@BridgeAR
Copy link
Member

@addaleax I often thought about adding make test to my landing script but the issue is that it increases the overhead immensely. The tests take to long to always run them :/ (and it is not mandatory right now as far as I know).

@addaleax
Copy link
Member

@BridgeAR Yeah, I feel the pain of make test taking far too long … I’m not sure what to do about that. :/

MayaLekova pushed a commit to MayaLekova/node that referenced this issue May 8, 2018
Fixes: nodejs#18940
PR-URL: nodejs#18941
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. http Issues or PRs related to the http subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

No branches or pull requests

3 participants