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?

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.

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?

(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>

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

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

Going to send this to try

Comment on attachment 480576 [details] [diff] [review] patch in progress Basic approach seems fine to me.

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.

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

Also, this includes tests for bug 600098

Comment on attachment 483282 [details] [diff] [review] patch rev1 seeing a bit of weirdness running mochitest chrome that I need to investigate

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.

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

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

Also verified that the xpcshell tests pass on Thunderbird.

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

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.

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

This is awesome!

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 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?

(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 on attachment 484544 [details] [diff] [review] patch rev5 Looks good

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.

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

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

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

Comment on attachment 496313 [details] [diff] [review] 1.9.2 patch a=LegNeato for 1.9.2.14

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

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

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.

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

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

Backed out of 1.9.2 to investigate if this caused bug 633869

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

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 on attachment 496313 [details] [diff] [review] 1.9.2 patch Approved for 1.9.2.16, a=dveditz for release-drivers

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