696585 - allow custom prefs to be set for a specific reftest in the manifest

Status: RESOLVED FIXED

(Testing :: Reftest, defect)

Description

[Jonathan Kew [:jfkthame]]

Attachment: patch, support pref(...) annotations in reftest manifest

There are times when it would be useful to set a specific preference for a reftest, when we want to test something other than the current default configuration.

An example from text rendering: we intend to land the graphite2 shaping code (bug 631479) initially preffed-off, but would like to have reftest coverage. Forcing the pref on globally for reftests would mean that we're testing a different code path from our shipping configuration for ALL text; it would be better to set it on just for the specific tests that depend on it. This would also mean that we can test both "on" and "off" settings and ensure that we get the expected (different) behavior from each.

Another example would be the pref that controls which scripts are shaped using harfbuzz vs platform-specific shaping APIs. Currently, we only test the default setting of this pref. As harfbuzz shaping develops, it would be useful to be able to reftest its behavior with additional scripts even before they are switched on by default.

I realize it is possible to use the mochitest framework instead; however, this is considerably less convenient for the developer while creating testcases, testing them manually in the browser, etc. Adding simple pref-setting support to the reftest manifest and harness would simplify life.

(When filing this, I noticed bug 563722, but it's not clear to me whether that's a desirable way forward; anyhow, in the meantime this seems like a much lighter-weight and less disruptive option.)

The attached patch provides basic support for setting reftest prefs. It recognizes a new annotation on reftest manifest lines:

  pref(path.of.pref.to.set,value)

where <value> may be a boolean (true|false), an integer, or a double-quoted string. The named preference will be set for this specific test, and restored to its original value afterwards. Multiple pref() annotations on a line ought to work, AFAICS.

This has some limitations (which could be overcome if we see a use case that justifies the extra effort), but it seems sufficient to be immediately useful. Known issues include:

* If the specified preference doesn't exist at all, it will be set, and then restored to a type-appropriate default of false/0/"", not removed completely. This could affect subsequent tests, if they depend on the complete absence of the pref.

* This is only useful for prefs that apply "live"; those that require a browser restart will have no effect.

* Setting a pref to the wrong type of value is a bad idea; it'll get "restored" to the default for its new type, I believe. Just don't do that.

* Setting prefs disables the caching of canvases across the affected test, which could hurt performance (but I assume it will not be common enough to matter at the moment).

* Strings may not include spaces, as those are top-level token separators in the manifest line; nor may they include internal double-quotes. If these become an issue, maybe we could use percent-encoding or something.

* It's not possible to set "complex" prefs; only bool, int and string are supported.

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: patch, support pref(...) annotations in reftest manifest, v2

Sorry, uploaded a slightly stale version of the patch (I had originally used "pref(name=value)" rather than "pref(name,value)" as described above). This is the refreshed version - apologies for the noise.

Comment 2

[Joel Maher ( :jmaher ) (UTC -8)]

we added something similar in mochitest:specialpowers:
https://bugzilla.mozilla.org/show_bug.cgi?id=621363

this is similar, the one difference that I see is that we found in mochitest when we set a pref, it wouldn't always be updated when we depended on the preference (specifically some font size preferences.  This might not be an issue in reftest since we set the pref before we load the content page that depends on it.  Just thought I would mention it in case it would be of concern.

Comment 3

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

Comment on attachment 568882 [details] [diff] [review]
patch, support pref(...) annotations in reftest manifest, v2

>+                if ((m = m[2].match(/^(true|false)$/))) {
>+                    type = PREF_BOOLEAN;
>+                    value = (m[1] == "true");
>+                } else if ((m = m[2].match(/^"(.*)"$/))) {
>+                    type = PREF_STRING;
>+                    value = m[1];
>+                } else if ((m = m[2].match(/^(-?\d+)$/))) {
>+                    type = PREF_INTEGER;
>+                    value = m[1];
>+                } else {
>+                    throw "Error in pref value in manifest file " + aURL.spec + " line " + lineNo;
>+                }

