-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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 Windows native test suites #53173
test: fix Windows native test suites #53173
Conversation
Fast-track has been requested by @StefanStojanovic. Please 👍 to approve. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM if an issue is open so we don't forget to fix it.
I stumbled upon this issue while working on nodejs/build#3739, but I'm not sure this should close it (I thought changes I made yesterday would be enough, but this popped up), so I'll monitor the CI for few days after all is back to normal and close the issue manually. At least that's the plan. |
I mean an issue about fixing and re-enabling the test on Windows |
Hmm, it seems strange why the test case is failing on Windows, AFAICT there isn't anything Windows specific about the test case. But let's skip it on Windows for now to unbreak CI first. |
2 PRs that landed independently caused this issue which makes every native suites run in CI fail on Windows. This is just a quick patch to unblock the CI. Refs: nodejs#52905 Refs: nodejs#52646
169b0f8
to
6735452
Compare
Landed in 8e9686d |
2 PRs that landed independently caused this issue which makes every native suites run in CI fail on Windows. This is just a quick patch to unblock the CI. Refs: #52905 Refs: #52646 PR-URL: #53173 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>
2 PRs that landed independently caused this issue which makes every native suites run in CI fail on Windows. This is just a quick patch to unblock the CI. Refs: nodejs#52905 Refs: nodejs#52646 PR-URL: nodejs#53173 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>
2 PRs that landed independently caused this issue which makes every native suites run in CI fail on Windows. This is just a quick patch to unblock the CI. Refs: nodejs#52905 Refs: nodejs#52646 PR-URL: nodejs#53173 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>
2 PRs that landed independently caused this issue which makes every native suites run in CI fail on Windows. This is just a quick patch to unblock the CI. Refs: #52905 Refs: #52646 PR-URL: #53173 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>
2 PRs that landed independently caused this issue which makes every native suites run in CI fail on Windows. This is just a quick patch to unblock the CI. Refs: #52905 Refs: #52646 PR-URL: #53173 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>
4 days ago, 2 PRs that landed independently around the same time caused this issue causing every native test suite run in CI to fail on Windows. This is just a quick patch to unblock the CI.
One PR added running
test\embedding\test-embedding.js
on Windows, and the other one added a new test case in the same file. As it turned out, that case is failing on Windows. This is a simple fix to reenable CI as without it no PRs can be merged and a follow-on PR actually fixing this issue is welcome.I've already run my branch against the CI to make sure native suites are fixed and they are: https://ci.nodejs.org/job/node-test-commit-windows-fanned/63233/
Tagging @RafaelGSS and @vmoroz as they opened the 2 PRs in question
Refs: #52905
Refs: #52646