Make Robocop disable keyguard and take wakelock (force screen on) when running locally

RESOLVED FIXED in Firefox 47

Status

()

Firefox for Android
Testing
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: nalexander, Assigned: nalexander)

Tracking

Trunk
Firefox 47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 affected, firefox47 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

3 years ago
Right now, Robocop fails early if the screen is off.  It's better than failing silently later, but still frustrating.

This ticket tracks disabling any keyguard and taking a wakelock to force the screen on while tests are running, at least when running locally.  (Locally here means "not in automation".)

I see no reason to not do this in automation, but earlier discussion (https://bugzilla.mozilla.org/show_bug.cgi?id=929654#c27 and https://bugzilla.mozilla.org/show_bug.cgi?id=929654#c28) suggested alternate approaches that are not useful for local testers.
(Assignee)

Updated

3 years ago
Assignee: nobody → nalexander
Status: NEW → ASSIGNED
(Assignee)

Comment 2

3 years ago
Created attachment 8612639 [details]
MozReview Request: Bug 1168175 - Part 1: Always wait for Gecko:Ready. r=mcomella,gbrown

Bug 1168175 - Part 1: Always wait for Gecko:Ready. r=mcomella,gbrown

This patch looks terrible but is mechanical.  It's not harmful to wait
for Gecko to start even if it's not needed (other than, perhaps,
bumping the rate of intermittent failures to receive Gecko:Ready).  My
assertion is that the dividing line between "Robocop things" and
"other things, i.e., purely JUnit things" should be in the class
hierarchy, and that it should be *above* BaseRobocopTest.  So that
tests of content providers, for example, should migrate to require
*none* of the existing Robocop baggage, and therefore will eventually
not feel this patch at all.
Attachment #8612639 - Flags: review?(michael.l.comella)
Attachment #8612639 - Flags: review?(gbrown)
(Assignee)

Comment 3

3 years ago
Created attachment 8612640 [details]
MozReview Request: Bug 1168175 - Part 2: Turn screen on before running each test. r=mcomella,gbrown

Bug 1168175 - Part 2: Turn screen on before running each test. r=mcomella,gbrown
Attachment #8612640 - Flags: review?(michael.l.comella)
Attachment #8612640 - Flags: review?(gbrown)
Comment on attachment 8612639 [details]
MozReview Request: Bug 1168175 - Part 1: Always wait for Gecko:Ready. r=mcomella,gbrown

https://reviewboard.mozilla.org/r/9619/#review8451

I like the idea in general.

Which tests were not waiting on Gecko:Ready before?

Holding off on r+ for testJarReader only.

