1147031 - Write mach command for luciddream

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Jonathan Griffin (:jgriffin)]

After luciddream has been moved in-tree (bug 1147029) we should create a mach command for it.

Comment 1

[Alexandre Poirot [:ochameau]]

Do you want me to rebase my patch or were you considering a different approach?

Comment 2

[Jonathan Griffin (:jgriffin)]

Feel free to rebase; I was going to use your patch mostly as-is.

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: patch v1

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 4

[Alexandre Poirot [:ochameau]]

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!

Comment 5

[Jonathan Griffin (: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'].

Comment 6

[Alexandre Poirot [:ochameau]]

(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.

Comment 7

[Jonathan Griffin (:jgriffin)]

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.

Comment 8

[Alexandre Poirot [:ochameau]]

Attachment: patch v2

Comment 9

[Alexandre Poirot [:ochameau]]

(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 10

[Alexandre Poirot [:ochameau]]

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

Comment 11

[Jonathan Griffin (:jgriffin)]

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!

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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

Comment 14

[Alexandre Poirot [:ochameau]]

Attachment: interdiff

Comment 15

[Alexandre Poirot [:ochameau]]

Attachment: patch v3

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.

Comment 16

[Jonathan Griffin (: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))

Comment 17

[Alexandre Poirot [:ochameau]]

Attachment: patch v4

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/2e94e80384c1