Closed
Bug 1147031
Opened 8 years ago
Closed 8 years ago
Write mach command for luciddream
Categories
(Testing :: General, defect)
Testing
General
Tracking
(firefox39 fixed, firefox40 fixed)
RESOLVED
FIXED
mozilla40
People
(Reporter: jgriffin, Assigned: ochameau)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 4 obsolete files)
14.66 KB,
patch
|
ochameau
:
review+
|
Details | Diff | Splinter Review |
After luciddream has been moved in-tree (bug 1147029) we should create a mach command for it.
Assignee | ||
Comment 1•8 years ago
|
||
Do you want me to rebase my patch or were you considering a different approach?
Reporter | ||
Comment 2•8 years ago
|
||
Feel free to rebase; I was going to use your patch mostly as-is.
Assignee | ||
Comment 3•8 years ago
|
||
So this patch introduce ./mach luciddream, that can work without any build! i.e. you don't have to build anything, you can just download binaries and pass firefox, b2g-desktop and gaia paths. But it will also use the obdir binary if a mozconfig is given. I also added couple of fixes and improvements to luciddream. - Added --consoles argument to open jsconsole to firefox and b2g-desktop. - Dump firefox stdout to firefox.log (we already do that for b2g-desktop to gecko.log). - set logger in LucidDreamTestCase prevents some exception https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5967b64ed37
Assignee | ||
Comment 4•8 years ago
|
||
Comment on attachment 8586412 [details] [diff] [review] patch v1 Review of attachment 8586412 [details] [diff] [review]: ----------------------------------------------------------------- Jonathan, I don't know exactly who should review such patch, feel free to redirect to whoever should!
Attachment #8586412 -
Flags: review?(ted)
Attachment #8586412 -
Flags: review?(jgriffin)
Reporter | ||
Comment 5•8 years ago
|
||
Comment on attachment 8586412 [details] [diff] [review] patch v1 Review of attachment 8586412 [details] [diff] [review]: ----------------------------------------------------------------- Looking good. The try run failed because these are scheduled only against opt builds; I guess we should get them running for debug as well! ::: testing/luciddream/luciddream/runluciddream.py @@ +102,5 @@ > > > #TODO: make marionette/client/marionette/runtests.py importable so we can > # just use cli from there. A lot of this is copy/paste from that function. > +def main(firefox, b2g_desktop, emulator, emulator_arch, gaia_profile, consoles, manifest, **kwargs): This breaks the ability to run the tests directly from the runner or via mozharness; you'll get e.g.,: TypeError: main() takes exactly 7 arguments (1 given) Most or all of these should be kwargs defaulting to None. @@ +118,5 @@ > 'browser': browser, > 'logger': logger, > } > + if consoles: > + kwargs['app_args'] = ['-jsconsole'] I think we should handle --consoles entirely in the mach command, and not here in the runner, by having the mach command set kwargs['app_args']=['-jsconsole'].
Attachment #8586412 -
Flags: review?(jgriffin) → review-
Assignee | ||
Comment 6•8 years ago
|
||
(In reply to Jonathan Griffin (:jgriffin) from comment #5) > Comment on attachment 8586412 [details] [diff] [review] > > The try run failed because these are scheduled only against opt builds; Oops > I guess we should get them running for debug as well! Doesn't sounds critical for now. > > ::: testing/luciddream/luciddream/runluciddream.py > @@ +102,5 @@ > > > > > > #TODO: make marionette/client/marionette/runtests.py importable so we can > > # just use cli from there. A lot of this is copy/paste from that function. > > +def main(firefox, b2g_desktop, emulator, emulator_arch, gaia_profile, consoles, manifest, **kwargs): > > This breaks the ability to run the tests directly from the runner or via > mozharness; you'll get e.g.,: > > TypeError: main() takes exactly 7 arguments (1 given) > > Most or all of these should be kwargs defaulting to None. I'm not against modifying this code, but how so? Today main() has no argument at all as it fetches the arguments from sys.argv which disallow calling it from another python module! If I move everything to kwargs, I imagine most of main() content is going to move to mach_commands. As I don't think it is reasonable to pass unexpected kwargs to BaseMarionetteTestRunner, mach_commands has to only pass the final fields. It sounds like you want to make the caller directly control LucidDreamTestRunner > BaseMarionetteTestRunner kwargs without any intermediate helper in between.
Reporter | ||
Comment 7•8 years ago
|
||
So, more explicitly, I think you should write: def main(firefox=None, b2g_desktop=None, emulator=None, ...., **kwargs): This allows those args to become optional, and won't affect the kwargs that are passed to Marionette.
Assignee | ||
Comment 8•8 years ago
|
||
Attachment #8586412 -
Attachment is obsolete: true
Attachment #8586412 -
Flags: review?(ted)
Attachment #8587535 -
Flags: review?(jgriffin)
Assignee | ||
Comment 9•8 years ago
|
||
(In reply to Jonathan Griffin (:jgriffin) from comment #7) > So, more explicitly, I think you should write: > > def main(firefox=None, b2g_desktop=None, emulator=None, ...., **kwargs): > > This allows those args to become optional, and won't affect the kwargs that > are passed to Marionette. okok. So I tried to ensure that kwargs can overload everything, also `logger` if you want.
Assignee | ||
Comment 10•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cdc461be997a
Reporter | ||
Comment 11•8 years ago
|
||
Comment on attachment 8587535 [details] [diff] [review] patch v2 Review of attachment 8587535 [details] [diff] [review]: ----------------------------------------------------------------- lgtm, thanks, and this time we have a green try run!
Attachment #8587535 -
Flags: review?(jgriffin) → review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → poirot.alex
Comment 12•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a209d4d37f8f
Keywords: checkin-needed
Comment 13•8 years ago
|
||
Backed out for bustage. https://hg.mozilla.org/integration/mozilla-inbound/rev/e580bfc4a232 https://treeherder.mozilla.org/logviewer.html#?job_id=8539867&repo=mozilla-inbound
Assignee | ||
Comment 14•8 years ago
|
||
Assignee | ||
Comment 15•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=97bfb4e40f04 I forgot to update runluciddream.py regarding its command line interface. I'm not a python expert... so there might be something clever to do in main() instead of just passing each argument manually. I renamed --b2gPath to --emulator-path and --emulator to --emulator-arch. I've let all the other existing argument names as-is. I also tweaked mach arguments to make them short and better patch luciddream conventions. See the other interdiff attachment to see only the new changes.
Attachment #8587535 -
Attachment is obsolete: true
Attachment #8590240 -
Flags: review?(jgriffin)
Reporter | ||
Comment 16•8 years ago
|
||
Comment on attachment 8590240 [details] [diff] [review] patch v3 Review of attachment 8590240 [details] [diff] [review]: ----------------------------------------------------------------- lgtm ::: testing/luciddream/luciddream/runluciddream.py @@ +144,5 @@ > sys.exit(0) > > +def main(): > + args = parse_args(sys.argv[1:]) > + run(args.browser_path, args.b2g_desktop_path, args.emulator_path, args.emulator_arch, args.gaia_profile, args.manifest, None) This could be done more neatly as: run(**vars(args))
Attachment #8590240 -
Flags: review?(jgriffin) → review+
Assignee | ||
Comment 17•8 years ago
|
||
Attachment #8590237 -
Attachment is obsolete: true
Attachment #8590402 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Updated•8 years ago
|
Attachment #8590240 -
Attachment is obsolete: true
Comment 18•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/399083596820
Keywords: checkin-needed
Comment 19•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/399083596820
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Comment 20•8 years ago
|
||
https://hg.mozilla.org/releases/mozilla-release/rev/2e94e80384c1
status-firefox39:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•