Closed Bug 880310 Opened 12 years ago Closed 12 years ago

Add some pushPrefEnv tests

Categories

(Testing :: Mochitest, defect)

Product:
Testing
Component:
Mochitest
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla24

People

(Reporter: martijn.martijn, Assigned: martijn.martijn)

Details

Attachments

(1 file, 2 obsolete files)

patch
7.61 KB, patch
jmaher
: review+
Details | Diff | Splinter Review
Attached patch patch (obsolete) — — Splinter Review
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)
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-
Attached patch patch (obsolete) — — Splinter Review
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)
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-
Attached patch patch — — Splinter Review
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+
Keywords: checkin-needed
Assignee: nobody → martijn.martijn
https://hg.mozilla.org/mozilla-central/rev/7df3c21c72e9
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: