1354066 - Port dom/base/test/test_classList.html to WPT

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P3)

Description

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

This is far more comprehensive than what's in wpt now, especially the version I've updated in bug 869788 <https://reviewboard-hg.mozilla.org/gecko/file/7cfdd595ff41/dom/base/test/test_classList.html>.  We should upstream it so other UAs can use it and it can be updated upstream for spec changes.  There are three potential problems:

1) It contains tests for XUL documents.  I don't think this is a problem per se, as long as it tests that they behave like XML documents of unknown namespace.

2) It contains tests for XUL-specific behavior.

3) It contains tests for mutation events, specifically DOMAttrModified.  This event is not implemented in Chrome and is therefore presumably never going to be standardized, per annevk.

It isn't easy to separate the non-standard bits.  Given the improvement to wpt testing for this property, if we aren't willing to drop the Gecko-specific bits, I would be in favor of upstreaming it with the Gecko-specific bits configured so that they only run in Gecko's internal testing framework and are ignored by other browsers.  This seems best for everyone.  But we should get approval from other UAs for this.

jgraham, if you're not completely opposed to it, who could we ask from other UAs about upstreaming it with some Gecko-specific stuff left in?  It's a bit ugly, and I wouldn't want to make a habit of it, but if I were them, I'd want the better testing.

Alternatively -- bz, would it be acceptable to just drop the XUL and mutation events tests here?

Comment 1

[Boris Zbarsky [:bzbarsky]]

> Alternatively -- bz, would it be acceptable to just drop the XUL and mutation events tests here?

No.  But you could fork the test and upstream a version with those bis removed, while keeping the entirety of the existing test.  And add comments to the existing test pointing out that changes should probably be made to either upstream or both, depending on the change.

Comment 2

[James Graham [:jgraham]]

I'm happy with bz's suggestion. I think upstreaming stuff that we don't expect other people to run (even if it's automatically detected that it shouldn't be run) is going to set a bad precedent that we don't want to deal with in the long run.

Comment 3

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Okay.

Comment 4

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

So it looks like it's not even possible to create a XUL document in a wpt test, and I have to keep the mochitest for the XUL bits.  This is considerably more of a pain to keep two copies of than two wpt tests.  Is it okay if I just leave the existing classList test as-is and only add all the new and exciting tests to the wpt test, which means mutation events and XUL will not be tested for the new tests?  Or do I have to really make a copy of my wpt test somehow work as a mochitest also?  (.replace() is already totally untested by the mochitest, and therefore untested for XUL/mutation events.)

Comment 5

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Sorry, I'm wrong, it works fine.  It doesn't work if I browse to the test in a regular web browser, but if I'm running it in the test harness it works.

Comment 6

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Bug 1354066 - Expand classList testing and port to wpt;

Because it tests some Gecko-specific things as well, I'm making two
copies, as advised by bz and jgraham.  One is to be submitted upstream,
and a second one has local changes.  This means most of the test is run
twice.

This overwrites the preexisting Element-classlist.html test upstream.  I
think I took the useful bits out of it (particularly replace() testing),
but there are some things that it had that I didn't think were
necessary, including: things that belong in idlharness; .className
testing; testing .contains() and stringification and hasAttribute() and
such after add/remove/etc. (instead of just testing getAttribute()); CSS
class selector matching.

Review commit: https://reviewboard.mozilla.org/r/131588/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/131588/

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Or do I have to really make a copy of my wpt test somehow work as a mochitest also? 

Note that we do support testharness mochitests.  So making a wpt test work as a mochitest is quite trivial, last I checked.  See dom/tests/mochitest/general/test_WebKitCSSMatrix.html for an example.

Comment 8

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Good to know.  In that case, why have testing/web-platform/mozilla/ at all?  Do testharness mochitests support expected fails?  (As it turns out, wpt also magically supports creating XUL stuff, so it's not needed here.)

Comment 9

[Boris Zbarsky [:bzbarsky]]

> In that case, why have testing/web-platform/mozilla/ at all?

Because the overall harness is different in various ways.

> Do testharness mochitests support expected fails? 

No.

Comment 10

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8859588 [details]
Bug 1354066 - Expand classList testing and port to wpt;

https://reviewboard.mozilla.org/r/131588/#review134436

Not a big fan of the duplication but it seems like the best option. Mostly LGTM other than that.

::: testing/web-platform/mozilla/tests/dom/classList.html:47
(Diff revision 1)
> -  if (before === null)
> +    if (before === null)
> -    e.removeAttribute("class");
> +      e.removeAttribute("class");
> -  else
> +    else
> -    e.setAttribute("class", before);
> +      e.setAttribute("class", before);

You have a lot of code which looks like this throughout this file - it might make sense to factor it out?

::: testing/web-platform/mozilla/tests/dom/classList.html:187
(Diff revision 1)
> +      assert_equals(e.classList.item(index), expected,
> +                    "classList.item(" + index + ")");
> +    }
>  
> +    function checkItemArray(index, expected) {
> +      assert_equals(e.classList[index], expected, "classList.[" + index + "]");

Why "classList.[...]" instead of "classList[...]"?

Comment 11

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Comment on attachment 8859588 [details]
Bug 1354066 - Expand classList testing and port to wpt;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/131588/diff/1-2/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8833b4289836
Expand classList testing and port to wpt; r=mystor

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8833b4289836