FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: fix test-worker-broadcastchannel-wpt by Trott · Pull Request #36353 · nodejs/node · GitHub

/ node Public

test: fix test-worker-broadcastchannel-wpt - #36353

Merged
Trott merged 1 commit into
nodejs:masterfrom
Trott:fix-broadcast-test
Dec 2, 2020
ConversationCommits (1)ChecksFiles changed
Merged

test: fix test-worker-broadcastchannel-wpt#36353
Trott merged 1 commit into
nodejs:masterfrom
Trott:fix-broadcast-test

Conversation

Trott commented Dec 2, 2020

Copy link
Copy Markdown
Member

Optimistic possible fix for a broken test on Windows.

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

nodejs-github-bot added the test Issues and PRs related to the tests. label Dec 2, 2020

This comment has been minimized.

Sign in to view
Trott mentioned this pull request Dec 2, 2020
Closed
4 tasks
jasnell approved these changes Dec 2, 2020
View reviewed changes

jasnell 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Good catch. I got distracted and forgot that CI hadn't been run after I had rebased and updated this. +1 for fast tracking assuming it goes through. I'd also be +1 on temporarily marking this flaky or commenting out this part of the test until the ordering can be revisited

Trott commented Dec 2, 2020

Copy link
Copy Markdown
Member Author

+1 for fast tracking assuming it goes through. I'd also be +1 on temporarily marking this flaky or commenting out this part of the test until the ordering can be revisited

Yeah, if this doesn't pass, I'll modify it to skip on Windows and leave a TODO comment. Since the feature is experimental, that should be fine.

Trott force-pushed the fix-broadcast-test branch from d3cbd20 to 4e147fc Compare December 2, 2020 15:02

Trott commented Dec 2, 2020

Copy link
Copy Markdown
Member Author

Well, that fixes the test case that fails currently, but something else fails later on. Put in code to skip the thing later on and trying CI again. In the meantime, taking out of draft mode and requesting fast-track. We already have one fast-track approval from @jasnell. Another one, anyone?

This comment has been minimized.

Sign in to view
Trott marked this pull request as ready for review December 2, 2020 15:04
Trott added the fast-track PRs that do not need to wait for 48 hours to land. label Dec 2, 2020

nodejs-github-bot commented Dec 2, 2020

Copy link
Copy Markdown
Collaborator

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

test: fix test-worker-broadcastchannel-wpt
c91e608 
Optimistic possible fix for a broken test on Windows.

PR-URL: nodejs#36353
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Trott force-pushed the fix-broadcast-test branch from 4e147fc to c91e608 Compare December 2, 2020 17:12

Trott commented Dec 2, 2020

Copy link
Copy Markdown
Member Author

Landed in c91e608

Trott merged commit c91e608 into nodejs:master Dec 2, 2020
Trott deleted the fix-broadcast-test branch December 2, 2020 17:12
danielleadams pushed a commit that referenced this pull request Dec 7, 2020
test: fix test-worker-broadcastchannel-wpt
faca2b8 
Optimistic possible fix for a broken test on Windows.

PR-URL: #36353
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
danielleadams mentioned this pull request Dec 7, 2020
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
benjamingr benjamingr approved these changes

Assignees

No one assigned

Labels

fast-track PRs that do not need to wait for 48 hours to land. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL