Skip to content

test: add a request-response test for MessageChannel - #21514

Closed
itaysabato wants to merge 1 commit into
nodejs:masterfrom
itaysabato:roundtrip
Closed

test: add a request-response test for MessageChannel#21514
itaysabato wants to merge 1 commit into
nodejs:masterfrom
itaysabato:roundtrip

Conversation

@itaysabato

@itaysabatoitaysabato commented Jun 24, 2018

Copy link
Copy Markdown
Contributor

This commit adds a request-response test for MessageChannel.

Suggested by @benjamingr and @TimothyGuhere

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-botnodejs-github-bot added the test Issues and PRs related to the tests. label Jun 24, 2018
@mscdex

Copy link
Copy Markdown
Contributor

The commit message prefix should be test: instead of worker: if only tests are being added.

Comment threadtest/parallel/test-message-port.js Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could port.onmessage = also be tested?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Used it here (line 31).
Did you have something more elaborate in mind?

@targostargosJun 25, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I commented on the wrong line, sorry. My suggestion is to use it for both port1 and port2 (also do port1.onmessage = ...)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Sure, why not :)

@itaysabatoitaysabato changed the title worker: add a request-response test for MessageChanneltest: add a request-response test for MessageChannelJun 25, 2018

@benjamingrbenjamingr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for your contribution!

@benjamingr

benjamingr commented Jun 25, 2018

Copy link
Copy Markdown
Member

@itaysabato

Copy link
Copy Markdown
ContributorAuthor

Build and test pass locally in both mac and windows. This CI failure may be a glitch...

This commit adds a request-response test for MessageChannel.
PR-URL: nodejs#21514
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@benjamingrbenjamingr added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 27, 2018
jasnell pushed a commit that referenced this pull request Jun 29, 2018
This commit adds a request-response test for MessageChannel.
PR-URL: #21514
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell

Copy link
Copy Markdown
Member

Landed in 71e1602

@jasnelljasnell closed this Jun 29, 2018
@itaysabato
itaysabato deleted the roundtrip branch June 29, 2018 20:23
targos pushed a commit that referenced this pull request Jun 30, 2018
This commit adds a request-response test for MessageChannel.
PR-URL: #21514
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targostargos mentioned this pull request Jul 3, 2018
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.testIssues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants

@itaysabato@mscdex@benjamingr@jasnell@lpinca@TimothyGu@targos@trivikr@nodejs-github-bot