Closed Bug 1066621 Opened 5 years ago Closed 4 years ago

dom/reflection-forms.html webplatform test is going to perma-fail when Gecko 35 merges to beta

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox35 --- wontfix
firefox36 --- wontfix
firefox37 --- wontfix
firefox38 --- wontfix
firefox38.0.5 --- wontfix
firefox39 --- wontfix
firefox40 --- fixed
firefox41 --- verified

People

(Reporter: RyanVM, Assigned: jgraham)

References

Details

Attachments

(4 files, 1 obsolete file)

Attached file Log copypasta
The webplatform tests are due to ride the Gecko 35 train. Per the run below, the reflection-forms.html test will perma-fail when it hits beta.

https://tbpl.mozilla.org/php/getParsedLog.php?id=47902750&tree=Try
HTMLInputElement.inputMode is behind the "dom.forms.inputmode" pref, which is set to false ifdef RELEASE_BUILD.

Can we make the failure annotations for web platform tests conditional on prefs or at least ifdefs?  We'll need this in general for features that are not ready to ship...

For this particular case, are there concrete plans for enabling inputmode?
Flags: needinfo?(bugs)
(In reply to Boris Zbarsky [:bz] from comment #1)
> Can we make the failure annotations for web platform tests conditional on
> prefs or at least ifdefs?  We'll need this in general for features that are
> not ready to ship...

We could probably make it conditional on versions, more easily if mozinfo gave me that (which I sort of thought it did, but it doesn't actually seem to), although it would be a bit of a pain for doing automatic updates to the metadata, because it would have to get the version information from somewhere that as well. I have no idea how to make it conditional on prefs or ifdefs though; the latter seems like the build system would have to be involved, somehow, and the former seems like it would be necessary to launch firefox to query the data.

Do we have any solution for other testsuites here? Do we just disable the tests based on ifdefs in the manifests or something?
There are various options for other test suites.

For this particular case, the test for inputMode in mochitest just sets the preference before it runs, for example.  Similarly, reftests can declare prefs to be set in the manifest.

The other common option is to have the test itself check whether the functionality it's testing is present, and if not not actually test it.  This is not really applicable to the web-platforms-test setup...
So are these prefs not set in prefs_general.js or do we change that when we uplift?
In the case of dom.forms.inputmode, the test just does:

  if (SpecialPowers.getBoolPref("dom.forms.inputmode")) {
    // actually test the reflection
  }

(and yes, I got that wrong in comment 3).
So I'm lost now. If the test does 
  if (SpecialPowers.getBoolPref("dom.forms.inputmode")) {
    // actually test the reflection
  }

how could it go to perm-fail in Gecko 35 beta?
Flags: needinfo?(bugs)
The web-platform-tests test doesn't do that because wpt doesn't have any concept of SpecialPowers, or any way of accessing prefs. bz was comparing to a mochitest that can access prefs.
I see.
So isn't this just a limitation in wpt?
How do other browsers deal with this kind of case when certain
features are in development builds only, but not in beta/release builds?
One possibly option is making regeneration of the web-platform-tests failure manifests part of the merge day steps.  The danger is that this could hide actual issues where something is not enabled on a branch but should be, I guess...
https://tbpl.mozilla.org/php/getParsedLog.php?id=49027028&tree=Try

Looks like dom/interfaces.html is joining the party now as well.

TEST-UNEXPECTED-FAIL | /html/dom/interfaces.html | HTMLInputElement interface: attribute inputMode - assert_true: The prototype object must have a property "inputMode" expected true got false
TEST-UNEXPECTED-FAIL | /html/dom/interfaces.html | HTMLInputElement interface: document.createElement("input") must inherit property "inputMode" with the proper type (17) - assert_inherits: property "inputMode" not found in prototype chain
James, can you please post a patch here for properly disabling these tests so I can test it ASAP?
Flags: needinfo?(james)
Sure if we have a try run for which the logs are still accessible.
Flags: needinfo?(james)
Flags: needinfo?(ryanvm)
This is an auto-generated update based on the logs from the try push given.
Attachment #8519937 - Flags: feedback?(ryanvm)
Comment on attachment 8519937 [details] [diff] [review]
Update web-platform-tests expected data for Gecko 35 / Aurora

Doesn't apply to Aurora at all.
Attachment #8519937 - Flags: feedback?(ryanvm) → feedback-
Attachment #8523309 - Flags: feedback?(ryanvm)
Assignee: nobody → james
Status: NEW → ASSIGNED
Attachment #8519937 - Attachment is obsolete: true
Comment on attachment 8523309 [details] [diff] [review]
Update web-platform-tests expected data

W3 is green :)
Attachment #8523309 - Flags: feedback?(ryanvm) → feedback+
Blocks: 1100444
We can deal with this as part of merging this cycle (bug 1100444 to track that), but this needs to be disabled in a more regular way in the future - applying patches after merging is not how we deal with this sort of thing. Usually things like this are configurable via something in the mozconfig, or do some magic that works even after uplift (eg: https://mxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.js#147).
Comment on attachment 8523309 [details] [diff] [review]
Update web-platform-tests expected data

I pushed this to mozilla-beta.
Attachment #8523309 - Flags: checkin+
From the looks of it, we're on track to have a grumpy RelEng (and/or sheriff) again in a few weeks :(
Seems unlikely that we're going to get anything landed on Aurora in time to avoid this happening again next week.
Looks like the expected data patch needs updating for Gecko 38. With it applied, I'm still seeing the following new failures on the Trunk-as-Beta simulation:
https://treeherder.mozilla.org/ui/logviewer.html#?job_id=4503060&repo=try

James, is that enough for you or do you need a run without the patch from this bug included?
Flags: needinfo?(james)
This should be enough. For the record the procedure to generate the new expected data is

(fetch and checkout a525deb54ade)
mkdir logs
cd logs
python ../testing/web-platform/update/fetchlogs.py try a525deb54ade
cd ..
./mach web-platform-tests-update logs/*.log

Which should create a patch with the new expected data. It's better tested with a git tree than a hg one, but in theory works with either.
Flags: needinfo?(james)
Ran the steps above (in hg, for the record); got this patch.
Attachment #8556563 - Flags: feedback?(ryanvm)
Comment on attachment 8556563 [details] [diff] [review]
More metadata updates

Works as expected, thanks :)
Attachment #8556563 - Flags: feedback?(ryanvm) → feedback+
Another merge, another manual landing :(
https://hg.mozilla.org/releases/mozilla-beta/rev/9c266e3362e3

James, this is getting pretty old...
We can set prefs in the harness now, right? Does that mean we can fix this bug once and for all now? Pretty please? :)
Flags: needinfo?(james)
Flags: needinfo?(james)
Attachment #8617837 - Flags: review?(Ms2ger)
Comment on attachment 8617837 [details] [diff] [review]
inputmode-pref.diff

Review of attachment 8617837 [details] [diff] [review]:
-----------------------------------------------------------------

Seems fair enough.
Attachment #8617837 - Flags: review?(Ms2ger) → review+
https://hg.mozilla.org/mozilla-central/rev/2de21d443c1a
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.