Closed
Bug 1168737
Opened 7 years ago
Closed 7 years ago
make Mac test app find custom runtime
Categories
(Firefox Graveyard :: Webapp Runtime, defect)
Tracking
(firefox41 fixed)
RESOLVED
FIXED
Firefox 41
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
People
(Reporter: myk, Assigned: myk)
References
Details
Attachments
(1 file, 3 obsolete files)
9.19 KB,
patch
|
marco
:
review+
|
Details | Diff | Splinter Review |
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)
Comment 1•7 years ago
|
||
> 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?
Updated•7 years ago
|
Attachment #8611063 -
Flags: feedback?(mar.castelluccio) → feedback+
Assignee | ||
Comment 2•7 years ago
|
||
(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 3•7 years ago
|
||
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+
Assignee | ||
Comment 4•7 years ago
|
||
(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.
Assignee | ||
Comment 5•7 years ago
|
||
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.
Assignee | ||
Comment 6•7 years ago
|
||
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 7•7 years ago
|
||
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+
Assignee | ||
Comment 8•7 years ago
|
||
(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.
Assignee | ||
Comment 9•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=cd741d3ae78a
Assignee | ||
Comment 10•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd741d3ae78a
Comment 11•7 years ago
|
||
Backed out in: https://hg.mozilla.org/integration/mozilla-inbound/rev/ed1115ca4083 See commit message of backout for failure information.
Flags: needinfo?(myk)
Assignee | ||
Comment 12•7 years ago
|
||
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)
Assignee | ||
Comment 13•7 years ago
|
||
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
Updated•7 years ago
|
Attachment #8625366 -
Flags: review?(mar.castelluccio) → review+
Comment 15•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b0cbb6e2284b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Updated•6 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•