Bug 257674

Summary: import-w3c-tests, -s, and web-platform-tests/css/selectors
Product: WebKit Reporter: Anne van Kesteren <annevk>
Component: Tools / TestsAssignee: Sam Sneddon [:gsnedders] <gsnedders>
Status: RESOLVED FIXED    
Severity: Normal CC: clopez, gsnedders, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=259750
https://bugs.webkit.org/show_bug.cgi?id=260349

Description Anne van Kesteren 2023-06-02 23:29:28 PDT
When I run

    import-w3c-tests web-platform-tests/css/selectors --clear-dest-dir -a -l -s ../../Documents/GitHub.nosync

it ends up removing 6 files

    LayoutTests/imported/w3c/web-platform-tests/css/selectors/is-default-ns-001-expected.html

among them. That doesn't appear to be the correct behavior as those files are needed.
Comment 1 Radar WebKit Bug Importer 2023-06-02 23:29:39 PDT
<rdar://problem/110201843>
Comment 2 Anne van Kesteren 2023-06-03 10:40:27 PDT
I'm not sure if we want to track this separately, but for the same PR I also ended up synchronizing web-platform-tests/css/cssom and I noticed it started making changes to some files of web-platform-tests/css/cssom-view.

This tool is quite broken. :-(
Comment 3 Sam Sneddon [:gsnedders] 2023-06-05 14:42:41 PDT
(In reply to Anne van Kesteren from comment #2)
> I'm not sure if we want to track this separately, but for the same PR I also
> ended up synchronizing web-platform-tests/css/cssom and I noticed it started
> making changes to some files of web-platform-tests/css/cssom-view.
> 
> This tool is quite broken. :-(

Please track that separately, that sounds like a totally separate set of bugs.
Comment 4 Anne van Kesteren 2023-06-08 09:31:33 PDT
Filed bug 257855.
Comment 5 Sam Sneddon [:gsnedders] 2023-06-23 10:59:01 PDT
(In reply to Anne van Kesteren from comment #0)
> When I run
> 
>     import-w3c-tests web-platform-tests/css/selectors --clear-dest-dir -a -l
> -s ../../Documents/GitHub.nosync
> 
> it ends up removing 6 files
> 
>    
> LayoutTests/imported/w3c/web-platform-tests/css/selectors/is-default-ns-001-
> expected.html
> 
> among them. That doesn't appear to be the correct behavior as those files
> are needed.

The upstream /css/selectors/is-default-ns-001.html contains:

<link rel="match" href="/css/reference/blank.html">

So it still has a ref, so it's surprising that it doesn't get imported successfully.
Comment 6 Sam Sneddon [:gsnedders] 2023-08-02 09:51:27 PDT
I can't reproduce this:

```
./Tools/Scripts/import-w3c-tests web-platform-tests/css/selectors --clear-dest-dir -a -t
```

Results in:

```
On branch main
Your branch is up to date with 'origin/main'.

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
	modified:   LayoutTests/imported/w3c/resources/import-expectations.json
	modified:   LayoutTests/imported/w3c/web-platform-tests/css/selectors/invalidation/user-action-pseudo-classes-in-has.html
	modified:   LayoutTests/imported/w3c/web-platform-tests/css/selectors/invalidation/w3c-import.log
	modified:   LayoutTests/imported/w3c/web-platform-tests/css/selectors/modal-pseudo-class.html
	modified:   LayoutTests/imported/w3c/web-platform-tests/css/selectors/w3c-import.log

Untracked files:
  (use "git add <file>..." to include in what will be committed)
	LayoutTests/imported/w3c/web-platform-tests/css/selectors/dir-pseudo-update-document-element-expected.html
	LayoutTests/imported/w3c/web-platform-tests/css/selectors/dir-pseudo-update-document-element-ref.html
	LayoutTests/imported/w3c/web-platform-tests/css/selectors/dir-pseudo-update-document-element.html
	LayoutTests/imported/w3c/web-platform-tests/css/selectors/invalidation/crashtests/
	LayoutTests/imported/w3c/web-platform-tests/css/selectors/invalidation/is-pseudo-containing-sibling-relationship-in-has.html
	LayoutTests/imported/w3c/web-platform-tests/css/selectors/invalidation/negated-nth-child-when-ancestor-changes-expected.html
	LayoutTests/imported/w3c/web-platform-tests/css/selectors/invalidation/negated-nth-child-when-ancestor-changes-ref.html
	LayoutTests/imported/w3c/web-platform-tests/css/selectors/invalidation/negated-nth-child-when-ancestor-changes.html
	LayoutTests/imported/w3c/web-platform-tests/css/selectors/invalidation/negated-nth-last-child-when-ancestor-changes-expected.html
	LayoutTests/imported/w3c/web-platform-tests/css/selectors/invalidation/negated-nth-last-child-when-ancestor-changes-ref.html
	LayoutTests/imported/w3c/web-platform-tests/css/selectors/invalidation/negated-nth-last-child-when-ancestor-changes.html
	LayoutTests/imported/w3c/web-platform-tests/css/selectors/invalidation/not-pseudo-containing-sibling-relationship-in-has.html

no changes added to commit (use "git add" and/or "git commit -a")
```

Note there's no deleted files.
Comment 7 Sam Sneddon [:gsnedders] 2023-08-02 16:20:29 PDT
This still fails to reproduce with `--clear-dest-dir` corrected to `--clean-dest-dir` (bug 259750).
Comment 8 Sam Sneddon [:gsnedders] 2023-08-17 04:08:20 PDT
Anne asked if this reproduced with `-s` specifically, and:

```
./Tools/Scripts/import-w3c-tests web-platform-tests/css/selectors --clean-dest-dir -a -t -s ~/projects/wpt
```

Results in:

```
On branch main
Your branch is up to date with 'origin/main'.

Changes not staged for commit:
  (use "git add/rm <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
	modified:   LayoutTests/imported/w3c/resources/import-expectations.json
	modified:   LayoutTests/imported/w3c/resources/resource-files.json
	deleted:    LayoutTests/imported/w3c/web-platform-tests/css/selectors/dir-pseudo-on-input-element-expected.txt
	modified:   LayoutTests/imported/w3c/web-platform-tests/css/selectors/invalidation/user-action-pseudo-classes-in-has.html
	modified:   LayoutTests/imported/w3c/web-platform-tests/css/selectors/invalidation/w3c-import.log
	deleted:    LayoutTests/imported/w3c/web-platform-tests/css/selectors/is-default-ns-001-expected.html
	deleted:    LayoutTests/imported/w3c/web-platform-tests/css/selectors/is-default-ns-002-expected.html
	deleted:    LayoutTests/imported/w3c/web-platform-tests/css/selectors/is-default-ns-003-expected.html
	modified:   LayoutTests/imported/w3c/web-platform-tests/css/selectors/is-where-error-recovery.html
	modified:   LayoutTests/imported/w3c/web-platform-tests/css/selectors/is-where-parsing.html
	modified:   LayoutTests/imported/w3c/web-platform-tests/css/selectors/modal-pseudo-class.html
	deleted:    LayoutTests/imported/w3c/web-platform-tests/css/selectors/not-default-ns-001-expected.html
	deleted:    LayoutTests/imported/w3c/web-platform-tests/css/selectors/not-default-ns-002-expected.html
	deleted:    LayoutTests/imported/w3c/web-platform-tests/css/selectors/not-default-ns-003-expected.html
	modified:   LayoutTests/imported/w3c/web-platform-tests/css/selectors/parsing/parse-has-disallow-nesting-has-inside-has.html
	modified:   LayoutTests/imported/w3c/web-platform-tests/css/selectors/parsing/parse-has.html
	modified:   LayoutTests/imported/w3c/web-platform-tests/css/selectors/w3c-import.log

Untracked files:
  (use "git add <file>..." to include in what will be committed)
	LayoutTests/imported/w3c/web-platform-tests/css/selectors/dir-pseudo-update-document-element-expected.html
	LayoutTests/imported/w3c/web-platform-tests/css/selectors/dir-pseudo-update-document-element-ref.html
	LayoutTests/imported/w3c/web-platform-tests/css/selectors/dir-pseudo-update-document-element.html
	LayoutTests/imported/w3c/web-platform-tests/css/selectors/has-sibling-chrome-crash.html
	LayoutTests/imported/w3c/web-platform-tests/css/selectors/invalidation/crashtests/
	LayoutTests/imported/w3c/web-platform-tests/css/selectors/invalidation/is-pseudo-containing-sibling-relationship-in-has.html
	LayoutTests/imported/w3c/web-platform-tests/css/selectors/invalidation/negated-nth-child-when-ancestor-changes-expected.html
	LayoutTests/imported/w3c/web-platform-tests/css/selectors/invalidation/negated-nth-child-when-ancestor-changes-ref.html
	LayoutTests/imported/w3c/web-platform-tests/css/selectors/invalidation/negated-nth-child-when-ancestor-changes.html
	LayoutTests/imported/w3c/web-platform-tests/css/selectors/invalidation/negated-nth-last-child-when-ancestor-changes-expected.html
	LayoutTests/imported/w3c/web-platform-tests/css/selectors/invalidation/negated-nth-last-child-when-ancestor-changes-ref.html
	LayoutTests/imported/w3c/web-platform-tests/css/selectors/invalidation/negated-nth-last-child-when-ancestor-changes.html
	LayoutTests/imported/w3c/web-platform-tests/css/selectors/invalidation/not-pseudo-containing-sibling-relationship-in-has.html

no changes added to commit (use "git add" and/or "git commit -a")
```

So… yes, it does. Well, that's cursed. That it manages to delete files in the simpler codepath but not the complex one seems deeply cursed.
Comment 9 Sam Sneddon [:gsnedders] 2023-08-17 04:29:46 PDT
Ah, there's a bunch of errors like:

WARNING: /Users/gsnedders/projects/wpt/css/reference/blank.html not found. Possible error in the test.

So we're resolving absolute URLs against the wrong base directory.
Comment 10 Sam Sneddon [:gsnedders] 2023-08-17 04:44:27 PDT
Bug 200717 seems to have deliberately have implemented this differing behaviour with `-s`; see `test2.html` and `test3.html` in https://github.com/WebKit/WebKit/blob/6c660bb7b4f5b27281db488bcb8890cee00a1f0b/Tools/Scripts/webkitpy/w3c/test_importer_unittest.py#L573-L593

Carlos, do you remember why you implemented this?
Comment 11 Sam Sneddon [:gsnedders] 2023-08-17 05:00:50 PDT
Pull request: https://github.com/WebKit/WebKit/pull/16787
Comment 12 EWS 2023-08-21 08:14:51 PDT
Committed 267087@main (c2f1b3775319): <https://commits.webkit.org/267087@main>

Reviewed commits have been landed. Closing PR #16787 and removing active labels.