1496351 - [css-contain] Move tests from vendor-imports to WPT standard folder

Status: RESOLVED DUPLICATE of bug 1423971

(Testing :: web-platform-tests, defect)

Description

[Manuel Rego Casasnovas]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/69.0.3497.100 Safari/537.36

Steps to reproduce:


Talking to Emilio it seems a good idea to move the tests from
testing/web-platform/tests/css/vendor-imports/mozilla/mozilla-central-reftests/contain/
to
testing/web-platform/tests/css/css-contain/

Some of the tests need updates, that can be done later (see bug #1491235).

Comment 1

[Manuel Rego Casasnovas]

Attachment: 0001-Bug-1496351-css-contain-Move-tests-from-vendor-impor.patch

Comment 2

[Daniel Holbert [:dholbert]]

This seems nice, but I'm worried that the (semi)-automatic "import" process might just recreate all these files in their old location after you've moved them away...

Do you know how these files got in their current location?  (I think dbaron has a script that periodically synchronizes our layout/reftests/w3c-css/submitted directory, to some upstream location -- I'm not sure if it's directly to this "vendor-imports" spot or to somewhere else.  But it seems like the most important part of a change like this would be to fix that automated process so that we don't end up recreating the old copies of the files.)

dbaron might have thoughts on what we should do here, too, since he maintains at least part of the layout/reftests/w3c-css synchronization process.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Oh, right, we probably need to remove them from `layout/reftests/w3c-css` as well.

Comment 4

[Daniel Holbert [:dholbert]]

That's basically bug 1423971, which is semi-blocked on bug 1263568 (making the WPT harness smarter so that we're not losing anything when we get rid of the Mozilla-specific reftest-harness copies).

Though: now that the WPT harness supports assertions at least (bug 1265584), maybe there's less of a compelling reason to keep using the Mozilla-specific reftest-harness,

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

We also don't run the web-platform-test reftests in the css/ subdirectory on any platform other than Linux:
https://searchfox.org/mozilla-central/rev/924e3d96d81a40d2f0eec1db5f74fc6594337128/testing/mozharness/scripts/web_platform_tests.py#217-218

We need to run them on Mac, Windows, and Android.  Bug 1481723 covers Mac and Windows.

This also seems like it's basically a duplicate of bug 1423971.

Comment 6

[Daniel Holbert [:dholbert]]

Yeah, let's dupe this into bug 1423971.

I don't think we can take this patch as-is, because our tooling will just immediately recreate the files that are being deleted here (it'll sync them from layout/reftests/w3c-css).  And we can't remove those original copies yet, per comment 5.

I suppose one thing we *could* do here (before bug 1423971 is ready) would be to update our layout/reftests/w3c-css synchronization tools to synchronize certain subdirectories (e.g. this "contain" directory) to a different target (e.g. to css/css-contain/).  But I'd lean against that unless it were absolutely necessary, since it adds some complexity to a tool/workflow that's going away soon anyway (assuming we can make progress on bug 1423971 soon.)

Comment 7

[Manuel Rego Casasnovas]

Ok, thank you very much for all the feedback.
It'd be nice to get rid of those vendor-import tests at some point,
good to know the work in in progress.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Also, if you want to land fixes to those tests, it's probably fine if you either:
 * land them in w-p-t and then let me know about it so I can land the changes on our side, or
 * post patches in bugzilla and ask somebody to land them in our repo, at which point I'll import them to w-p-t.

Comment 9

[Manuel Rego Casasnovas]

(In reply to David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-7 from comment #8)
> Also, if you want to land fixes to those tests, it's probably fine if you
> either:
>  * land them in w-p-t and then let me know about it so I can land the
> changes on our side, or
>  * post patches in bugzilla and ask somebody to land them in our repo, at
> which point I'll import them to w-p-t.

Thanks again for the information.
I thought the only option was the second one, but it's good to now
that the first one would be a valid approach too.

Anyway for this particular case I've been brave enough to try to fix
the bug and update the tests directly (see bug #1491235),
let's hope the patch makes some sense. O:-)

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 9014319 [details] [diff] [review]
0001-Bug-1496351-css-contain-Move-tests-from-vendor-impor.patch

(Marking this as r- to reflect reality per comment 6)