601518 - Need updater tests to cover nsUpdateDriver.cpp code.

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Justin Dolske [:Dolske]]

Sure would have been nice if bug 601469 had been prevented by having a test catch that failure. :-(

Seems like we would benefit from having a higher-fidelity test that actually invokes this code. Perhaps a test that copies the whole appdir (!), stages a simple MAR (eg, just adding a new file), launches the clone'd app, and then checks to see if the MAR was applied in the clone'd dir? I'm sure there are complications (like UAC), maybe this just isn't practical to test automatically?

Comment 1

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

Ideally, we would get mozmill-restart tests and run the current mozmill app update tests using the previous nightly updating with a partial mar to the current nightly and a nightly from a couple of days prior updating with a complete mar to the current nightly. That likely won't be happening soon and I have a couple of ideas of how this could be tested though they are a tad hackish to say the least.

Comment 2

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

I've got a bare bones xpcshell test that launches the Firefox, Thunderbird, or SeaMonkey main bbinary if present with the following command line args:
-silent -no-remote -P <path to a temp profile>

I've tested with one of the test mars and so far all appears to be good.

Any objections to taking this approach?

Comment 3

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

(In reply to comment #2)
> I've got a bare bones xpcshell test that launches the Firefox, Thunderbird, or
> SeaMonkey main bbinary if present with the following command line args:
> -silent -no-remote -P <path to a temp profile>
should be
-silent -no-remote -Profile <path to a temp profile>

Comment 4

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

I've also verified this works for Firefox, Thunderbird, and SeaMonkey... I think this will be the way to go.

Comment 5

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

Attachment: patch in progress

Definitely a tad hackish but it gets the job done. Still needs work but I'd like some feedback about the general approach.

Comment 6

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

Attachment: patch rev1

Going to send this to try

Comment 7

[Justin Dolske [:Dolske]]

Comment on attachment 480576 [details] [diff] [review]
patch in progress

Basic approach seems fine to me.

Comment 8

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

Attachment: patch in progress

Comment 9

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

Attachment: patch in progress - almost there

Comment 10

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

Attachment: patch in progress - almost almost there ;)

Comment 11

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

Attachment: patch rev1

Sending this to try

couple of notes:
I had an SSL tunnel failure during mochitest chrome on one of the try runs that the subsequent tests didn't cleanly recover from which I fixed (most of the changes to utils.js).
Lots of logging cleanup.
The remainder I think I've discussed with you in person.

Comment 12

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

btw: I just pushed this to try and will report findings when they have completed.

Comment 13

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

Also, this includes tests for bug 600098

Comment 14

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

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

seeing a bit of weirdness running mochitest chrome that I need to investigate

Comment 15

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

xpcshell for the http server is launched with -v 170 and I am quite certain I am using features greater than JavaScript 1.7 which is likely the cause of the weirdness. I use a shared file between the sjs and the tests where a ton of the shared file isn't needed by the sjs... I'm going to try to refactor it so the sjs gets only what it needs.

Comment 16

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

For reference xpcshell for the http server is asserting though not at the same point during each run. I've also given cdleary a heads up though it appears to be due to my rewrite and not a regression.
Assertion failure: OptionsHasXML(options) == VersionHasXML(version), at /builds/slave/tryserver-linux64-debug/build/js/src/jsapi.cpp:1032

Comment 17

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

Attachment: patch rev2

Comment 18

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

Attachment: patch rev3

This has passed several try server runs and I've also verified this passes with SeaMonkey Win32.

Comment 19

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

Also verified that the xpcshell tests pass on Thunderbird.

Comment 20

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

The patch adds tests for bug 600098 and bug 601469 so adding dependencies and it also fixes the remainder of bug 596788

Comment 21

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

note to Dave and self: to handle the case where the binary is in use on Windows I could just use a copy of it instead.

Comment 22

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

Attachment: patch rev4

Use a copy of the app exe on Windows and use a custom updater.ini to avoid launching an app provided post update executable.

Comment 23

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

This is awesome!

Comment 24

[Dave Townsend [:mossop]]

Comment on attachment 484060 [details] [diff] [review]
patch rev4

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

>+function runTestDefaultWaitForWindowClosed() {
>+  gCloseWindowTimeoutCounter++;
>+  if (gCloseWindowTimeoutCounter > CLOSE_WINDOW_TIMEOUT_MAXCOUNT) {
>+    try {
>+      finishTest();
>+    }
>+    catch (e) {
>+      finishTestDefault();
>+    }
>+    return;
>+  }
>+
>+  // The update window should not be open at this time. If it is the call to
>+  // |closeUpdateWindow| will close it and cause the test to fail.
>+  if (closeUpdateWindow()) {

closeUpdateWindow doesn't return anything.

>+    SimpleTest.executeSoon(waitForWindowClosedOnStart);

As I said in person, this is wrong.

>+function finishTestDefaultWaitForWindowClosed() {
>+  gCloseWindowTimeoutCounter++;
>+  if (gCloseWindowTimeoutCounter > CLOSE_WINDOW_TIMEOUT_MAXCOUNT) {
>+    SimpleTest.finish();
>+    return;
>+  }
>+
>+  // The update window should not be open at this time. If it is the call to
>+  // |closeUpdateWindow| will close it and cause the test to fail.
>+  if (closeUpdateWindow()) {

closeUpdateWindow doesn't return anything.

>diff --git a/toolkit/mozapps/update/test/unit/test_0010_general.js b/toolkit/mozapps/update/test/unit/test_0010_general.js
>--- a/toolkit/mozapps/update/test/unit/test_0010_general.js
>+++ b/toolkit/mozapps/update/test/unit/test_0010_general.js
>@@ -34,28 +34,36 @@
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK *****
>  */
> 
> /* General Update Service Tests */
> 
> function run_test() {
>+  do_test_pending();

Why is do_test_pending needed here?

Comment 25

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

Attachment: patch updated to comments so far

Comment 26

[Dave Townsend [:mossop]]

Comment on attachment 484060 [details] [diff] [review]
patch rev4

Really awesome work, such a large patch and all I can see is mainly some questions to be answered. Next patch should be a quick r+ assuming interdiff saves me

>diff --git a/toolkit/mozapps/update/test/unit/test_0200_app_launch_apply_update.js b/toolkit/mozapps/update/test/unit/test_0200_app_launch_apply_update.js
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/mozapps/update/test/unit/test_0200_app_launch_apply_update.js
>@@ -0,0 +1,680 @@
>+/* Any copyright is dedicated to the Public Domain.
>+ * http://creativecommons.org/publicdomain/zero/1.0/
>+ */
>+
>+/* Test applying an update by staging an update and launching an application */
>+
>+// Note: The application shell scripts for launching the application work on all
>+// platforms that provide a launch shell script except for Mac OS X 10.5 which
>+// is why this test uses the binaries to launch the application.
>+const BIN_NAMES = [ "firefox.exe",     "firefox-bin",
>+                    "seamonkey.exe",   "seamonkey-bin",
>+                    "thunderbird.exe", "thunderbird-bin" ];

Can we preprocess this into the head_updates.js.in or something instead?

>+// A shell script is used to get the OS version due to nsSystemInfo not
>+// returning the actual OS version. It is possible to get the actual OS version
>+// using ctypes but it would be more complicated than using a shell script.
>+XPCOMUtils.defineLazyGetter(this, "gIsLessThanMacOSX_10_6", function test_gMacVer() {
>+  if (!IS_MACOSX) {
>+    return false;
>+  }
>+
>+  let [versionScript, versionFile] = getVersionScriptAndFile();
>+  // Precreate the script with executable permissions
>+  versionScript.create(AUS_Ci.nsILocalFile.NORMAL_FILE_TYPE, PERMS_DIRECTORY);
>+  let scriptContents = "#! /bin/sh\nsw_vers -productVersion >> " + versionFile.path;
>+  writeFile(versionScript, scriptContents);
>+  logTestInfo("created " + versionScript.path + " shell script containing:\n" +
>+              scriptContents);
>+
>+  let versionScriptPath = versionScript.path;
>+  if (/ /.test(versionScriptPath)) {
>+    versionScriptPath = '"' + versionScriptPath + '"';
>+  }
>+
>+
>+  let launchBin = getLaunchBin();
>+  let args = [versionScriptPath];
>+  let process = AUS_Cc["@mozilla.org/process/util;1"].
>+                createInstance(AUS_Ci.nsIProcess);
>+  process.init(launchBin);
>+  process.run(true, args, args.length);
>+  if (process.exitValue != 0) {
>+    do_throw("Version script exited with " + process.exitValue + "... unable " +
>+             "to get Mac OS X version!");
>+  }
>+
>+  let version = readFile(versionFile).split("\n")[0];
>+  logTestInfo("executing on Mac OS X verssion " + version);

Can't you use ctypes to pull this from the OS directly?

Comment 27

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

Attachment: patch rev5

(In reply to comment #24)
>...
> Why is do_test_pending needed here?
When I tested with do_register_cleanup the cleanup function wasn't called without it.

(In reply to comment #26)
>...
> Can't you use ctypes to pull this from the OS directly?
Since I am already having to rely on a shell script to launch the app and all of the other complexities of this test I don't want to add the additional complexity of using ctypes on top of everything else at this time. I'll revisit it when there is more time to do so.

Everything else addressed

Comment 28

[Dave Townsend [:mossop]]

Comment on attachment 484544 [details] [diff] [review]
patch rev5

Looks good

Comment 29

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

Attachment: patch as checked in

I had to make a minor change since I need to use the binary files on Mac OS X and Linux which requires adding -bin to the binary.

Comment 30

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

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

Comment 31

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

This caused an orange - bug 605767. I'll look into it tomorrow

Comment 32

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

Attachment: 1.9.2 patch

I'm backporting the tests on trunk so there are some tests for Bug 600362, etc.

Comment 33

[LegNeato]

Comment on attachment 496313 [details] [diff] [review]
1.9.2 patch

a=LegNeato for 1.9.2.14

Comment 34

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

Attachment: 1.9.2 tests

Still need to verify these work properly on Mac OS X.

Comment 35

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

Attachment: 1.9.2 tests

Comment 36

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

Pushed to mozilla-1.9.2
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/91d198f81f1e
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/080844eedd1b

Comment 37

[Al Billings [:abillings - ex-MoCo]]

Looking at the test logs, the only thing I see that isn't an obvious pass for 1.9.2. is this message on win32:

9486 INFO TEST-PASS | chrome://mochikit/content/chrome/toolkit/mozapps/update/test/chrome/test_0091_installed.xul | Checking currentPage.pageid equals installed in pageshow - "installed" should equal "installed"
*** Failed to format string whatsNewURL in bundle: chrome://branding/locale/brand.properties

Is this a test failure? It doesn't happen on OS X or Linux.

Comment 38

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

That isn't a test failure and doesn't cause any problems in the client. It is covered by bug 546609.

Comment 39

[Al Billings [:abillings - ex-MoCo]]

All right. Marking verified for 1.9.2 based on tests then.

Comment 40

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

Backed out of 1.9.2 to investigate if this caused bug 633869

Comment 41

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

Attachment: Updated 1.9.2 patch

Comment 42

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

Attachment: updated 1.9.2 test patch

attachment #496313 [details] [diff] [review] still applies on 1.9.2 so I only need to update the test patch

Comment 43

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

Comment on attachment 496313 [details] [diff] [review]
1.9.2 patch

Definitely want this for 1.9.2 to test updates launched by applications.

Comment 44

[Daniel Veditz [:dveditz]]

Comment on attachment 496313 [details] [diff] [review]
1.9.2 patch

Approved for 1.9.2.16, a=dveditz for release-drivers

Comment 45

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

Pushed to mozilla-1.9.2
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/2394ec5d026f
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/1983c81ef406