1168737 - make Mac test app find custom runtime

Status: RESOLVED FIXED

Description

[Myk Melez [:myk] [@mykmelez]]

Attachment: WIP: make Mac test app find custom runtime

The Mac app stub currently looks for the runtime by querying the system for an bundle with one of the Firefox bundle IDs.  But custom builds have the same ID as nightly builds, and developers who test custom builds often have a nightly build installed.  So the system sometimes returns the nightly build instead of a custom build when a developer runs tests after building the runtime.

The attached patch forces the test app to use the custom runtime instead of a nightly build by explicitly specifying the path to an "override" runtime to use.

One issue is that the path may change on stabilization branches like Aurora and Beta, where we'd also like to be able to run tests, since the path includes the name of the bundle (Nightly.app), which should be different on those branches.  I'm not yet sure what to do about that, as I don't want to preprocess the TestApp bundle (I'm not sure the build system even supports that).

Comment 1

[Marco Castelluccio [:marco]]

> I'm not yet sure what to do about that, as I don't want to preprocess the TestApp bundle (I'm not sure the build system even supports that).

Could we override the runtime path with a command line option instead of a value in the plist file?

Comment 2

[Myk Melez [:myk] [@mykmelez]]

Attachment: override runtime with command-line argument

(In reply to Marco Castelluccio [:marco] from comment #1)
> Could we override the runtime path with a command line option instead of a
> value in the plist file?

Yes, that's a better idea.  Here's a patch that works in my manual tests, although I haven't been able to confirm that it works with the automated tests because test invocation appears to be broken.

Comment 3

[Marco Castelluccio [:marco]]

Comment on attachment 8615883 [details] [diff] [review]
override runtime with command-line argument

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

> I haven't been able to confirm that it works with the automated tests because test invocation appears to be broken.

Have you filed a bug about this?

Comment 4

[Myk Melez [:myk] [@mykmelez]]

(In reply to Marco Castelluccio [:marco] from comment #3)
> > I haven't been able to confirm that it works with the automated tests
> > because test invocation appears to be broken.
> 
> Have you filed a bug about this?

I hadn't yet, because I wanted to do a bit more investigation.  I've now filed bug 1171987.

Comment 5

[Myk Melez [:myk] [@mykmelez]]

Now that bug 1171987 is resolved, I can run tests, but I don't see the *-runtime* flag being passed through from the Mach commands script to the Mochitest runner script, so this still needs further investigation.

Comment 6

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 8615883 [details] [diff] [review]
override runtime with command-line argument

(In reply to Myk Melez [:myk] [@mykmelez] from comment #5)
> Now that bug 1171987 is resolved, I can run tests, but I don't see the
> *-runtime* flag being passed through from the Mach commands script to the
> Mochitest runner script, so this still needs further investigation.

Hmm, I'm not sure why I didn't see it earlier, since I do see it now.  So this is working as intended.


Requesting review from jmaher on the testing/mochitest/mach_commands.py change.

Comment 7

[Joel Maher ( :jmaher ) (UTC-4)]

Comment on attachment 8615883 [details] [diff] [review]
override runtime with command-line argument

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

::: webapprt/mac/webapprt.mm
@@ +117,5 @@
> +      NSLog(@"Runtime specified with -runtime flag: %@", firefoxPath);
> +    } else {
> +      firefoxPath = PathToWebRT(alternateBinaryID);
> +    }
> +    NSLog(@"Found runtime: %@", firefoxPath);

this nslog could be inside the else clause since for the case where we pass in args, we will print two NSLog messages effectively duplicating it.  Not critical.

Comment 8

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v3: address review nit

(In reply to Joel Maher (:jmaher) from comment #7)
> this nslog could be inside the else clause since for the case where we pass
> in args, we will print two NSLog messages effectively duplicating it.  Not
> critical.

Good point, I've made that change in this updated patch, which I'll push to inbound once it reopens.

Comment 9

[Myk Melez [:myk] [@mykmelez]]

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=cd741d3ae78a

Comment 10

[Myk Melez [:myk] [@mykmelez]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cd741d3ae78a

Comment 11

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-7]

Backed out in:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed1115ca4083

See commit message of backout for failure information.

Comment 12

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v4: make "app update" test use -runtime flag

The failing test needs to use the new -runtime flag to specify the location of its fake runtime directory.

Here's a try run with the changes:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2449d6583501

(They should only affect Mac, where they fix the test; but I scheduled the tryserver to run on Windows and Linux too, since those platforms also executed some of the changed code.)

Comment 13

[Myk Melez [:myk] [@mykmelez]]

The previous try run excluded Mac OS X 10.10, since I didn't explicitly enable it via the trychooser syntax.  But that's where the failure occurred, so here's another try run that explicitly enables 10.10:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d22ee21d933

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b0cbb6e2284b

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b0cbb6e2284b