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: fix test-worker-broadcastchannel-wpt #36353

Merged
merged 1 commit into from
Dec 2, 2020

Conversation

Trott
Copy link
Member

@Trott Trott commented Dec 2, 2020

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 nodejs-github-bot added the test Issues and PRs related to the tests. label Dec 2, 2020
@nodejs-github-bot

This comment has been minimized.

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member Author

Trott commented Dec 2, 2020

+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 Trott force-pushed the fix-broadcast-test branch from d3cbd20 to 4e147fc Compare December 2, 2020 15:02
@Trott
Copy link
Member Author

Trott commented Dec 2, 2020

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?

@nodejs-github-bot

This comment has been minimized.

@Trott Trott marked this pull request as ready for review December 2, 2020 15:04
@Trott Trott added the fast-track PRs that do not need to wait for 48 hours to land. label Dec 2, 2020
@nodejs-github-bot
Copy link
Collaborator

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 Trott force-pushed the fix-broadcast-test branch from 4e147fc to c91e608 Compare December 2, 2020 17:12
@Trott
Copy link
Member Author

Trott commented Dec 2, 2020

Landed in c91e608

@Trott Trott merged commit c91e608 into nodejs:master Dec 2, 2020
@Trott Trott deleted the fix-broadcast-test branch December 2, 2020 17:12
danielleadams pushed a commit that referenced this pull request Dec 7, 2020
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 danielleadams mentioned this pull request Dec 7, 2020
cjihrig pushed a commit to cjihrig/node that referenced this pull request Dec 8, 2020
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

5 participants