test_property_database.html throws an exception if pref-controlled property's preference does not exist

RESOLVED FIXED in Firefox 44

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

(Blocks: 1 bug)

Trunk
mozilla44
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

2 years ago
[I tripped over this bug when testing patches for bug 837211.]

The testcase test_property_database.html uses the function "SpecialPowers.getBoolPref" to check whether properties are enabled. *But*, this API throws (and causes a test failure) if a property does not exist.

So right now, we are (unintentionally) requiring that all pref-controlled properties *have their pref set to something* during mochitest runs. This seems like an undesirable requirement.

I think we should use a helper-function that wraps the getBoolPref in try{}catch{} to allow for unset pref-controlling properties.
(Assignee)

Comment 1

2 years ago
(In reply to Daniel Holbert [:dholbert] from comment #0)
> The testcase test_property_database.html uses the function
> "SpecialPowers.getBoolPref" to check whether properties are enabled. *But*,
> this API throws (and causes a test failure) if a property does not exist.

Sorry -- I meant to end that sentence with "...if a pref does not exist."  (s/property/pref/)
(Assignee)

Comment 2

2 years ago
Created attachment 8665527 [details] [diff] [review]
demo patch to cause this test failure (leaving "layout.css.grid.enabled" unset)

As an example, here's a patch that triggers this failure, simply by removing layout.css.grid.enabled from all.js and the mochitest-specific prefs file.

With this patch, I get these failures when I run test_property_database.html:
{
355 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_property_database.html | uncaught exception - uncaught exception: Error getting pref 'layout.css.grid.enabled' at :0
    simpletestOnerror@SimpleTest/SimpleTest.js:1517:11
    OnErrorEventHandlerNonNull*@SimpleTest/SimpleTest.js:1504:1
356 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_property_database.html | 'background-blend-mode' listed in gCSSProperties 
    @layout/style/test/test_property_database.html:40:1
357 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_property_database.html | 'box-decoration-break' listed in gCSSProperties 
    @layout/style/test/test_property_database.html:40:1
358 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_property_database.html | 'contain' listed in gCSSProperties 
    @layout/style/test/test_property_database.html:40:1
359 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_property_database.html | uncaught exception - uncaught exception: Error getting pref 'layout.css.grid.enabled' at :0
}

The first failure is the issue described in comment 0. The latter failures are from a similar issue when processing property_database.js (which also uses one-off getBoolPref() calls to check prefs).
(Assignee)

Comment 3

2 years ago
Created attachment 8665529 [details] [diff] [review]
fix v1
Attachment #8665529 - Flags: review?(dbaron)
(Assignee)

Comment 4

2 years ago
The patch just
 (1) creates a new utility function "isBoolPrefSetAndEnabled" which lives in property_database.js
 (2) Performs s/SpecialPowers.getBoolPref/isBoolPrefSetAndEnabled/ in property_database.js and test_property_database.html

With that, the earlier attached demo-patch no longer causes uncaught exceptions / test-failures in test_property_database.html. (And similarly, future pref-controlled CSS properties whose prefs happen to be not-set-by-default will no longer break test_property_database.html.)
(Assignee)

Updated

2 years ago
Blocks: 837211
Comment on attachment 8665529 [details] [diff] [review]
fix v1

I think you should report a test failure in case of an exception (i.e., inside the catch).  Otherwise we risk not taking the pref-guarded chunks out of the pref-guard and leaving things untested, no?  (Especially for prefs that aren't entire properties.)

Maybe call the function CSSPropertyPrefEnabled or something like that?

r=dbaron with that
Attachment #8665529 - Flags: review?(dbaron) → review+
(Assignee)

Comment 6

2 years ago
(In reply to David Baron [:dbaron] ⌚UTC-7 from comment #5)
> I think you should report a test failure in case of an exception (i.e.,
> inside the catch).

So, I initially filed this bug to *avoid* the test-failure that was being caused by this exception, because I was tripping over it with the patch stack in bug 837211 (which currently add some CSS properties behind a hidden pref).  And reporting a test-failure inside of the 'catch' kind of defeats that goal, though it is a bit cleaner than leaning on the harness to catch the exception.

But you've convinced me that we really *should* be considering this scenario (pref-check for a missing pref) a test-failure, particularly in cases like you mentioned where we remove a pref (because a property is fully & solidly supported) and forget to remove its pref-guarded tests.

So, I'll make the changes you requested.  (And I'll have to be sure that bug 837211's patches actually add the pref that controls the new properties there, or else it'll run afoul of this test failure.)
(Assignee)

Comment 7

2 years ago
Created attachment 8667092 [details] [diff] [review]
fix v2

Here's the updated fix with a test-failure added, and with the function renamed to IsCSSPropertyPrefEnabled.

Test-failures used to look like comment 2; now with this patch, they'll look like this:
{
2479 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_property_database.html | Failed to look up property-controlling pref 'layout.css.grid.enabled' (Error getting pref 'layout.css.grid.enabled') 
    IsCSSPropertyPrefEnabled@layout/style/test/property_database.js:22:5
    @layout/style/test/property_database.js:5822:5
}
Attachment #8665529 - Attachment is obsolete: true
Attachment #8667092 - Flags: review+
(Assignee)

Comment 8

2 years ago
Green try run (w/ a few intermittents): https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f295274a2d3

Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/e801ecdbd5f9
(Assignee)

Updated

2 years ago
Flags: in-testsuite-
https://hg.mozilla.org/mozilla-central/rev/e801ecdbd5f9
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox44: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
(Assignee)

Updated

2 years ago
Blocks: 1237119
You need to log in before you can comment on or make changes to this bug.