Last Comment Bug 771344 - workerURL should be optional
: workerURL should be optional
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: SocialAPI (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 16
Assigned To: :Gavin Sharp [email: gavin@gavinsharp.com]
:
: Shane Caraveo (:mixedpuppy)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-05 14:36 PDT by :Gavin Sharp [email: gavin@gavinsharp.com]
Modified: 2012-07-07 12:03 PDT (History)
1 user (show)
gavin.sharp: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (6.82 KB, patch)
2012-07-05 15:02 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
adw: review+
Details | Diff | Splinter Review

Description :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-05 14:36:54 PDT
Shane points out that it's perfectly reasonable for a provider to not have a worker, so there's no reason to enforce the presence of a worker URL in the provider object.
Comment 1 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-05 15:02:10 PDT
Created attachment 639480 [details] [diff] [review]
patch

This also merges the tests into one, since the duplication was getting to me (I know I just landed that a few hours ago). I'm not sure I understand the AsyncRunner magic well enough to be confident that this is right, but it seems to work!
Comment 2 Drew Willcoxon :adw 2012-07-05 16:06:27 PDT
Comment on attachment 639480 [details] [diff] [review]
patch

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

I prefer small, separate test files per SocialService method rather than a big omnibus test_SocialService.js that's only going to get bigger.  (We can factor out common code into head.js, which is what head.js is for.)  I'd really like that changed back before landing, but I bet you disagree.

::: toolkit/components/social/test/xpcshell/test_getProvider.js
@@ +23,5 @@
>    Cu.import("resource://gre/modules/SocialService.jsm");
>  
>    let runner = new AsyncRunner();
> +  runner.appendIterator(testGetProvider(manifests, runner.next.bind(runner)));
> +  runner.appendIterator(testGetProviderList(manifests, runner.next.bind(runner)));

Store runner.next.bind(runner) in a local variable and pass that variable to the generator functions so that we're not repeating it every time a new generator is added.  (If you keep test_SocialService.js.)

@@ +45,5 @@
> +  do_check_true(providers.length >= manifests.length);
> +  for (let i = 0; i < manifests.length; i++) {
> +    do_check_neq(providers.map(function (p) p.origin).indexOf(manifests[i].origin), -1);
> +    do_check_neq(providers.map(function (p) p.workerURL).indexOf(manifests[i].workerURL), -1);
> +    do_check_neq(providers.map(function (p) p.name).indexOf(manifests[i].name), -1);

These just check that the origin, workerURL, and name exist in some provider in the list.  They don't ensure that all three are in the same provider.  What I would do is for each manifest, first find the provider with the right origin, and then check that its workerURL and name are right.
Comment 3 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-06 08:49:58 PDT
Made the test changes you suggested, but kept test_SocialService - we can always split it up again if it gets too unwieldy.

https://hg.mozilla.org/integration/mozilla-inbound/rev/aef382189fef
Comment 4 Ryan VanderMeulen [:RyanVM] 2012-07-07 12:03:31 PDT
https://hg.mozilla.org/mozilla-central/rev/aef382189fef

Note You need to log in before you can comment on or make changes to this bug.