Add some pushPrefEnv tests

RESOLVED FIXED in mozilla24

Status

Testing
Mochitest
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Martijn Wargers (zombie), Assigned: Martijn Wargers (zombie))

Tracking

unspecified
mozilla24
x86
Mac OS X
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
Created attachment 759189 [details] [diff] [review]
patch

I added a mochitest that tests the pushPrefEnv function.

I haven't been able to get SpecialPowers.setComplexValue to work at all, I got js failures. It doesn't seem to be used in the source code at all.

prefBranch.setComplexValue is this:
http://mxr.mozilla.org/mozilla-central/source/modules/libpref/public/nsIPrefBranch.idl#177
..where type represents nsIFile, nsISupportsString (UniChar) or nsIPrefLocalizedString (Localized UniChar)

Afaik, specialpowers tries to pass nsIIDRef, but only strings can be passed from content to chrome, so the code needs to be adjusted, to get setComplexValue to work, afaik.
Attachment #759189 - Flags: review?(jmaher)
(Assignee)

Comment 1

5 years ago
I have only tested this on desktop thus far, btw.
Comment on attachment 759189 [details] [diff] [review]
patch

Review of attachment 759189 [details] [diff] [review]:
-----------------------------------------------------------------

I would like to see errors while setting prefs (i.e. setting the wrong type to an existing pref, etc...)

::: testing/mochitest/tests/test_SpecialPowersPushPrefEnv.html
@@ +8,5 @@
> +<body onload="starttest();">
> +
> +<pre id="test">
> +<script class="testbody" type="text/javascript">
> +function starttest(){

nit, space between () and {

@@ +23,5 @@
> +  try {
> +    SpecialPowers.getBoolPref('test.bool');
> +  } catch(e) {
> +    dump('/**** test.bool not set ****/\n');
> +    setTimeout(test1, 0);

if test.bool isn't set, won't this be a recursive loop?, also dump() should be a ok(false, '...')

@@ +28,5 @@
> +    return;
> +  }
> +
> +  try {
> +    SpecialPowers.getIntPref('test.int')

nit- missing ;
we never validate the value here.

@@ +44,5 @@
> +    return;
> +  }
> +
> +  try {
> +    //SpecialPowers.getComplexValue('test.complex', SpecialPowers.Ci.nsISupportsString);

I would rather comment out the entire try/catch block here.

@@ +57,5 @@
> +
> +function test2() {
> +  // test non-changing values
> +  //, ["test.complex", "test", SpecialPowers.Ci.nsIFile]
> +  SpecialPowers.pushPrefEnv({"set": [["test.bool", true], ["test.int", 1], ["test.char", "test"]]}, test3);

theoretically these are set to these values in starttest, could we pick different values?

@@ +166,5 @@
> +    } catch(e) {
> +      try {
> +        SpecialPowers.getCharPref('test.char');
> +      } catch(e) {
> +         SimpleTest.finish();

I don't like having the only way to call finish() in the catch clause (let alone nested 3 deep).
Attachment #759189 - Flags: review?(jmaher) → review-
(Assignee)

Comment 3

5 years ago
Created attachment 762013 [details] [diff] [review]
patch

I tried to adress your comments as far as possible.

(In reply to Joel Maher (:jmaher) from comment #2)
> I would like to see errors while setting prefs (i.e. setting the wrong type
> to an existing pref, etc...)

I did that for setIntPref and setBoolPref, for setCharPref, I didn't, because booleans and integers are converted to strings automatically by javascript.

> > +  try {
> > +    SpecialPowers.getBoolPref('test.bool');
> > +  } catch(e) {
> > +    dump('/**** test.bool not set ****/\n');
> > +    setTimeout(test1, 0);
> 
> if test.bool isn't set, won't this be a recursive loop?, also dump() should
> be a ok(false, '...')

setBoolPref is asynchronous, that's why this loop is there, to make sure that the value is set. I guess I should remove the dump calls (I just added them in case this somehow would cause a loop (which it shouldn't)).
So I don't see how I could change this code.

> > +function test2() {
> > +  // test non-changing values
> > +  //, ["test.complex", "test", SpecialPowers.Ci.nsIFile]
> > +  SpecialPowers.pushPrefEnv({"set": [["test.bool", true], ["test.int", 1], ["test.char", "test"]]}, test3);
> 
> theoretically these are set to these values in starttest, could we pick
> different values?

That's the purpose of this test, to test non-changing values, see the comment at the beginning of the function.
 
> @@ +166,5 @@
> > +    } catch(e) {
> > +      try {
> > +        SpecialPowers.getCharPref('test.char');
> > +      } catch(e) {
> > +         SimpleTest.finish();
> 
> I don't like having the only way to call finish() in the catch clause (let
> alone nested 3 deep).

I changed this into something else.
Attachment #759189 - Attachment is obsolete: true
Attachment #762013 - Flags: review?(jmaher)
(Assignee)

Comment 4

5 years ago
I addressed the nits and the other things you commented upon.
Comment on attachment 762013 [details] [diff] [review]
patch

Review of attachment 762013 [details] [diff] [review]:
-----------------------------------------------------------------

only 1 real issue with this now!

::: testing/mochitest/tests/test_SpecialPowersPushPrefEnv.html
@@ +49,5 @@
> +  } catch(e) {
> +    dump('/**** test.char not set ****/\n');
> +    setTimeout(test1, 0);
> +    return;
> +  }

the dump statements should be removed.  I would like some safeguard in here if we get in a solid setTimeout(test1, 0) loop.  Can we make this fail after 20 attempts?

@@ +180,5 @@
> +    setTimeout(test11, 0);
> +  } catch(e) {
> +    SimpleTest.finish();
> +  }
> +}

test 9, 10, 11 need some method for failing if we get stuck in a loop, I recommended 20 attempts in test1.
Attachment #762013 - Flags: review?(jmaher) → review-
(Assignee)

Comment 6

5 years ago
Created attachment 762992 [details] [diff] [review]
patch

Ok, addressed those issues in this patch.
Attachment #762013 - Attachment is obsolete: true
Attachment #762992 - Flags: review?(jmaher)
Comment on attachment 762992 [details] [diff] [review]
patch

Review of attachment 762992 [details] [diff] [review]:
-----------------------------------------------------------------

thanks, this looks great.
Attachment #762992 - Flags: review?(jmaher) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
(Assignee)

Updated

5 years ago
Assignee: nobody → martijn.martijn

Comment 9

5 years ago
https://hg.mozilla.org/mozilla-central/rev/7df3c21c72e9
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.