916512 - [mozrunner] mozrunner should use -foreground at least for mac

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Jeff Hammel]

automation.py.in does:

http://hg.mozilla.org/mozilla-central/file/655ac375b1c7/build/automation.py.in#l794

mozrunner intends to, but it doesn't:

https://github.com/mozilla/mozbase/blob/master/mozrunner/mozrunner/local.py#L104

It should.  See also
https://bugzilla.mozilla.org/show_bug.cgi?id=746243#c87

Comment 1

[Jeff Hammel]

Attachment: bug-916512.diff

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 805253 [details] [diff] [review]
bug-916512.diff

Review of attachment 805253 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozrunner/mozrunner/local.py
@@ +110,2 @@
>              self.cmdargs = _cmdargs
>              self.cmdargs.append('-foreground')

So what's with that line? Is it still necessary?

Comment 3

[Andrew Halberstadt [:ahal]]

Comment on attachment 805253 [details] [diff] [review]
bug-916512.diff

Review of attachment 805253 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozrunner/mozrunner/local.py
@@ +110,3 @@
>              self.cmdargs = _cmdargs
>              self.cmdargs.append('-foreground')
> +        if mozinfo.isMac and '-foreground' not in self.cmdargs:

Could just make this 'elif mozinfo.isMac' and drop the '-foreground' clause, though either way works :)

Comment 4

[Jeff Hammel]

pushed: https://github.com/mozilla/mozbase/commit/630fccd22472495c9ccca256ac1517601753f5b1 

I elected to keep the added logic separate from the preceding indented block since I'm not sure the status of bug 625614 (outside of outstanding) and the added logic, AFAIK, can be kept even if bug 625614 is resolved.  In my opinion the extra line of code is worth it, though I also don't care that much.

Comment 5

[Dave Hunt [:davehunt] [he/him] ⌚BST]

This has caused the Gaia UI tests on b2gdesktop to steal focus for every test, which is rather annoying when running them locally. Is there a way we can make this optional? It's not a requirement for these tests as we use focusmanager.testmode where focus is required.

Comment 6

[Jeff Hammel]

(In reply to Dave Hunt (:davehunt) from comment #5)
> This has caused the Gaia UI tests on b2gdesktop to steal focus for every
> test, which is rather annoying when running them locally. Is there a way we
> can make this optional? It's not a requirement for these tests as we use
> focusmanager.testmode where focus is required.

AFAIK, the -foreground is for mac only.  I assume you are on a Mac?  I'd be fine with a subclass that didn't have this behaviour.

Comment 7

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Yes, I'm using Mac. Do you know why we need to run with -foreground on Mac? Would enabling the focusmanager.testmode preference be a suitable alternative? If we're to add a new subclass, could you suggest an approach... perhaps a LocalBackgroundRunner that sets a flag to force running in the background?

Comment 8

[(not currently active) Ted Mielczarek]

If you don't run with -foreground on Mac the app doesn't start in the foreground. With focusmanager.testmode I'm not sure this is important for tests anymore.

Comment 9

[Jeff Hammel]

(In reply to Dave Hunt (:davehunt) from comment #7)
> Yes, I'm using Mac. Do you know why we need to run with -foreground on Mac?
> Would enabling the focusmanager.testmode preference be a suitable
> alternative? If we're to add a new subclass, could you suggest an
> approach... perhaps a LocalBackgroundRunner that sets a flag to force
> running in the background?

Pretty much Ted said. For a subclass, say LocalBackgroundRunner, I'd A. break out the '-foreground' appending logic to its own, say, function (def set_arguments()) and B. have the subclass not do anything with the method.

I'm also fine discarding this logic if unneeded, but IIRC there were problems when I quite accidentally did so.

Comment 10

[(no longer active)]

"me too" on the focus issues in comment 5.