Skip to content

test: don't fail http2 abort test if 'data' is called multiple times#21925

Closed
rubys wants to merge 1 commit intonodejs:masterfrom
rubys:test-http2-abort-data-twice
Closed

test: don't fail http2 abort test if 'data' is called multiple times#21925
rubys wants to merge 1 commit intonodejs:masterfrom
rubys:test-http2-abort-data-twice

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

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

@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

mcollina commented Jul 23, 2018

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/

mcollina
mcollina approved these changes Jul 25, 2018
View reviewed changes
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

mcollina commented Jul 25, 2018

Landed in 838001d

@mcollina mcollina closed this Jul 25, 2018
mcollina pushed a commit that referenced this pull request Jul 25, 2018
@rubys @mcollina
test: don't fail http2 abort test if 'data' is called multiple times
838001d 
PR-URL: #21925
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos pushed a commit that referenced this pull request Jul 26, 2018
@rubys @targos
test: don't fail http2 abort test if 'data' is called multiple times
793a5bb 
PR-URL: #21925
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@targos targos mentioned this pull request Jul 31, 2018
Merged
This was referenced Aug 1, 2018
Closed
Closed
Closed
Closed
Closed
Closed
@kjin kjin mentioned this pull request Sep 19, 2018
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes

@jasnell jasnell jasnell approved these changes

@Trott Trott Trott approved these changes

@benjamingr benjamingr benjamingr approved these changes

@lpinca lpinca lpinca approved these changes

@cjihrig cjihrig cjihrig approved these changes

@trivikr trivikr trivikr approved these changes

+1 more reviewer

@apapirovski apapirovski apapirovski approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@rubys @Trott @mcollina @apapirovski @jasnell @benjamingr @lpinca @cjihrig @trivikr