Skip to content

test: split wasi poll test for windows - #52538

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
JaneaSystems:huseyin-11972-flaky-test-wasi-poll
Apr 17, 2024
Merged

test: split wasi poll test for windows#52538
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
JaneaSystems:huseyin-11972-flaky-test-wasi-poll

Conversation

@huseyinacacak-janea

@huseyinacacak-janeahuseyinacacak-janea commented Apr 15, 2024

Copy link
Copy Markdown
Contributor

After upgrading to Visual Studio v17.9, the test-wasi-poll started failing. Upon investigation, I discovered a bug in V8 and opened a CL in V8: https://chromium-review.googlesource.com/c/v8/v8/+/5438113

Notably, some parts of the poll.wasm (created using poll.c) are unsupported on Windows, as indicated here:

node/test/wasi/c/poll.c

Lines 34 to 36 in 9efc84a

// The rest of the test is unsupported on Windows.
if (is_win)
return0;

However, the bug in V8 enables these unsupported parts of the test to execute and makes the test fail.
In this PR, I've created a new poll_win.wasm which will only be used on Windows until this bug is fixed upstream and ported to Node.

Fixes: #51822

@nodejs-github-bot

nodejs-github-bot commented Apr 15, 2024

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/wasi

@nodejs-github-botnodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Apr 15, 2024
lpinca
lpinca approved these changes Apr 15, 2024
View reviewed changes

@lpincalpinca 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.

RSLGTM

@lpinca

lpinca commented Apr 15, 2024

Copy link
Copy Markdown
Member

@nodejs/platform-windows

mhdawson
mhdawson approved these changes Apr 15, 2024
View reviewed changes

@mhdawsonmhdawson 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.

LGTM

@mhdawsonmhdawson added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 15, 2024
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 15, 2024
@nodejs-github-bot

nodejs-github-bot commented Apr 15, 2024

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented Apr 16, 2024

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented Apr 17, 2024

Copy link
Copy Markdown
Collaborator

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

@richardlaurichardlau added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 17, 2024
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 17, 2024
@nodejs-github-bot
nodejs-github-bot merged commit b41347e into nodejs:mainApr 17, 2024
@nodejs-github-bot

nodejs-github-bot commented Apr 17, 2024

Copy link
Copy Markdown
Collaborator

Landed in b41347e

aduh95 pushed a commit that referenced this pull request Apr 29, 2024
@huseyinacacak-janea@aduh95
test: split wasi poll test for windows
18aa5d6
PR-URL: #52538Fixes: #51822
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
@aduh95aduh95 mentioned this pull request Apr 30, 2024
Merged
marco-ippolito pushed a commit that referenced this pull request May 2, 2024
@huseyinacacak-janea@marco-ippolito
test: split wasi poll test for windows
e567005
PR-URL: #52538Fixes: #51822
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
@marco-ippolitomarco-ippolito mentioned this pull request May 2, 2024
Merged
marco-ippolito pushed a commit that referenced this pull request May 3, 2024
@huseyinacacak-janea@marco-ippolito
test: split wasi poll test for windows
b6cb74d
PR-URL: #52538Fixes: #51822
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lpincalpincalpinca approved these changes
+1 more reviewer
@mhdawsonmhdawsonmhdawson approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

needs-ciPRs that need a full CI run.testIssues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

flaky: wasi.test-wasi

5 participants

@huseyinacacak-janea@nodejs-github-bot@lpinca@mhdawson@richardlau