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.

Sign up for GitHub

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

Jump to bottom

worker: add test for messagePort.onmessage #21510

Closed
wants to merge 1 commit into from
Closed

worker: add test for messagePort.onmessage #21510

wants to merge 1 commit into from

Conversation

targos
Copy link
Member

@targos targos commented Jun 24, 2018

@nodejs/workers

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

@targos targos added test Issues and PRs related to the tests. worker Issues and PRs related to Worker support. labels Jun 24, 2018
@nodejs-github-bot
Copy link
Collaborator

@targos build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/146/pipeline

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jun 24, 2018
benjamingr
benjamingr approved these changes Jun 24, 2018
View reviewed changes
Copy link
Member

@benjamingr benjamingr left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Changes LGTM :) Would be great if you could come up with ideas for tests for worker and make them good-first-contribution

Also, maybe add yourself to @nodejs/workers?

@targos
Copy link
Member Author

targos commented Jun 24, 2018

Would be great if you could come up with ideas for tests for worker and make them good-first-contribution

Writing tests is a good way for me to test if the API meets my expectations, but I'll keep that in mind :)

Also, maybe add yourself to @nodejs/workers?

Thanks for the suggestion, done!

@TimothyGu
Copy link
Member

There should be an equivalent test for MessageChannel and MessagePort themselves in test/parallel/test-message-port.js

@benjamingr
Copy link
Member

@itaysabato do you want to take a stab at those tests @TimothyGu just outlined in #21510 (comment)?

@benjamingr benjamingr mentioned this pull request Jun 24, 2018
Closed
@itaysabato itaysabato mentioned this pull request Jun 24, 2018
Closed
3 tasks
@trivikr trivikr added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 25, 2018
@trivikr
Copy link
Member

trivikr commented Jun 25, 2018

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

@targos
Copy link
Member Author

targos commented Jun 25, 2018

Note to self: change commit prefix to test:

jasnell pushed a commit that referenced this pull request Jun 29, 2018
@targos @jasnell
workers: add test for messagePort.onmessage
f5db04d 
PR-URL: #21510
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Yuta Hiroto <hello@hiroppy.me>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Weijia Wang <starkwang@126.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Jun 29, 2018

Landed in f5db04d

@jasnell jasnell closed this Jun 29, 2018
@targos targos deleted the test-worker-onmessage branch June 29, 2018 18:52
@Trott
Copy link
Member

Trott commented Jun 29, 2018

This landed without the commit message change. Probably not a big deal, only affects the CHANGELOG/Release Notes?

targos added a commit that referenced this pull request Jun 29, 2018
@targos
workers: add test for messagePort.onmessage
41c4b2c 
PR-URL: #21510
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Yuta Hiroto <hello@hiroppy.me>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Weijia Wang <starkwang@126.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Jun 29, 2018

Ah... just saw the note to self from @targos ... yeah, I don't think it's super critical

@targos targos mentioned this pull request Jul 3, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@benjamingr benjamingr benjamingr approved these changes

@lpinca lpinca lpinca approved these changes

@TimothyGu TimothyGu TimothyGu approved these changes

@cjihrig cjihrig cjihrig approved these changes

@hiroppy hiroppy hiroppy approved these changes

@starkwang starkwang starkwang approved these changes

@trivikr trivikr trivikr approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. test Issues and PRs related to the tests. worker Issues and PRs related to Worker support.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.