Closed
Bug 1354066
Opened 7 years ago
Closed 7 years ago
Port dom/base/test/test_classList.html to WPT
Categories
(Core :: DOM: Core & HTML, enhancement, P3)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: ayg, Unassigned)
Details
Attachments
(1 file)
59 bytes,
text/x-review-board-request
|
Details |
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?
Flags: needinfo?(james)
Flags: needinfo?(bzbarsky)
Comment 1•7 years ago
|
||
> 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.
Flags: needinfo?(bzbarsky)
Comment 2•7 years ago
|
||
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.
Flags: needinfo?(james)
Reporter | ||
Comment 3•7 years ago
|
||
Okay.
Updated•7 years ago
|
Priority: -- → P3
Reporter | ||
Comment 4•7 years ago
|
||
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.)
Flags: needinfo?(bzbarsky)
Reporter | ||
Comment 5•7 years ago
|
||
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.
Flags: needinfo?(bzbarsky)
Comment hidden (mozreview-request) |
Comment 7•7 years ago
|
||
> 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.
Reporter | ||
Comment 8•7 years ago
|
||
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•7 years ago
|
||
> 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.
Reporter | ||
Updated•7 years ago
|
Attachment #8859588 -
Flags: review?(james) → review?(bugs)
Updated•7 years ago
|
Attachment #8859588 -
Flags: review?(bugs) → review?(michael)
Comment 10•7 years ago
|
||
mozreview-review |
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[...]"?
Attachment #8859588 -
Flags: review?(michael) → review+
Comment hidden (mozreview-request) |
Comment 12•7 years ago
|
||
Pushed by ayg@aryeh.name: https://hg.mozilla.org/integration/autoland/rev/8833b4289836 Expand classList testing and port to wpt; r=mystor
Updated•7 years ago
|
Attachment #8859588 -
Flags: review?(james)
Comment 13•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8833b4289836
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Assignee | ||
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•