Skip to content
This repository has been archived by the owner on Feb 26, 2022. It is now read-only.

Bug 789854 PrefTarget() now works #562

Merged
merged 6 commits into from
Sep 13, 2012
Merged

Bug 789854 PrefTarget() now works #562

merged 6 commits into from
Sep 13, 2012

Conversation

erikvold
Copy link
Contributor

No description provided.

@ochameau
Copy link
Contributor

PrefsTarget() now doesn't throw, but it looks pretty weak to say that it actually works.
No need to write a 100% test coverage, but it would be cool to have at least one test that highlights the main use case between PrefsTarget().

@erikvold
Copy link
Contributor Author

PrefsTarget() now doesn't throw

I get the following error:

Traceback (most recent call last):
  File "resource://5e3343f0-ed36-4283-8436-fbf7be0893fc-at-jetpack/api-utils/lib/timer.js", line 28, in notify
    callback.apply(null, args);
  File "resource://5e3343f0-ed36-4283-8436-fbf7be0893fc-at-jetpack/test-harness/lib/run-tests.js", line 59, in 
    onDone: onDone});
  File "resource://5e3343f0-ed36-4283-8436-fbf7be0893fc-at-jetpack/test-harness/lib/harness.js", line 314, in runTests
    nextIteration();
  File "resource://5e3343f0-ed36-4283-8436-fbf7be0893fc-at-jetpack/test-harness/lib/harness.js", line 240, in nextIteration
    onDone: nextIteration
  File "resource://5e3343f0-ed36-4283-8436-fbf7be0893fc-at-jetpack/api-utils/lib/unit-test.js", line 20, in findAndRunTests
    function (tests) {
  File "resource://5e3343f0-ed36-4283-8436-fbf7be0893fc-at-jetpack/api-utils/lib/unit-test-finder.js", line 73, in findTests
    cb(tests);
  File "resource://5e3343f0-ed36-4283-8436-fbf7be0893fc-at-jetpack/api-utils/lib/unit-test.js", line 23, in 
    onDone: options.onDone});
  File "resource://5e3343f0-ed36-4283-8436-fbf7be0893fc-at-jetpack/api-utils/lib/unit-test.js", line 420, in startMany
    runNextTest(this);
  File "resource://5e3343f0-ed36-4283-8436-fbf7be0893fc-at-jetpack/api-utils/lib/unit-test.js", line 415, in runNextTest
    self.start({test: test, onDone: runNextTest});
  File "resource://5e3343f0-ed36-4283-8436-fbf7be0893fc-at-jetpack/api-utils/lib/unit-test.js", line 438, in start
    this.test.testFunction(this);
  File "resource://5e3343f0-ed36-4283-8436-fbf7be0893fc-at-jetpack/api-utils/lib/unit-test-finder.js", line 25, in runTest
    test(runner);
  File "resource://5e3343f0-ed36-4283-8436-fbf7be0893fc-at-jetpack/api-utils/tests/test-prefs-target.js", line 9, in 
    let pt = PrefsTarget();
  File "resource://5e3343f0-ed36-4283-8436-fbf7be0893fc-at-jetpack/api-utils/lib/heritage.js", line 121, in constructor
    return apply(prototype.constructor, create(prototype), arguments);
  File "resource://5e3343f0-ed36-4283-8436-fbf7be0893fc-at-jetpack/api-utils/lib/heritage.js", line 129, in constructor
    this.initialize.apply(this, arguments);
  File "resource://5e3343f0-ed36-4283-8436-fbf7be0893fc-at-jetpack/api-utils/lib/prefs/target.js", line 19, in 
    let branchName = options.branchName || '';
TypeError: options is undefined

@ochameau
Copy link
Contributor

Oh sorry if I wasn't clear. I meant, PrefsTarget doesn't throw with your patch, but it doesn't necessary mean it works. I'm pretty sure it does, but I was suggesting to land this with a better unit test.

@erikvold
Copy link
Contributor Author

@ochameau I've added some better tests now

@erikvold
Copy link
Contributor Author

the simple-prefs module extends the PrefsTarget class, and it has many more tests. But the one included here covers almost everything I think.


test.assertEqual(get(name, ''), '', 'test pref is blank');

pt.once('test', function() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be use name instead of 'test' here and elsewhere.

erikvold added a commit that referenced this pull request Sep 13, 2012
Fix bug 789854 PrefTarget() now works r=@ochameau
@erikvold erikvold merged commit 723788a into mozilla:master Sep 13, 2012
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants