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: don't fail http2 abort test if 'data' is called multiple times #21925

Closed
wants to merge 1 commit into from

Conversation

rubys
Copy link
Member

@rubys rubys commented Jul 21, 2018

I'm seeing a regression in make test on master on macOS High Sierra Version 10.13.5. The error I'm seeing is:

=== release test-http2-respond-with-file-connection-abort ===                  
Path: parallel/test-http2-respond-with-file-connection-abort
Mismatched <anonymous> function calls. Expected exactly 1, actual 2.
    at Object.exports.mustCall (/Users/rubys/git/node/test/common/index.js:427:10)
    at Http2Server.server.listen.common.mustCall (/Users/rubys/git/node/test/parallel/test-http2-respond-with-file-connection-abort.js:25:25)
    at Http2Server.<anonymous> (/Users/rubys/git/node/test/common/index.js:467:15)
    at Object.onceWrapper (events.js:273:13)
    at Http2Server.emit (events.js:182:13)
    at emitListeningNT (net.js:1370:10)
    at process._tickCallback (internal/process/next_tick.js:63:19)
    at Function.Module.runMain (internal/modules/cjs/loader.js:745:11)
    at startup (internal/bootstrap/node.js:266:19)
(node:20069) ExperimentalWarning: The http2 module is an experimental API.
Command: out/Release/node /Users/rubys/git/node/test/parallel/test-http2-respond-with-file-connection-abort.js
[02:54|% 100|+ 2354|-   1]: Done   

It seems that the test case is assuming that a data event will be emitted exactly once, but I see no reason why that should be guaranteed to be the case.

With this fix, the new results are:

[02:55|% 100|+ 2355|-   0]: Done                                               

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@Trott
Copy link
Member

Trott commented Jul 21, 2018

Another approach would be to use .once() instead of .on(). That way destroy() and close() don't get called more than once. But I'm 👍 on the approach taken here too.

@Trott
Copy link
Member

Trott commented Jul 21, 2018

@nodejs/http2

@Trott
Copy link
Member

Trott commented Jul 22, 2018

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 22, 2018
@mcollina
Copy link
Member

Flaky test on OS X, I've hit resume build.

@Trott
Copy link
Member

Trott commented Jul 23, 2018

Resumed build is https://ci.nodejs.org/job/node-test-pull-request/15976/ (and green).

@Trott Trott force-pushed the test-http2-abort-data-twice branch from 165e78e to ef32a01 Compare July 23, 2018 18:47
@Trott
Copy link
Member

Trott commented Jul 23, 2018

Had to resolve a merge conflict. Restarting CI again: https://ci.nodejs.org/job/node-test-pull-request/15979/

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mcollina
Copy link
Member

Landed in 838001d

@mcollina mcollina closed this Jul 25, 2018
mcollina pushed a commit that referenced this pull request Jul 25, 2018
PR-URL: #21925
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
targos pushed a commit that referenced this pull request Jul 26, 2018
PR-URL: #21925
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@targos targos mentioned this pull request Jul 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants