546595 - Add mochitests for the incompatibleList and incompatibleCheck pages

Status: VERIFIED FIXED

(Toolkit :: Application Update, defect)

Description

[Robert Strong (they/them - no direct email)]

Spinoff of bug 530872 to test the incompatibleList and incompatibleCheck pages in the update UI... this will require adding / removing extensions to the extensions datasource.

Comment 1

[Robert Strong (they/them - no direct email)]

Attachment: patch in progress

Comment 2

[Robert Strong (they/them - no direct email)]

Attachment: patch in progress

Comment 3

[Robert Strong (they/them - no direct email)]

note to self... this also covers the test for bug 567054

Comment 4

[Robert Strong (they/them - no direct email)]

Adding dependency on bug 567184 since I'd like to test with an appDisabled add-on.

Comment 5

[Robert Strong (they/them - no direct email)]

Attachment: patch in progress

This tests pretty much all scenarios but it takes around 5 minutes to complete on my system possibly due to installing / uninstalling add-ons during each test. I've already added an optimization for the creation of the xpi's and now I'm going to try to optimize the install / uninstall.

Comment 6

[Robert Strong (they/them - no direct email)]

Attachment: patch rev1

The first test to run will install the test add-ons and each subsequent test will just set userDisabled for the add-ons that won't be compatible with the app update as appropriate for the test... the test_9999_cleanup.xul will uninstall all of the add-ons.

It currently installs two each of the following add-ons for the tests though I think it is safe to just install one of each.

 * appdisabled
 *   disabled by the application due to being incompatible with the current
 *   toolkit version.
 *
 * compatible
 *   compatible with the current toolkit version and the update's toolkit
 *   version.
 *
 * noupdate
 *   the add-on is compatible with the current toolkit version and does not have
 *   an update to make it compatible with the update's toolkit version. Tests
 *   that need to have all add-ons compatible for the application update can
 *   disable this add-on by setting the gDisableNoUpdateAddon variable to true.
 *
 * updatecompatibility
 *   the add-on is compatible with the current toolkit version and has a
 *   compatibility update to make it compatible with the update's toolkit
 *   version.
 *
 * updateversion
 *   the add-on is compatible with the current toolkit version and has a version
 *   update to make it compatible with the update's toolkit version.
 *
 * userdisabled
 *   disabled by the user and compatible with the current toolkit version but
 *   not the update's toolkit version. This add-on will be disabled after its
 *   install completes.

Comment 7

[Dave Townsend [:mossop]]

Comment on attachment 448478 [details] [diff] [review]
patch rev1

Going to review the rest on the way home, but a couple of comments on the shared code:

>diff --git a/toolkit/mozapps/update/test/chrome/update.sjs b/toolkit/mozapps/update/test/chrome/update.sjs

>+function getUpdateRDF(aParams) {
>+  var addonVersion;
>+  var addonID = aParams.addonID;
>+  var addonUpdateType = addonID.split("_")[0];
>+  var maxVersion = aParams.platformVersion;
>+
>+  switch (addonUpdateType) {
>+    case "updatecompatibility":
>+      // Use "1.0" for the add-on version for the compatibility update case since
>+      // the tests create all add-ons with "1.0" for the version.
>+      addonVersion = "1.0";
>+      maxVersion++;
>+      break;
>+    case "updateversion":
>+      // Use "2.0" for the add-on version for the version update case since the
>+      // tests create all add-ons with "1.0" for the version.
>+      addonVersion = "2.0";
>+      maxVersion++;
>+      break;

Why do we need to increment maxVersion here, it should already be larger than the current platform version right?

>diff --git a/toolkit/mozapps/update/test/chrome/utils.js b/toolkit/mozapps/update/test/chrome/utils.js

> /**
>+ * Gets an application version that is greater than the current application
>+ * version. The version is created by taking the first sequence from the current
>+ * application version, adding 1 to it, and then appending ".0" to it.
>+ *
>+ * @return A version string greater than the current application version string.
>+ */
>+function getNewerAppVersion() {
>+  let appVersion = Services.appinfo.version.split(".")[0];
>+  appVersion++;
>+  return appVersion;
>+}
>+
>+/**
>+ * Gets a platform version that is greater than the current platform version.
>+ * The version is created by taking the first sequence from the current platform
>+ * version, adding 1 to it, and then appending ".0" to it.
>+ *
>+ * @return A version string greater than the current platform version string.
>+ */
>+function getNewerPlatformVersion() {
>+  let platformVersion = Services.appinfo.platformVersion.split(".")[0];
>+  platformVersion++;
>+  return platformVersion;
>+}

These comments don't seem to match what you're actually doing.

Comment 8

[Robert Strong (they/them - no direct email)]

(In reply to comment #7)
> (From update of attachment 448478 [details] [diff] [review])
> Going to review the rest on the way home, but a couple of comments on the
> shared code:
> 
> >diff --git a/toolkit/mozapps/update/test/chrome/update.sjs b/toolkit/mozapps/update/test/chrome/update.sjs
> 
> >+function getUpdateRDF(aParams) {
> >+  var addonVersion;
> >+  var addonID = aParams.addonID;
> >+  var addonUpdateType = addonID.split("_")[0];
> >+  var maxVersion = aParams.platformVersion;
> >+
> >+  switch (addonUpdateType) {
> >+    case "updatecompatibility":
> >+      // Use "1.0" for the add-on version for the compatibility update case since
> >+      // the tests create all add-ons with "1.0" for the version.
> >+      addonVersion = "1.0";
> >+      maxVersion++;
> >+      break;
> >+    case "updateversion":
> >+      // Use "2.0" for the add-on version for the version update case since the
> >+      // tests create all add-ons with "1.0" for the version.
> >+      addonVersion = "2.0";
> >+      maxVersion++;
> >+      break;
> 
> Why do we need to increment maxVersion here, it should already be larger than
> the current platform version right?
You're right... I forgot to update that after changing the mochitest code to send the newer platformVersion. Fixed

> >diff --git a/toolkit/mozapps/update/test/chrome/utils.js b/toolkit/mozapps/update/test/chrome/utils.js
> 
> > /**
> >+ * Gets an application version that is greater than the current application
> >+ * version. The version is created by taking the first sequence from the current
> >+ * application version, adding 1 to it, and then appending ".0" to it.
> >+ *
> >+ * @return A version string greater than the current application version string.
> >+ */
> >+function getNewerAppVersion() {
> >+  let appVersion = Services.appinfo.version.split(".")[0];
> >+  appVersion++;
> >+  return appVersion;
> >+}
> >+
> >+/**
> >+ * Gets a platform version that is greater than the current platform version.
> >+ * The version is created by taking the first sequence from the current platform
> >+ * version, adding 1 to it, and then appending ".0" to it.
> >+ *
> >+ * @return A version string greater than the current platform version string.
> >+ */
> >+function getNewerPlatformVersion() {
> >+  let platformVersion = Services.appinfo.platformVersion.split(".")[0];
> >+  platformVersion++;
> >+  return platformVersion;
> >+}
> 
> These comments don't seem to match what you're actually doing.
fixed the comments to read
/**
 * Gets a platform version that is greater than the current platform version.
 * The version is created by taking the first sequence from the current platform
 * version and adding 1 to it.

Comment 9

[Dave Townsend [:mossop]]

Comment on attachment 448478 [details] [diff] [review]
patch rev1

r+ for the rest

Comment 10

[Robert Strong (they/them - no direct email)]

Attachment: patch with comments addressed for checkin

Carrying forward r+

Comment 11

[Robert Strong (they/them - no direct email)]

Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/0425643a900d

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Marking verified fixed based on a clean landing on trunk. Thanks Rob!