::: mobile/android/tests/browser/robocop/helpers/GeckoHelper.java:27
(Diff revision 1)
> -        sActivity = context.getActivity();
> +    public static void init(Actions actions, long millisecondsToWaitForEvents) {

Passing millisecondsToWaitForEvents seems clumsy -- it is a minor part of GeckoHelper overall. I would prefer to see GECKO_READY_WAIT_MS moved into GeckoHelper.

::: mobile/android/tests/browser/robocop/testAboutHomePageNavigation.java:22
(Diff revision 1)
> -        GeckoHelper.blockForDelayedStartup();
> +        GeckoHelper.getInstance().blockForDelayedStartup();

I am a little worried about blockForDelayedStartup() missing its event because it was processed while blocked waiting for Gecko:Ready. Is that possible?

::: mobile/android/tests/browser/robocop/testAdobeFlash.java:30
(Diff revision 1)
>              setPreferenceAndWaitForChange(jsonPref);

This is an interesting case where a test clearly wants to run some code before Gecko:Ready. I'm pretty sure it is not necessary in this case, but in general, how should that be dealt with? Override setUp()?

::: mobile/android/tests/browser/robocop/testBrowserProviderPerf.java
(Diff revision 1)
> -    protected Intent createActivityIntent() {

Why is this being removed? Is it related to the Gecko:Ready changes?

::: mobile/android/tests/browser/robocop/testJarReader.java
(Diff revision 1)
> -        blockForGeckoReady();

This test seems to be crashing in your try run, on 2.3. I'm not sure why. Obviously this moves the Gecko:Ready wait from the end of the test to the beginning, but I don't see how that is significant.
Attachment #8612639 - Flags: review?(gbrown)
(Assignee)

Comment 5

3 years ago
(In reply to Geoff Brown [:gbrown] from comment #4)
> Comment on attachment 8612639 [details]
> MozReview Request: Bug 1168175 - Part 1: Always wait for Gecko:Ready.
> r=mcomella,gbrown
> 
> https://reviewboard.mozilla.org/r/9619/#review8451
> 
> I like the idea in general.
> 
> Which tests were not waiting on Gecko:Ready before?
> 
> Holding off on r+ for testJarReader only.
> 
> ::: mobile/android/tests/browser/robocop/helpers/GeckoHelper.java:27
> (Diff revision 1)
> > -        sActivity = context.getActivity();
> > +    public static void init(Actions actions, long millisecondsToWaitForEvents) {
> 
> Passing millisecondsToWaitForEvents seems clumsy -- it is a minor part of
> GeckoHelper overall. I would prefer to see GECKO_READY_WAIT_MS moved into
> GeckoHelper.

Sure.  I don't care where it lives.

> ::: mobile/android/tests/browser/robocop/testAboutHomePageNavigation.java:22
> (Diff revision 1)
> > -        GeckoHelper.blockForDelayedStartup();
> > +        GeckoHelper.getInstance().blockForDelayedStartup();
> 
> I am a little worried about blockForDelayedStartup() missing its event
> because it was processed while blocked waiting for Gecko:Ready. Is that
> possible?

It's possible but it was already possible.  (None of this messaging is particularly robust.)  I'll add a part 3, which will:
* always register a Gecko:DelayedStartup listener;
* record the message with a CountdownLatch;
* block on the CountdownLatch as appropriate.

> ::: mobile/android/tests/browser/robocop/testAdobeFlash.java:30
> (Diff revision 1)
> >              setPreferenceAndWaitForChange(jsonPref);
> 
> This is an interesting case where a test clearly wants to run some code
> before Gecko:Ready. I'm pretty sure it is not necessary in this case, but in
> general, how should that be dealt with? Override setUp()?

Hmm.  Tricky, because setUp executes before every test, not just the one you want to do something before.  Although Robocop expects exactly one function named like test*.  I guess yes, overriding setUp() is reasonable.

> ::: mobile/android/tests/browser/robocop/testBrowserProviderPerf.java
> (Diff revision 1)
> > -    protected Intent createActivityIntent() {
> 
> Why is this being removed? Is it related to the Gecko:Ready changes?

Sort of.  I'm just trying to standardize here, although I realize now that this is a Talos test and this could inject instability into the startup process.  If I recall Talos correctly, though, it's not the total test run that matters, it's timing events that happen *during* the test run.  I guess the browser will be running.  This requires more thought, because if the browser is running we may get interference with the database writes, etc.

> ::: mobile/android/tests/browser/robocop/testJarReader.java
> (Diff revision 1)
> > -        blockForGeckoReady();
> 
> This test seems to be crashing in your try run, on 2.3. I'm not sure why.
> Obviously this moves the Gecko:Ready wait from the end of the test to the
> beginning, but I don't see how that is significant.

Weird.  I see a NoSuchMethodError issue; I'll investigate.  Probably an API version thing.

22:39:04     INFO -  05-28 22:37:39.779 I/Robocop (  490): {"message":"","time":1432877859771,"source":"robocop","status":"PASS","test":"testJarReader","thread":null,"subtest":"Robocop tests need the test device screen to be powered on.","action":"test_status","pid":null}
22:39:04     INFO -  05-28 22:37:39.908 D/GeckoHealthRec(  490): Done initializing profile cache. Beginning storage init.
22:39:04     INFO -  05-28 22:37:40.009 I/Robocop (  490): {"time":1432877859987,"source":"robocop","status":"PASS","test":"testJarReader","thread":null,"subtest":"Illegal characters are escaped away.","action":"test_status","pid":null}
22:39:04     INFO -  05-28 22:37:40.040 I/Robocop (  490): {"time":1432877860026,"source":"robocop","status":"PASS","test":"testJarReader","thread":null,"subtest":"Path characters aren't escaped.","action":"test_status","pid":null}
22:39:04     INFO -  05-28 22:37:40.078 I/Robocop (  490): {"message":"finished in 52609ms","time":1432877860079,"source":"robocop","status":"OK","test":"testJarReader","thread":null,"action":"test_end","pid":null}
22:39:04     INFO -  05-28 22:37:40.122 I/Robocop (  490): {"action":"log","message":"TEST-START | Shutdown","time":1432877860113,"pid":null,"level":"info","source":"robocop","thread":null}
22:39:04     INFO -  05-28 22:37:40.142 I/Robocop (  490): {"action":"log","message":"Passed: 4","time":1432877860135,"pid":null,"level":"info","source":"robocop","thread":null}
22:39:04     INFO -  05-28 22:37:40.218 I/Robocop (  490): {"action":"log","message":"Failed: 0","time":1432877860175,"pid":null,"level":"info","source":"robocop","thread":null}
22:39:04     INFO -  05-28 22:37:40.218 I/Robocop (  490): {"action":"log","message":"Todo: 0","time":1432877860221,"pid":null,"level":"info","source":"robocop","thread":null}
22:39:04     INFO -  05-28 22:37:40.239 I/Robocop (  490): {"action":"log","message":"SimpleTest FINISHED","time":1432877860228,"pid":null,"level":"info","source":"robocop","thread":null}
22:39:04     INFO -  05-28 22:37:40.239 W/System.err(  490): java.lang.NoSuchMethodError: android.os.Bundle.getString
22:39:04     INFO -  05-28 22:37:40.239 W/System.err(  490): 	at org.mozilla.gecko.tests.BaseRobocopTest.tearDown(BaseRobocopTest.java:229)
22:39:04     INFO -  05-28 22:37:40.249 W/System.err(  490): 	at junit.framework.TestCase.runBare(TestCase.java:130)
22:39:04     INFO -  05-28 22:37:40.249 W/System.err(  490): 	at junit.framework.TestResult$1.protect(TestResult.java:106)
22:39:04     INFO -  05-28 22:37:40.249 W/System.err(  490): 	at junit.framework.TestResult.runProtected(TestResult.java:124)
22:39:04     INFO -  05-28 22:37:40.249 W/System.err(  490): 	at junit.framework.TestResult.run(TestResult.java:109)
22:39:04     INFO -  05-28 22:37:40.258 W/System.err(  490): 	at junit.framework.TestCase.run(TestCase.java:118)
22:39:04     INFO -  05-28 22:37:40.258 W/System.err(  490): 	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:169)
22:39:04     INFO -  05-28 22:37:40.268 W/System.err(  490): 	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:154)
22:39:04     INFO -  05-28 22:37:40.268 W/System.err(  490): 	at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:529)
22:39:04     INFO -  05-28 22:37:40.268 W/System.err(  490): 	at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1448)
2
(Assignee)

Comment 6

3 years ago
(In reply to Nick Alexander :nalexander from comment #5)

> 22:39:04     INFO -  05-28 22:37:40.239 W/System.err(  490):
> java.lang.NoSuchMethodError: android.os.Bundle.getString


Definitely an API version thing: http://stackoverflow.com/a/8916617.
Comment on attachment 8612639 [details]
MozReview Request: Bug 1168175 - Part 1: Always wait for Gecko:Ready. r=mcomella,gbrown

https://reviewboard.mozilla.org/r/9619/#review8487

Do we know how much of an impact this will make on test times? What percentage of our tests were using BlockForReady anyway?

::: mobile/android/tests/browser/robocop/helpers/GeckoHelper.java:39
(Diff revision 1)
> +    public void blockForGeckoReady() {

I'd prefer these methods to be static and access the static instance - it is consistent with how our other helpers do testing and is less verbose (although is less flexible but I don't see that being an issue in the future).

It'd probably be a good idea to add a "isInitialized" assertion so non-sensical NullPointers don't happen down the line.
Attachment #8612639 - Flags: review?(michael.l.comella) → review+
(Assignee)

Comment 8

3 years ago
https://reviewboard.mozilla.org/r/9619/#review8489

> I'd prefer these methods to be static and access the static instance - it is consistent with how our other helpers do testing and is less verbose (although is less flexible but I don't see that being an issue in the future).
> 
> It'd probably be a good idea to add a "isInitialized" assertion so non-sensical NullPointers don't happen down the line.

I disagree with this static approach pretty strongly, but gbrown asked to fold the timeout constant into the helper so I'll consider it.  In general, static causes problems.  It's frustrating that we have two trees of tests, BaseTest and UITest, that basically do the same things... but are hard to unify, largely because of these static helpers.  If we could just provide instances as appropriate, life would be better.
(In reply to Nick Alexander :nalexander from comment #5)
> > > -    protected Intent createActivityIntent() {
> > 
> > Why is this being removed? Is it related to the Gecko:Ready changes?
> 
> Sort of.  I'm just trying to standardize here, 

I remember this being added to avoid a timing issue that caused something like a NullPointerException - I'm surprised this works. Is this test disabled?

If you could get it working without this work-around then WFM.
(In reply to Nick Alexander :nalexander from comment #8)
> I disagree with this static approach pretty strongly, but gbrown asked to
> fold the timeout constant into the helper so I'll consider it.  In general,
> static causes problems.  It's frustrating that we have two trees of tests,
> BaseTest and UITest, that basically do the same things... but are hard to
> unify, largely because of these static helpers.  If we could just provide
> instances as appropriate, life would be better.

I've never thought it of this way - I think I was looking to make things as simple as possible when I initially wrote it and the methods at that time didn't require the use of an instance. I'd be for making these non-static and adding some instance references to BaseRobocopTest. File?
Comment on attachment 8612640 [details]
MozReview Request: Bug 1168175 - Part 2: Turn screen on before running each test. r=mcomella,gbrown

https://reviewboard.mozilla.org/r/9621/#review8495

::: mobile/android/tests/browser/robocop/BaseRobocopTest.java:212
(Diff revision 1)
> +        if (wakeLock != null) {
> +            wakeLock.release();
> +        }
> +

Shouldn't you also reenableKeyguard()?
Attachment #8612640 - Flags: review?(gbrown) → review+
Comment on attachment 8612640 [details]
MozReview Request: Bug 1168175 - Part 2: Turn screen on before running each test. r=mcomella,gbrown

https://reviewboard.mozilla.org/r/9621/#review8491

::: mobile/android/tests/browser/robocop/BaseRobocopTest.java:196
(Diff revision 1)
> +        final KeyguardManager.KeyguardLock kl = km.newKeyguardLock("RobocopKeyguardLock");

This seems to be deprecated - any reason you didn't use the Window flags (API 5)?

See the docs: https://developer.android.com/reference/android/app/KeyguardManager.KeyguardLock.html

::: mobile/android/tests/browser/robocop/BaseRobocopTest.java:197
(Diff revision 1)
> +        kl.disableKeyguard();

If you continue with this API, this needs to be re-enabled - tearDown?
Attachment #8612640 - Flags: review?(michael.l.comella)
Comment on attachment 8612640 [details]
MozReview Request: Bug 1168175 - Part 2: Turn screen on before running each test. r=mcomella,gbrown

https://reviewboard.mozilla.org/r/9621/#review8501

Ship It!
Attachment #8612640 - Flags: review+
(In reply to Nick Alexander :nalexander from comment #5)
> (In reply to Geoff Brown [:gbrown] from comment #4)
> > ::: mobile/android/tests/browser/robocop/testBrowserProviderPerf.java
> > (Diff revision 1)
> > > -    protected Intent createActivityIntent() {
> > 
> > Why is this being removed? Is it related to the Gecko:Ready changes?
> 
> Sort of.  I'm just trying to standardize here, although I realize now that
> this is a Talos test and this could inject instability into the startup
> process.  If I recall Talos correctly, though, it's not the total test run
> that matters, it's timing events that happen *during* the test run.  I guess
> the browser will be running.  This requires more thought, because if the
> browser is running we may get interference with the database writes, etc.

I realized that this test is not running: tprovider was disabled in bug 1141656.
(Assignee)

Comment 15

3 years ago
https://reviewboard.mozilla.org/r/9621/#review8507

> This seems to be deprecated - any reason you didn't use the Window flags (API 5)?
> 
> See the docs: https://developer.android.com/reference/android/app/KeyguardManager.KeyguardLock.html

Yeah, sadly it doesn't work.  An earlier patch does this, and it's not enough to actually keep the screen on during the activity.
(Assignee)

Comment 16

3 years ago
https://reviewboard.mozilla.org/r/9621/#review8505

> Shouldn't you also reenableKeyguard()?

I guess so.  It's not clear if process death re-instates the keyguard; locally, it appears to.
(Assignee)

Updated

2 years ago
Attachment #8612639 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8612640 - Attachment is obsolete: true
(Assignee)

Comment 18

2 years ago
Created attachment 8715906 [details]
MozReview Request: Bug 1168175 - Turn on screen and disable keyboard before running each test. r=gbrown

Review commit: https://reviewboard.mozilla.org/r/33651/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33651/
Attachment #8715906 - Flags: review?(gbrown)
(Assignee)

Comment 19

2 years ago
(In reply to Nick Alexander :nalexander from comment #18)
> Created attachment 8715906 [details]
> MozReview Request: Bug 1168175 - Turn on screen and disable keyboard before
> running each test. r=gbrown
> 
> Review commit: https://reviewboard.mozilla.org/r/33651/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/33651/

Here's a new approach, cribbed from https://github.com/JakeWharton/RxBinding/pull/44/files.  Let's see how it fares in try!  It works well locally.
Comment on attachment 8715906 [details]
MozReview Request: Bug 1168175 - Turn on screen and disable keyboard before running each test. r=gbrown

https://reviewboard.mozilla.org/r/33651/#review30343
Attachment #8715906 - Flags: review?(gbrown) → review+
(Assignee)

Comment 21

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/edfca1614182aa60d23d763a3a2142dbdf966e6f
Bug 1168175 - Turn on screen and disable keyboard before running each test. r=gbrown

Comment 23

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/edfca1614182
https://hg.mozilla.org/mozilla-central/rev/80b86fb2bd9d
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Duplicate of this bug: 1043720
Depends on: 1247697
You need to log in before you can comment on or make changes to this bug.