788967 - allow "default-preferences" statement in reftest manifests to avoid repeating prefs on many tests

Status: RESOLVED FIXED

(Testing :: Reftest, defect)

Description

[Cameron McCormack (:heycam)]

Attachment: patch

Comment 1

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

Comment on attachment 658816 [details] [diff] [review]
patch

r=dbaron

Comment 2

[Cameron McCormack (:heycam)]

Oddly, I got a bunch of consistent reftest failures when I posted this through the try server.  I'll have to investigate further before landing it.

Comment 3

[Jesse Ruderman]

I'm concerned that putting the pref() on the include statement, rather than something at the top of the included file, will cause "TEST_PATH=layout/reftests/foo/ make reftest" to not use prefs it needs.

Comment 4

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

(In reply to Jesse Ruderman from comment #3)
> I'm concerned that putting the pref() on the include statement, rather than
> something at the top of the included file, will cause
> "TEST_PATH=layout/reftests/foo/ make reftest" to not use prefs it needs.

Yeah, I think you're right; we should use something at the top of the manifest.

Comment 5

[Cameron McCormack (:heycam)]

OK, how about syntax like:

* pref("blah","whatever")

(i.e. an asterisk at the start) which is only allowed before any actual tests are encountered?

Comment 6

[Cameron McCormack (:heycam)]

Attachment: patch (v2)

I see that there's already a similar direct (url-prefix) so I think a name like default-preferences would be better.  Here's a patch for this.  It also allows default-preferences in the middle of a manifest, like url-prefix.  And it doesn't inherit into included manifests.

Comment 7

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

Comment on attachment 679016 [details] [diff] [review]
patch (v2)

># HG changeset patch
># User Cameron McCormack <[e-mail]>
>
>Bug 788967 - Add a default-preferences statement for reftest manifests. r=?
>
>
>diff --git a/layout/reftests/reftest-sanity/default-preferences-include.list b/layout/reftests/reftest-sanity/default-preferences-include.list
>new file mode 100644
>index 0000000..2d0de2f
>--- /dev/null
>+++ b/layout/reftests/reftest-sanity/default-preferences-include.list
>@@ -0,0 +1,6 @@
>+# test default-preferences on include commands
>+
>+# In default-preferences-tests.list, the default-preferences line in effect
>+# before the include statement sets different font sizes for the test and
>+# reference files. Those default preferences should not inherit into this file.
>+== font-default.html font-default.html

This should test == font-default.html font-size-16.html

>diff --git a/layout/reftests/reftest-sanity/default-preferences-tests.list b/layout/reftests/reftest-sanity/default-preferences-tests.list
>new file mode 100644
>index 0000000..ad23bfa
>--- /dev/null
>+++ b/layout/reftests/reftest-sanity/default-preferences-tests.list
>@@ -0,0 +1,28 @@
>+# test default-preferences
>+
>+# test default-preferences with a pref()
>+default-preferences pref(font.size.variable.x-western,24)
>+!= font-size-16.html font-default.html
>+== font-size-24.html font-default.html
>+
>+# test that a default preference can be overridden
>+pref(font.size.variable.x-western,16) == font-size-16.html font-default.html
>+pref(font.size.variable.x-western,16) != font-size-24.html font-default.html
>+
>+# test that default preferences are kept when other test-specific preferences are set
>+pref(font.size.variable.zh-HK,36) != font-size-16.html font-default.html
>+pref(font.size.variable.zh-HK,36) == font-size-24.html font-default.html

I tend to think it makes more sense to switch test and ref for these 6 above.

>+
>+# test default-preferences with test-pref() and ref-pref()
>+default-preferences test-pref(font.size.variable.x-western,16) ref-pref(font.size.variable.x-western,24)
>+!= font-default.html font-default.html
>+== font-default.html font-size-16.html
>+== font-size-24.html font-default.html
>+
>+# test that default-preferences does not apply to include commands
>+include default-preferences-include.list
>+
>+# test resetting default-preferences
>+default-preferences
>+== font-default.html font-default.html

This should be testing against font-size-16.html as well.

>diff --git a/layout/tools/reftest/README.txt b/layout/tools/reftest/README.txt
>+4. Specification of default preferences
>+
>+   default-preferences <preference>*
>+
>+   where <preference> is defined above.
>+
>+   The <preference> settings will be used for all following test items in the
>+   manifest.
>+
>+   If a test item includes its own preference settings, then they will override
>+   any settings for preferences of the same names that are set using
>+   default-preferences.

Maybe add "just as later items within a line override earlier ones" (assuming that's true, right?).

>diff --git a/layout/tools/reftest/reftest.js b/layout/tools/reftest/reftest.js
>+        if (items[0] == "default-preferences") {
>+            var m;
>+            var item;
>+            defaultTestPrefSettings = [];
>+            defaultRefPrefSettings = [];
>+            items.shift();
>+            while ((item = items.shift())) {
>+                if ((m = item.match(/^(|test-|ref-)pref\((.+?),(.*)\)$/))) {
>+                    if (!AddPrefSettings(m[1], m[2], m[3], sandbox, defaultTestPrefSettings, defaultRefPrefSettings)) {
>+                        throw "Error in pref value in manifest file " + aURL.spec + " line " + lineNo;
>+                    }
>+                }
>+            }

 - please put the regex in a global variable rather than writing it twice

 - You should also throw if item.match() returns false.  It's probably ok to use the same throw, but you'd need to reword it, though I suppose two separate throw statements might give better error messages, i.e.:
    if (!(m = item.match(...))) {
        throw "unexpected item in default-preferences list" + ...;
    }
    if (!AddPrefSettings(...)) {
        // the throw above
    }

>+            if (items.length)
>+                throw "Error in default-preferences syntax in manifest file " + aURL.spec + " line " + lineNo;

I don't see how items.length could possibly be nonzero at this point.



r=dbaron with those things fixed

Comment 8

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

(In reply to David Baron [:dbaron] from comment #7)
> This should be testing against font-size-16.html as well.

Er, actually, I guess it shouldn't be given what the previous default-preferences line is.

Comment 9

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] from comment #7)
> >--- /dev/null
> >+++ b/layout/reftests/reftest-sanity/default-preferences-include.list
> >@@ -0,0 +1,6 @@
> >+# test default-preferences on include commands
> >+
> >+# In default-preferences-tests.list, the default-preferences line in effect
> >+# before the include statement sets different font sizes for the test and
> >+# reference files. Those default preferences should not inherit into this file.
> >+== font-default.html font-default.html
> 
> This should test == font-default.html font-size-16.html

I don't think so, because we want to check that that default-preferences statement from the including manifest does not apply here.  If it did apply, then the test and reference would render at different sizes.  If it did not apply (as it shouldn't), then we don't actually know what the default font size will be, but we know that it will be the same in the test and reference.

Comment 10

[Cameron McCormack (:heycam)]

(For comment #9.)

Comment 11

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

That's fine; no need to wait on me for little things like that before landing.

Comment 12

[Cameron McCormack (:heycam)]

Thanks.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5218cec180c8

Comment 13

[Cameron McCormack (:heycam)]

Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/ae5e741238cc

Comment 14

[Cameron McCormack (:heycam)]

Relanded with a minor fix (to restore prefs in the reverse order that we apply them; important if we have multiple settings for the same pref for one test):

https://hg.mozilla.org/integration/mozilla-inbound/rev/febc983734f7

Comment 15

[Ed Morley [:emorley]]

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