I'm guessing that you tested this with boolean prefs only.  You need to
assign m[2] to some other variable before this chain so that you don't
end up with m being whatever match returns when there's no match (false?
null?) for the second and third else ifs.

Also, you need to convert the string you have to quote marks and
actually convert the number to a number.

I'd suggest restructuring the whole business to use evalInSandbox on the
preference value (just like the evalInSandbox call above) and then
calling typeof() on it -- and if the type is a number checking that it's
integral.



I think the code that sets things back the way they were should
distinguish between a pref that was set explicitly an a pref 

>+                        try {
>+                            oldVal = prefs.getBoolPref(ps.name);
>+                        } catch (e) {
>+                            oldVal = false;
>+                        }

These *three* catch clases should also report errors (print a
TEST-UNEXPECTED-FAIL and increment gTestResults.AssertionUnexpected or
UnexpectedFail) because we shouldn't have any prefs that don't have default
values.  If we do, that's a bug in the code that set up the pref, and it also
implies a bug in this code (because the restoring won't restore to the old
state).  We should prevent that from happening by making it a failure.

>                           slow: slow,
>                           url1: testURI,
>-                          url2: null } );
>+                          url2: null,
>+                          prefSettings: prefSettings } );

3 times: I think it makes more sense to put prefSettings between
slow and url1.  It also introduces fewer diffs.




The way you clear gURICanvases (twice) is a problem -- we do this thing
where we count the number of occurrences in advance where we're going to
need the canvas.  This will mean that if a pref() is used between two
occurrences of a test or reference we'll end up with that test or
reference's canvas (a lot of memory) hanging around for the rest of the
reftest run.

Instead, you should make tests that have pref settings not count (in
BuildUseCounts) and not check gURICanvases (in the various places we
manipulate it, which could perhaps use consolidating).




r=dbaron with those things fixed, but I'd like to look at the revised
patch, so marking as review-

Comment 4

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

(In reply to David Baron [:dbaron] from comment #3)
> I think the code that sets things back the way they were should
> distinguish between a pref that was set explicitly an a pref 

Sorry, I meant to delete that comment, but managed to delete only part of it.

Comment 5

[Jonathan Kew [:jfkthame]]

Attachment: patch, support pref(...) annotations in reftest manifest, v3

This should handle boolean, integer and string prefs properly now, and fixes the canvas-counting issues. Also includes tests (of modifying each type of pref) to make sure it's actually working as intended.

Comment 6

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

Comment on attachment 583924 [details] [diff] [review]
patch, support pref(...) annotations in reftest manifest, v3

>+pref(font.size.variable.x-western,16) == font-size-16.html font-default.html
>+pref(font.size.variable.x-western,24) == font-size-24.html font-default.html

Could you check the inverses with != ?

>+pref(font.default.x-western,"serif") == font-serif.html font-default.html
>+pref(font.default.x-western,"sans-serif") == font-sans-serif.html font-default.html

Likewise.

>+                } else if (type == "number" && (parseFloat(val) == parseInt(val))) {

You can probably just check that parseInt(val) == val, since val
is already a number.

I'd slightly prefer having two different variables called type
here as well, one for the result of typeof() and the other for
PREF_* types.


And, sorry, I missed this before:  you need to document the pref()
function in layout/tools/reftest/README.txt .  You should explain how
it affects both test and reference.  (Right?)


(Maybe beware of merge conflicts with bug 704509?)


r=dbaron

Comment 7

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

(In reply to David Baron [:dbaron] from comment #6)
> I'd slightly prefer having two different variables called type
> here as well, one for the result of typeof() and the other for
> PREF_* types.

Er, they can't both be called type.  I meant you're using the variable called type for two different things in sequence, and I'd prefer you use two different variables; obviously one needs to have a different name.

Comment 8

[Jonathan Kew [:jfkthame]]

Adjusted as per review comments, and pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f730d670f5c4

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/f730d670f5c4