Last Comment Bug 788967 - allow "default-preferences" statement in reftest manifests to avoid repeating prefs on many tests
: allow "default-preferences" statement in reftest manifests to avoid repeating...
Status: RESOLVED FIXED
:
Product: Testing
Classification: Components
Component: Reftest (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla20
Assigned To: Cameron McCormack (:heycam)
:
Mentors:
Depends on:
Blocks: 816357 816463
  Show dependency treegraph
 
Reported: 2012-09-06 01:33 PDT by Cameron McCormack (:heycam)
Modified: 2012-11-29 03:19 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (4.70 KB, patch)
2012-09-06 01:33 PDT, Cameron McCormack (:heycam)
dbaron: review+
Details | Diff | Review
patch (v2) (10.88 KB, patch)
2012-11-06 18:24 PST, Cameron McCormack (:heycam)
dbaron: review+
Details | Diff | Review

Description Cameron McCormack (:heycam) 2012-09-06 01:33:00 PDT
Created attachment 658816 [details] [diff] [review]
patch
Comment 1 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-09-10 17:14:14 PDT
Comment on attachment 658816 [details] [diff] [review]
patch

r=dbaron
Comment 2 Cameron McCormack (:heycam) 2012-09-10 22:10:16 PDT
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 2012-11-04 12:52:37 PST
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-7 (review requests must explain patch) 2012-11-05 09:02:35 PST
(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) 2012-11-05 16:06:56 PST
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) 2012-11-06 18:24:18 PST
Created attachment 679016 [details] [diff] [review]
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-7 (review requests must explain patch) 2012-11-26 12:01:34 PST
Comment on attachment 679016 [details] [diff] [review]
patch (v2)

># HG changeset patch
># User Cameron McCormack <cam@mcc.id.au>
>
>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-7 (review requests must explain patch) 2012-11-26 12:02:49 PST
(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) 2012-11-26 15:50:13 PST
(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) 2012-11-27 14:23:08 PST
(For comment #9.)
Comment 11 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-11-27 14:59:04 PST
That's fine; no need to wait on me for little things like that before landing.
Comment 12 Cameron McCormack (:heycam) 2012-11-27 15:07:05 PST
Thanks.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5218cec180c8
Comment 13 Cameron McCormack (:heycam) 2012-11-27 15:40:56 PST
Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/ae5e741238cc
Comment 14 Cameron McCormack (:heycam) 2012-11-28 01:34:41 PST
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] 2012-11-28 10:26:14 PST
https://hg.mozilla.org/mozilla-central/rev/febc983734f7

Note You need to log in before you can comment on or make changes to this bug.