Closed Bug 479571 Opened 15 years ago Closed 15 years ago

[mozmill] Add module to have easier access to preferences

Categories

(Mozilla QA Graveyard :: Mozmill Tests, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: whimboo, Assigned: whimboo)

References

Details

Attachments

(1 file, 3 obsolete files)

We should have the availability to read and write preferences. This feature should be put into the utils module which can be included by all tests which need to check or modify preferences.

http://hg.mozilla.org/qa/mozmill-tests/file/2cde62365a0c/shared-modules/test_utilsAPI.js

I'll try to get something written over the weekend.
Blocks: 479500
Blocks: 479314
Attached patch Helper module for preferences (obsolete) — Splinter Review
This module gives access to the preferences service and branch. Further two more helper methods were added which simplify the getter/setter access.
Attachment #363654 - Flags: review?(ctalbert)
No longer blocks: 479314
Blocks: 479314
Summary: Add functions to read/write preferences to the utils module → [mozmill] Add module to have easier access to preferences
Attached patch patch v1.1 (obsolete) — Splinter Review
I messed-up with the license in mostly all tests. Let's get this in correctly.
Attachment #363654 - Attachment is obsolete: true
Attachment #363827 - Flags: review?(ctalbert)
Attachment #363654 - Flags: review?(ctalbert)
Attached patch patch v1.1.1 (obsolete) — Splinter Review
Now with a complete qrefresh...
Attachment #363827 - Attachment is obsolete: true
Attachment #363829 - Flags: review?(ctalbert)
Attachment #363827 - Flags: review?(ctalbert)
Comment on attachment 363829 [details] [diff] [review]
patch v1.1.1

>diff --git a/shared-modules/test_prefsAPI.js b/shared-modules/test_prefsAPI.js

>+function Preferences() {
>+  this.branch  = Components.classes["@mozilla.org/preferences-service;1"].
>+                 getService(Components.interfaces.nsIPrefBranch);
>+  this.service = Components.classes["@mozilla.org/preferences-service;1"].
>+                 getService(Components.interfaces.nsIPrefService);

this.service is never used in this class.  Eliminate it.  r=ctalbert with that.  Otherwise, the patch looks great.  Thanks!
Attachment #363829 - Flags: review?(ctalbert) → review+
(In reply to comment #4)
> >+  this.service = Components.classes["@mozilla.org/preferences-service;1"].
> >+                 getService(Components.interfaces.nsIPrefService);
> 
> this.service is never used in this class.  Eliminate it.  r=ctalbert with that.
>  Otherwise, the patch looks great.  Thanks!

Oh, the class itself doesn't use it. But any test can suffer from it. Let's take an example:

var teardownModule = function(module) {
  module.service.resetPrefs();
}

In that way you can completely reset all changed preferences. I really don't wanna kill it. Clint, please reconsider.
(In reply to comment #5)
> (In reply to comment #4)
> > >+  this.service = Components.classes["@mozilla.org/preferences-service;1"].
> > >+                 getService(Components.interfaces.nsIPrefService);
> > 
> > this.service is never used in this class.  Eliminate it.  r=ctalbert with that.
> >  Otherwise, the patch looks great.  Thanks!
> 
> Oh, the class itself doesn't use it. But any test can suffer from it. Let's
> take an example:
> 
> var teardownModule = function(module) {
>   module.service.resetPrefs();
> }
> 
> In that way you can completely reset all changed preferences. I really don't
> wanna kill it. Clint, please reconsider.
Ok, that's fine, but let's put this as a getter on the class then.  Something like getPrefService which calls into the service and gets the preference service and caches the object at that time as this.service.  Just having it hang out for the entire duration of the test seems wasteful if the test doesn't use it.
Whimboo, I also just noticed that the call to setComplexPref is wrong.  it takes one more value: http://mxr.mozilla.org/mozilla-central/source/modules/libpref/public/nsIPref.idl#160
Sorry to keep "one more thing"ing you.  But in the set pref code, you need break statements or else your switch statement will fall through to the next case.  You don't need these in the getPref function because you early return.
Attached patch patch v2Splinter Review
Ok, seems like the way I have used wasn't the best way. As what we have seen using any functions from nsIPrefService isn't really healthy. That's why I have removed the instantiation completely. Further it will be not possible to get/set a complex pref. If users wanna do that they have to use the methods of nsIPrefBranch directly. Even a prototype isn't really necessary. We only have one instance open all the time while a test is running. So no code has to be shared. All of this makes the class really simple to use.

Clint, I hope you will like the new patch. I personally do it and have learned a lot while working on it.
Attachment #363829 - Attachment is obsolete: true
Attachment #364489 - Flags: review?
Attachment #363829 - Flags: review+ → review-
Attachment #364489 - Flags: review? → review?(ctalbert)
Comment on attachment 364489 [details] [diff] [review]
patch v2

r=ctalbert.  I really like the new approach. Nice work.
Attachment #364489 - Flags: review?(ctalbert) → review+
checked in: http://hg.mozilla.org/qa/mozmill-tests/rev/fd0cd7b00bdd

It can be used in the following way:


var setupModule = function(module) {
  module.controller = mozmill.getBrowserController();
  module.prefs = collector.getModule('prefsAPI').preferences;
}

var testPrefs = function() {
  // simplified access
  prefs.setPref('this.is.my.own.user.pref', true);

  // usage of a function of the nsIPrefBranch
  prefs.branch.clearUserPref('this.is.my.own.user.pref');
}
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Mass move of Mozmill Test related project bugs to newly created components. You can filter out those emails by using "Mozmill-Tests-to-MozillaQA" as criteria.
Component: Mozmill → Mozmill Tests
Product: Testing → Mozilla QA
QA Contact: mozmill → mozmill-tests
Version: Trunk → unspecified
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: