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

RESOLVED FIXED in mozilla20

Status

Testing
Reftest
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: heycam, Assigned: heycam)

Tracking

unspecified
mozilla20
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Created attachment 658816 [details] [diff] [review]
patch
Attachment #658816 - Flags: review?(dbaron)
Comment on attachment 658816 [details] [diff] [review]
patch

r=dbaron
Attachment #658816 - Flags: review?(dbaron) → review+
(Assignee)

Comment 2

5 years ago
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

5 years ago
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.
(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.
(Assignee)

Comment 5

5 years ago
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?
(Assignee)

Comment 6

5 years ago
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.
Attachment #658816 - Attachment is obsolete: true
Attachment #679016 - Flags: review?(dbaron)
(Assignee)

Updated

5 years ago
Summary: allow pref() on "include" statements in reftest manifests → allow "default-preferences" statement in reftest manifests to avoid repeating prefs on many tests
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
Attachment #679016 - Flags: review?(dbaron) → review+
(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.
(Assignee)

Comment 9

5 years ago
(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.
(Assignee)

Comment 10

5 years ago
(For comment #9.)
Flags: needinfo?(dbaron)
That's fine; no need to wait on me for little things like that before landing.
Flags: needinfo?(dbaron)
(Assignee)

Comment 12

5 years ago
Thanks.

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

Comment 13

5 years ago
Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/ae5e741238cc
(Assignee)

Comment 14

5 years ago
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
https://hg.mozilla.org/mozilla-central/rev/febc983734f7
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Blocks: 816357
Blocks: 816463
You need to log in before you can comment on or make changes to this bug.