workerURL should be optional

RESOLVED FIXED in Firefox 16

Status

()

Firefox
SocialAPI
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Gavin, Assigned: Gavin)

Tracking

Trunk
Firefox 16
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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.
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!
Assignee: nobody → gavin.sharp
Status: NEW → ASSIGNED
Attachment #639480 - Flags: review?(adw)

Comment 2

5 years ago
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.
Attachment #639480 - Flags: review?(adw) → review+
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
Flags: in-testsuite+
Target Milestone: --- → Firefox 16
https://hg.mozilla.org/mozilla-central/rev/aef382189fef
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.