bug 710117 testing that pref listeners are removed on unload #306

Merged
merged 7 commits into from Mar 23, 2012

Conversation

Projects
None yet
2 participants
@erikvold

This comment has been minimized.

Show comment
Hide comment
@erikvold

erikvold Mar 18, 2012

Contributor

@Gozala mind taking a look at this one? I've removed the setTimeout that @mykmelez asked me to do finally.

Contributor

erikvold commented Mar 18, 2012

@Gozala mind taking a look at this one? I've removed the setTimeout that @mykmelez asked me to do finally.

@@ -4,8 +4,8 @@
const { Loader } = require("./helpers");
-const setTimeout = require("timers").setTimeout;
-const notify = require("observer-service").notify;
+const { setTimeout } = require("timers");

This comment has been minimized.

@Gozala

Gozala Mar 19, 2012

Member

This require is not necessary as setTimout is no longer used.

@Gozala

Gozala Mar 19, 2012

Member

This require is not necessary as setTimout is no longer used.

This comment has been minimized.

@erikvold

erikvold Mar 19, 2012

Contributor

It's still used in line 120 for exports.testPrefRemoveListener, though that use should be removed too, I'd like to do that in a separate pull request.

@erikvold

erikvold Mar 19, 2012

Contributor

It's still used in line 120 for exports.testPrefRemoveListener, though that use should be removed too, I'd like to do that in a separate pull request.

This comment has been minimized.

@Gozala

Gozala Mar 19, 2012

Member

Ahh sorry I just have not seen it in this request and assumed it was not used.

@Gozala

Gozala Mar 19, 2012

Member

Ahh sorry I just have not seen it in this request and assumed it was not used.

@Gozala

This comment has been minimized.

Show comment
Hide comment
@Gozala

Gozala Mar 19, 2012

Member

In general I discourage use of loader.sandbox which we have added only in order to avoid rewriting old tests (which we'll do at some point in a future). So if there is any way to test API without an access to a sandbox that's much more preferred alternative. In this case I believe it should be possible to do it without emit by just changing an actual pref no ? If so do you mind doing it this way ?

Member

Gozala commented Mar 19, 2012

In general I discourage use of loader.sandbox which we have added only in order to avoid rewriting old tests (which we'll do at some point in a future). So if there is any way to test API without an access to a sandbox that's much more preferred alternative. In this case I believe it should be possible to do it without emit by just changing an actual pref no ? If so do you mind doing it this way ?

@erikvold

This comment has been minimized.

Show comment
Hide comment
@erikvold

erikvold Mar 22, 2012

Contributor

@Gozala I've removed the use of loader.sandbox now

Contributor

erikvold commented Mar 22, 2012

@Gozala I've removed the use of loader.sandbox now

@Gozala

View changes

packages/addon-kit/lib/simple-prefs.js
},
unload: function manager_unload() {
this._removeAllListeners();
branch.removeObserver("", this._prefObserver);
- },
+ },
+ emit: function(key, value) this._emit(key, value),

This comment has been minimized.

@Gozala

Gozala Mar 22, 2012

Member

I think this changes are no longer required then are they ?

P.S.: It has a merge conflict now.

@Gozala

Gozala Mar 22, 2012

Member

I think this changes are no longer required then are they ?

P.S.: It has a merge conflict now.

@Gozala

View changes

packages/addon-kit/tests/test-simple-prefs.js
+
+ loader.unload();
+
+ sp.prefs["test-listen3"] = false;

This comment has been minimized.

@Gozala

Gozala Mar 22, 2012

Member

This is nice but not quite what I meant, sorry I was not very clear about it. Can you instead of using sp.prefs a non-unloaded one (require('simple-prefs').prefs) to make sure that an actual pref change does not call's listeners on the unloaded prefs module.

@Gozala

Gozala Mar 22, 2012

Member

This is nice but not quite what I meant, sorry I was not very clear about it. Can you instead of using sp.prefs a non-unloaded one (require('simple-prefs').prefs) to make sure that an actual pref change does not call's listeners on the unloaded prefs module.

@Gozala

This comment has been minimized.

Show comment
Hide comment
@Gozala

Gozala Mar 22, 2012

Member

We're almost there, sorry for not being very clear first time around. Also could you please merge with upstream/master cause otherwise it won't auto merge. (P.S. getting rid of changes in simple-prefs.js before merge would probably make it easier)

Member

Gozala commented Mar 22, 2012

We're almost there, sorry for not being very clear first time around. Also could you please merge with upstream/master cause otherwise it won't auto merge. (P.S. getting rid of changes in simple-prefs.js before merge would probably make it easier)

erikvold added some commits Mar 23, 2012

Merge remote-tracking branch 'mozilla/master' into bug-710117
Conflicts:
	packages/addon-kit/lib/simple-prefs.js
@erikvold

This comment has been minimized.

Show comment
Hide comment
@erikvold

erikvold Mar 23, 2012

Contributor

Hey @Gozala I think I made the changes that you requested.

Contributor

erikvold commented Mar 23, 2012

Hey @Gozala I think I made the changes that you requested.

Gozala added a commit that referenced this pull request Mar 23, 2012

Merge pull request #306 from erikvold/bug-710117
fix bug 710117 testing that pref listeners are removed on unload r=@gozala

@Gozala Gozala merged commit ee036f5 into mozilla:master Mar 23, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment