Skip to content

test: ignore helper files in WPTs - #48079

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
panva:wpt-ignore-helpers
May 21, 2023
Merged

test: ignore helper files in WPTs#48079
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
panva:wpt-ignore-helpers

Conversation

@panva

@panvapanva commented May 19, 2023

Copy link
Copy Markdown
Member

Because daily WPT report started timing out on these helper files. These are not test entrypoints so we're not losing any coverage.

@panvapanva added test Issues and PRs related to the tests. fast-track PRs that do not need to wait for 48 hours to land. request-ci Add this label to start a Jenkins CI on a PR. labels May 19, 2023
@nodejs-github-botnodejs-github-bot added the needs-ci PRs that need a full CI run. label May 19, 2023
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 19, 2023
@nodejs-github-bot

nodejs-github-bot commented May 19, 2023

Copy link
Copy Markdown
Collaborator

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

aduh95
aduh95 reviewed May 19, 2023
View reviewed changes
Comment threadtest/common/wpt.js
@panvapanva removed the fast-track PRs that do not need to wait for 48 hours to land. label May 19, 2023
@nodejsnodejs deleted a comment from github-actionsBotMay 19, 2023
@panva

panva commented May 20, 2023

Copy link
Copy Markdown
MemberAuthor

I'm hoping to just rename the one offending file that doesn't follow the, I assume unwritten, convention.

KhafraDev
KhafraDev approved these changes May 20, 2023
View reviewed changes

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

Although there's no harm in skipping these files, I don't understand why they would time out. Is it a deeper problem with the WPT runner?

@panva

panva commented May 20, 2023•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Is it a deeper problem with the WPT runner?

The way it selects files that contain tests is definitely one. Combined with the recent change to register a timeout to keep the event loop spinning being another, I wanted to avoid that but the wpt maintainers weren't keen to acknowledging our event loop and called my patch to a particular test a bug hiding solution instead.

We can go with this workaround for now until I get the time to pull in the native WPT python tool scripts that build the manifest of files to run. That should avoid any future test file selection problems.

@panvapanva added the commit-queue Add this label to land a pull request using GitHub Actions. label May 21, 2023
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 21, 2023
@nodejs-github-bot
nodejs-github-bot merged commit 6439f68 into nodejs:mainMay 21, 2023
@nodejs-github-bot

nodejs-github-bot commented May 21, 2023

Copy link
Copy Markdown
Collaborator

Landed in 6439f68

@panva
panva deleted the wpt-ignore-helpers branch May 21, 2023 13:58
targos pushed a commit that referenced this pull request May 30, 2023
@panva@targos
test: ignore helper files in WPTs
8d2a3b1
PR-URL: #48079
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@targostargos mentioned this pull request Jun 4, 2023
Merged
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
@panva@danielleadams
test: ignore helper files in WPTs
a0d14b0
PR-URL: #48079
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MoLow pushed a commit to MoLow/node that referenced this pull request Jul 6, 2023
@panva@MoLow
test: ignore helper files in WPTs
5c5b1d2
PR-URL: nodejs#48079
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@danielleadamsdanielleadams mentioned this pull request Jul 10, 2023
Merged
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@panva@Ceres6
test: ignore helper files in WPTs
bd39529
PR-URL: nodejs#48079
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@panva@Ceres6
test: ignore helper files in WPTs
f2eeafa
PR-URL: nodejs#48079
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aduh95aduh95aduh95 left review comments
@lpincalpincalpinca approved these changes
@anonriganonriganonrig approved these changes
@KhafraDevKhafraDevKhafraDev approved these changes

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.

6 participants

@panva@nodejs-github-bot@lpinca@anonrig@aduh95@KhafraDev