Closed Bug 704642 Opened 9 years ago Closed 8 years ago

TPS tests for addon sync engine

Categories

(Firefox :: Sync, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla11

People

(Reporter: rnewman, Assigned: gps)

References

Details

(Whiteboard: [fixed in services])

Attachments

(2 files, 3 obsolete files)

+++ 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)
Depends on: 686019
Assignee: nobody → ally
Status: NEW → ASSIGNED
Attached patch Part 1: use AddonManager APIs (obsolete) — Splinter Review
This patch changes the TPS add-ons testing code to use the public AddonManager APIs instead of the private XPIProvider ones.
Assignee: ally → gps
Attachment #578923 - Flags: review?(rnewman)
This supports stubbing out of repository searches on the test TPS HTTP server.
Attachment #578925 - Flags: review?(rnewman)
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.
(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 on attachment 578923 [details] [diff] [review]
Part 1: use AddonManager APIs

Looks good to me.
Attachment #578923 - Flags: feedback+
Attachment #578925 - Flags: feedback+
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) {
Attachment #578923 - Flags: review?(rnewman) → review+
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.
Attachment #578925 - Flags: review?(rnewman) → review+
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...
Attached patch Refactor TPS add-ons support (obsolete) — Splinter Review
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.
Attachment #578923 - Attachment is obsolete: true
Attachment #579926 - Flags: review?(rnewman)
Attachment #579926 - Flags: feedback?(jgriffin)
No longer depends on: 534956
Blocks: 534956
Attachment #579926 - Flags: review?(rnewman) → review+
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).
Attachment #579926 - Flags: feedback?(jgriffin) → feedback+
Attached patch Refactor TPS add-ons support, v2 (obsolete) — Splinter Review
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.
Attachment #579926 - Attachment is obsolete: true
Attachment #580325 - Flags: review?(rnewman)
Previous patch was missing some files.
Attachment #580325 - Attachment is obsolete: true
Attachment #580325 - Flags: review?(rnewman)
Attachment #580327 - Flags: review?(rnewman)
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
Whiteboard: [fixed in services]
Comment on attachment 580327 [details] [diff] [review]
Refactor TPS add-ons support, v3

I'm guessing I reviewed this :)
Attachment #580327 - Flags: review?(rnewman) → review+
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.