773529 - add temporary addProvider/removeProvider methods for testing convenience

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

The current SocialService doesn't support adding/removing providers, which makes it difficult to test certain things without directly creating SocialProvider objects. Let's fix that by adding addProvider/removeProvider helpers (that don't actually persist changes beyond the current session, for the moment).

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

Shane and I collaborated on this one, so let's get Drew to sign-off.

The API is:
addProvider(manifest, callback), where callback receives the added provider.
removeProvider(origin, callback), where callback is called with no arguments when the removal is complete.

Several times I had to correct myself and write removeProvider(provider.origin) rather than removeProvider(provider), so perhaps that particular choice is worth revisiting - I went with an origin to match getProvider{List}, but perhaps that's a foolish consistency since the purpose of the methods are different. On the other hand, having to retrieve a provider just to remove it would be annoying... but I'm not sure how often that'd really happen in practice.

Comment 2

[Drew Willcoxon :adw]

Comment on attachment 641692 [details] [diff] [review]
patch

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

removeProvider(origin) is the right way to go.

I don't understand why the browser test checks provider properties but the xpcshell test doesn't.  Actually I don't understand why you need both tests, but I haven't been closely following recent developments either.

::: toolkit/components/social/SocialService.jsm
@@ +75,5 @@
> +  // Adds a provider given a manifest, and returns the added provider.
> +  addProvider: function addProvider(manifest, onDone) {
> +    let provider = new SocialProvider(manifest, SocialServiceInternal.enabled);
> +    if (SocialServiceInternal.providers[provider.origin])
> +      throw new Error("SocialService.addProvider: provider with this origin already exists");

It's weird to create the new provider before checking if one already exists.  Is there a reason you can't do the check first?

@@ +84,5 @@
> +      onDone(provider);
> +    });
> +  },
> +  
> +  removeProvider: function removeProvider(origin, onDone) {

addProvider has a nice comment, so how about removeProvider too?

@@ +85,5 @@
> +    });
> +  },
> +  
> +  removeProvider: function removeProvider(origin, onDone) {
> +    let provider = SocialServiceInternal.providers[origin];

Won't SpiderMonkey's strict mode log a warning here if !(origin in providers)?  (I don't mean "use strict", but the javascript.options.strict pref.)  To avoid that I'd probably do `if !(origin in providers) throw` and then get the provider.

::: toolkit/components/social/test/browser/browser_SocialProvider.js
@@ +29,5 @@
> +    ok(provider.enabled, "provider is re-enabled");
> +    ok(provider.port, "should be able to get a port from re-enabled provider");
> +    ok(provider.workerAPI, "should be able to get a workerAPI from re-enabled provider");
> +
> +    // Now remove the provider

This comment isn't helping anything.

::: toolkit/components/social/test/browser/browser_workerAPI.js
@@ +25,5 @@
> +      if (topic == "test-initialization-complete") {
> +        is(provider.workerAPI.initialized, true, "workerAPI is now initialized");
> +        // Remove the added provider
> +        SocialService.removeProvider(provider.origin);
> +        finish();

Shouldn't this be

removeProvider(provider.origin, finish)

like you did in browser_SocialProvider.js?

::: toolkit/components/social/test/browser/head.js
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +let SocialService = Components.utils.import("resource://gre/modules/SocialService.jsm", {}).SocialService;

Why not just Components.utils.import("resource://gre/modules/SocialService.jsm")?

::: toolkit/components/social/test/xpcshell/test_SocialService.js
@@ +29,5 @@
>    let next = runner.next.bind(runner);
>    runner.appendIterator(testGetProvider(manifests, next));
>    runner.appendIterator(testGetProviderList(manifests, next));
>    runner.appendIterator(testEnabled(manifests, next));
> +  runner.appendIterator(testAddRemoveProvider(manifests, next));

Eventually it'd be nice to have an array of test functions instead of a grab bag of functions in global scope.  Then you'd loop through it here, calling appendIterator for each one.

@@ +104,5 @@
> +    SocialService.addProvider(manifests[0]);
> +  } catch (ex) {
> +    threw = ex;
> +  }
> +  do_check_true(!!threw);

Check the error message to make sure you're catching what you think you're catching.

@@ +117,5 @@
> +  let retrievedNewProvider = yield SocialService.getProvider(newProvider.origin, next);
> +  do_check_eq(newProvider, retrievedNewProvider);
> +
> +  let providersAfter = yield SocialService.getProviderList(next);
> +  do_check_true(providersAfter.length == originalProviders.length + 1);

do_check_eq?  And it would be more solid to check that the new provider is actually in the list, not just that the list has grown in size.

@@ +122,5 @@
> +
> +  // Now remove the provider
> +  yield SocialService.removeProvider(newProvider.origin, next);
> +  providersAfter = yield SocialService.getProviderList(next);
> +  do_check_eq(providersAfter.length, originalProviders.length);

Same comment here, but this time about checking that the provider should no longer be in the list.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

(In reply to Drew Willcoxon :adw from comment #2)
> I don't understand why the browser test checks provider properties but the
> xpcshell test doesn't.  Actually I don't understand why you need both tests,
> but I haven't been closely following recent developments either.

The FrameWorker code doesn't work when run in xpcshell, because there's no hidden window with which to create the worker. So the tests that depend on testing FrameWorker-dependent stuff (workerAPI etc.) need to be in browser-chrome.

> It's weird to create the new provider before checking if one already exists.
> Is there a reason you can't do the check first?

I used the origin from the provider rather than the one used as input, to handle cases where the provider constructor would do some kind of sanitization of origin or something. I guess that's pointless, I don't really see us changing that, and we can change this back if we do.

> > +let SocialService = Components.utils.import("resource://gre/modules/SocialService.jsm", {}).SocialService;
> 
> Why not just
> Components.utils.import("resource://gre/modules/SocialService.jsm")?

Without this, we trigger the "leaked a global" code at http://hg.mozilla.org/mozilla-central/annotate/32c911bc2579/testing/mochitest/browser-test.js#l228, since it installs the variable on the window which can potentially impact future tests.

> Eventually it'd be nice to have an array of test functions instead of a grab
> bag of functions in global scope.  Then you'd loop through it here, calling
> appendIterator for each one.

Yeah, I have a few ideas for cleaning up tests that I was going to tackle later.

Fixed the rest of the comments, new patch coming up.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch with comments addressed

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch with comments addressed

(forgot one comment)

Comment 6

[:Gavin Sharp [email: [e-mail]]]

That was entirely the wrong patch, but I landed the right one:
http://hg.mozilla.org/integration/mozilla-inbound/rev/567d5343e363

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/567d5343e363