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: move custom WHATWG URL tests into separate files #22442

Closed
wants to merge 1 commit into from

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Aug 21, 2018

To enable automatic update of WPT, move all our custom
WHATWG URL tests that are not present in the upstream into
files starting with test-whatwg-url-custom-, so it's easier
to identify test cases that can be upstreamed and test cases
that should be rolled into our repo (possibly with automation).

I also have a WIP branch of node-core-utils for git node wpt that automatically roll the upstream into our repo (there are currently 2 any.js that fail due to dependency of other Web APIs). There are still a few pending changes to be made to run the window.js/.html tests properly.

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 Aug 21, 2018
@joyeecheung
Copy link
Member Author

joyeecheung commented Aug 21, 2018

To enable automatic update of WPT, move all our custom
WHATWG URL tests that are not present in the upstream into
files starting with `test-whatwg-url-custom-`, so it's easier
to identify test cases that can be upstreamed and test cases
that should be rolled into our repo (possibly with automation).
Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

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

Thanks! Excited to see automated WPT updates.

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

Mostly rubber stamp LGTM

@targos
Copy link
Member

targos commented Aug 22, 2018

Just thinking out loud: would it help to isolate wpt in test/wpt?

@jasnell
Copy link
Member

jasnell commented Aug 22, 2018

...would it help to isolate wpt ...

I think so, yes. In either test/wpt or deps/wpt

@joyeecheung
Copy link
Member Author

@targos For the custom tests I think it's better if they stay in parallel until they are upstreamed (otherwise it does not make sense to put tests that are not written in WPT format into a folder called wpt). For the rolled tests I think this is a good idea (although we cannot run them parallel that way, but they are not really time-consuming anyway). In my WIP branch I have tried both putting the drivers in multiple files (one parallel/test-whatwg-url-* corresponds to one upstream .any.js), and putting them in one driver that loops through a list and run time in a sandbox, it doesn't seem to make too much difference since the running time of neither is noticeable.

@joyeecheung
Copy link
Member Author

Also for the tests downloaded from the upstream I think it makes sense to put them in test/fixtures and read them to run in a sandbox in scripts under test/wpt. It's a bit weird to put them in deps because they will not be compiled into the releases.

@joyeecheung
Copy link
Member Author

@joyeecheung
Copy link
Member Author

@joyeecheung
Copy link
Member Author

ARM CI is green

@joyeecheung
Copy link
Member Author

Landed in 6dd694c, thanks!

joyeecheung added a commit that referenced this pull request Aug 25, 2018
To enable automatic update of WPT, move all our custom
WHATWG URL tests that are not present in the upstream into
files starting with `test-whatwg-url-custom-`, so it's easier
to identify test cases that can be upstreamed and test cases
that should be rolled into our repo (possibly with automation).

PR-URL: #22442
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
addaleax pushed a commit that referenced this pull request Aug 27, 2018
To enable automatic update of WPT, move all our custom
WHATWG URL tests that are not present in the upstream into
files starting with `test-whatwg-url-custom-`, so it's easier
to identify test cases that can be upstreamed and test cases
that should be rolled into our repo (possibly with automation).

PR-URL: #22442
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
targos pushed a commit that referenced this pull request Sep 3, 2018
To enable automatic update of WPT, move all our custom
WHATWG URL tests that are not present in the upstream into
files starting with `test-whatwg-url-custom-`, so it's easier
to identify test cases that can be upstreamed and test cases
that should be rolled into our repo (possibly with automation).

PR-URL: #22442
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
targos pushed a commit that referenced this pull request Sep 6, 2018
To enable automatic update of WPT, move all our custom
WHATWG URL tests that are not present in the upstream into
files starting with `test-whatwg-url-custom-`, so it's easier
to identify test cases that can be upstreamed and test cases
that should be rolled into our repo (possibly with automation).

PR-URL: #22442
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants