make Mac test app find custom runtime

RESOLVED FIXED in Firefox 41

Status

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: myk, Assigned: myk)

Tracking

Trunk
Firefox 41
Unspecified
macOS
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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).
Attachment #8611063 - Flags: feedback?(mar.castelluccio)
> 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?
Attachment #8611063 - Flags: feedback?(mar.castelluccio) → feedback+
(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.
Attachment #8611063 - Attachment is obsolete: true
Attachment #8615883 - Flags: review?(mar.castelluccio)
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?
Attachment #8615883 - Flags: review?(mar.castelluccio) → review+
(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.
Depends on: 1171987
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 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.
Attachment #8615883 - Flags: review?(jmaher)
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.
Attachment #8615883 - Flags: review?(jmaher) → review+
Posted patch patch v3: address review nit (obsolete) — Splinter Review
(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.
Backed out in:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed1115ca4083

See commit message of backout for failure information.
Flags: needinfo?(myk)
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.)
Attachment #8615883 - Attachment is obsolete: true
Attachment #8624795 - Attachment is obsolete: true
Flags: needinfo?(myk)
Attachment #8625366 - Flags: review?(mar.castelluccio)
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
Attachment #8625366 - Flags: review?(mar.castelluccio) → review+
https://hg.mozilla.org/mozilla-central/rev/b0cbb6e2284b
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Duplicate of this bug: 914754
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.