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

Give some more guidance around import-w3c-tests #27

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

annevk
Copy link

@annevk annevk commented Mar 24, 2023

Make it clearer how to checkout web-platform-tests/wpt locally and how to use -s.

Copy link
Member

@stwrt stwrt left a comment

Choose a reason for hiding this comment

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

Can you also add a description to your change on the pull request.

docs/Infrastructure/ImportWPTTests.md Outdated Show resolved Hide resolved
Make it clearer how to checkout web-platform-tests/wpt locally and how to use -s.
@stwrt
Copy link
Member

stwrt commented Apr 5, 2023

Still no description in the pull request 🥺.

The comment looks better 😀.

@annevk
Copy link
Author

annevk commented Apr 5, 2023

Oh sorry, I updated the commit message. I didn't realize we use the PR OP for something. Made that match the commit message body now.

@@ -19,3 +19,5 @@ If you have the upstream WPT repository locally you can skip redownloading it by
```
Tools/Scripts/import-w3c-tests web-platorm-tests/folder_to_import_here -l -s path_to_web_platform_tests
```

Important: your checkout of WPT needs to be in a directory named `web-platform-tests` and the path passed to `-s` should not include that directory. I.e., if your checkout is at `~/web-platform-tests` you pass `-s ~`. `import-w3c-tests` will unfortunately not complain if you got this wrong.
Copy link

@marcoscaceres marcoscaceres Apr 6, 2023

Choose a reason for hiding this comment

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

Maybe:

Suggested change
Important: your checkout of WPT needs to be in a directory named `web-platform-tests` and the path passed to `-s` should not include that directory. I.e., if your checkout is at `~/web-platform-tests` you pass `-s ~`. `import-w3c-tests` will unfortunately not complain if you got this wrong.
**Important**: your checkout of WPT needs to be in a directory named `web-platform-tests` and the path passed to `-s` should not include that directory. I.e., if your checkout is at `~/web-platform-tests` you pass `-s ~`. `import-w3c-tests`. Unfortunately `import-w3c-tests` does not currently report an error if it can't find the path.

We should file a bug on WebKit itself to fix this... like, locally my folder is "~/dev/wpt" and I really would prefer not to rename it to "web-platform-tests".

Copy link
Member

Choose a reason for hiding this comment

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

You Only to do it once, and can get it to that name when you first clone. Maybe that should be in the instructions on how to clone instead

Choose a reason for hiding this comment

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

I get that, but when you clone directly from WPT it's also called "wpt" unless you explicitly alias it (i.e., this a webkit-ism... and not so great if you are just doing general "web work" or standards stuff)... maybe we should be asking for to create a symlink?

Copy link
Author

Choose a reason for hiding this comment

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

I support filing a bug on that to get that changed. It's indeed annoying.

@clopez
Copy link
Contributor

clopez commented May 11, 2023

Here is the old doc with info related to this https://trac.webkit.org/wiki/WebKitW3CTesting but not sure if is still 100% up-to-date.

Maybe there is some info from there that is worth adding here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants