704642 - TPS tests for addon sync engine

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Richard Newman [:rnewman]]

+++ This bug was initially created as a clone of Bug #702814 +++

jgriffin's changes to TPS have landed, but will/may require some followup additions (such as the restartless addon), in addition to the tests themselves, and are not dependent on the implementation details of the engine. 

Unit tests can be found on the addon sync engine bug (534956)

Comment 1

[Gregory Szorc [:gps]]

Attachment: Part 1: use AddonManager APIs

This patch changes the TPS add-ons testing code to use the public AddonManager APIs instead of the private XPIProvider ones.

Comment 2

[Gregory Szorc [:gps]]

Attachment: Part 2: Support repository searching on local HTTP server

This supports stubbing out of repository searches on the test TPS HTTP server.

Comment 3

[Gregory Szorc [:gps]]

These patches are being split out of things rnewman has already given feedback on in bug 534956. This seems like a more appropriate bug for the review.

Comment 4

[Richard Newman [:rnewman]]

(In reply to Gregory Szorc [:gps] from comment #3)
> These patches are being split out of things rnewman has already given
> feedback on in bug 534956. This seems like a more appropriate bug for the
> review.

Thank you :)

Feedback from jgriffin also welcome.

Comment 5

[Jonathan Griffin (:jgriffin)]

Comment on attachment 578923 [details] [diff] [review]
Part 1: use AddonManager APIs

Looks good to me.

Comment 6

[Richard Newman [:rnewman]]

Comment on attachment 578923 [details] [diff] [review]
Part 1: use AddonManager APIs

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

r+ with nits addressed.

::: services/sync/tps/extensions/tps/modules/addons.jsm
@@ +100,5 @@
>        that.addon = addon;
>        Logger.logInfo('addon ' + addon.id + ' found, isActive: ' + addon.isActive);
>        if (state == STATE_ENABLED || state == STATE_DISABLED) {
>            Logger.AssertEqual(addon.isActive,
>              state == STATE_ENABLED ? true : false,

Think about this line for a moment. What does "==" return?

Also, what are the possible values of state? Presumably there are ones other than STATE_ENABLED and STATE_DISABLED. Do you care if you assert on ones other than STATE_ENABLED?

@@ +103,5 @@
>            Logger.AssertEqual(addon.isActive,
>              state == STATE_ENABLED ? true : false,
>              "addon " + that.id + " has an incorrect enabled state");
>        }
>      };

let logAddon = function (addon) {
  ...
}.bind(this);

Note space after "function", use of infixCaps, and bind instead of `that`. You can get rid of `that`.

Then below you can just do

  logAddon(addon);

rather than .call(that, addon).

@@ +113,3 @@
>          addon_found = true;
>          log_addon.call(that, addon);
> +    } else {

Return rather than else. Knock a level of indentation off.

@@ +119,2 @@
>        addonlist = Async.waitForSyncCallback(cb);
> +      for (let i in addonlist) {

What a weird choice of iteration construct.

Either:

  * Use for-each here, and avoid all of those addonlist[i] clauses below, or
  * Use an ordinary fast indexed for-loop.

@@ +121,2 @@
>          if (addonlist[i].addon && addonlist[i].addon.id == that.id &&
>              addonlist[i].state == AddonManager.STATE_INSTALLED) {

This would be much neater with an actual binding for the item! But improve the alignment regardless.

  if (item.addon &&
      item.addon.id == this.id &&
      item.state == AddonManager.STATE_INSTALLED) {

Comment 7

[Richard Newman [:rnewman]]

Comment on attachment 578925 [details] [diff] [review]
Part 2: Support repository searching on local HTTP server

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

This looks good to me. Granted, my Python is so rusty that I might have just given you tetanus…

::: testing/tps/tps/mozhttpd.py
@@ +69,5 @@
> +        if o.path.find('/en-US/firefox/api/1.5/search/guid:') == 0:
> +            ids = urllib.unquote(o.path[len('/en-US/firefox/api/1.5/search/guid:'):])
> +
> +            if ids.count(',') > 0:
> +                raise Exception('Only searching for single ids is currently supported.')

'Searching for multiple ids is not supported'. A little clearer IMO.

Comment 8

[Gregory Szorc [:gps]]

Pushed the 2nd patch to s-c:

https://hg.mozilla.org/services/services-central/rev/4643c1624014

I'm working on fixing the first one now...

Comment 9

[Gregory Szorc [:gps]]

Attachment: Refactor TPS add-ons support

This patch significantly updates the add-ons support in TPS. It is a continuation of the original part 1 to refactor to use the AddonManager APIs. Along the way, I rewrote bits to use the helper APIs present in the add-on sync engine. This reduces the code count significantly. I also took the opportunity to address style nits in the code.

addons.jsm was effectively rewritten, so it is best to apply the patch and look at the new file as a whole.

The add-ons engine isn't checked in yet, so this won't work until it is. The change imports the add-ons engine file, so even though the code paths aren't active, it would break TPS if this were checked in before the engine.

Comment 10

[Jonathan Griffin (:jgriffin)]

Comment on attachment 579926 [details] [diff] [review]
Refactor TPS add-ons support

Looks reasonable.  I didn't actually test it, since I wasn't sure what patches I'd need to get it all working.  test_addon_sanity.js will need to be updated to account for the change in verbs (setState -> setEnabled), and running this test will at least tell us if the TPS components are happy with the changes (that test doesn't actually rely on addon sync to work, it's more of a unit test of TPS's addon functions).

Comment 11

[Gregory Szorc [:gps]]

Attachment: Refactor TPS add-ons support, v2

Here is the latest version of the previously-r+ patch.

I held off committing the original because I wanted the tests to pass first. These TPS tests do pass. They require the most recent version of the add-ons engine and TPS refactoring, however.

Comment 12

[Gregory Szorc [:gps]]

Attachment: Refactor TPS add-ons support, v3

Previous patch was missing some files.

Comment 13

[Gregory Szorc [:gps]]

Pushed to s-c after IRC approval from rnewman. 2 commits because personal stupidity.

https://hg.mozilla.org/services/services-central/rev/883459c2e7a7
https://hg.mozilla.org/services/services-central/rev/a3d8630619eb

Comment 14

[Gregory Szorc [:gps]]

Pushed to m-c:

https://hg.mozilla.org/mozilla-central/rev/4643c1624014
https://hg.mozilla.org/mozilla-central/rev/883459c2e7a7
https://hg.mozilla.org/mozilla-central/rev/a3d8630619eb
https://hg.mozilla.org/mozilla-central/rev/efa03f900513

Comment 15

[Richard Newman [:rnewman]]

Comment on attachment 580327 [details] [diff] [review]
Refactor TPS add-ons support, v3

I'm guessing I reviewed this :)