Closed Bug 1147031 Opened 5 years ago Closed 5 years ago

Write mach command for luciddream

Categories

(Testing :: General, defect)

defect
Not set

Tracking

(firefox39 fixed, firefox40 fixed)

RESOLVED FIXED
mozilla40
Tracking Status
firefox39 --- fixed
firefox40 --- fixed

People

(Reporter: jgriffin, Assigned: ochameau)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 4 obsolete files)

After luciddream has been moved in-tree (bug 1147029) we should create a mach command for it.
Do you want me to rebase my patch or were you considering a different approach?
Feel free to rebase; I was going to use your patch mostly as-is.
Attached patch patch v1 (obsolete) — Splinter Review
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
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)
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-
(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.
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.
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #8586412 - Attachment is obsolete: true
Attachment #8586412 - Flags: review?(ted)
Attachment #8587535 - Flags: review?(jgriffin)
(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.
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+
Keywords: checkin-needed
Assignee: nobody → poirot.alex
Attached patch interdiff (obsolete) — Splinter Review
Attached patch patch v3 (obsolete) — Splinter Review
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)
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+
Attached patch patch v4Splinter Review
Attachment #8590237 - Attachment is obsolete: true
Attachment #8590402 - Flags: review+
Keywords: checkin-needed
Attachment #8590240 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/399083596820
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.