Closed Bug 1168175 Opened 9 years ago Closed 8 years ago

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

Categories

(Firefox for Android Graveyard :: Testing, defect)

Product:
Firefox for Android Graveyard
Component:
Testing
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox41 affected, firefox47 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 47
Tracking Flags:
Tracking Status
firefox41 --- affected
firefox47 --- fixed

People

(Reporter: nalexander, Assigned: nalexander)

References

Details

Attachments

(1 file, 2 obsolete files)

MozReview Request: Bug 1168175 - Turn on screen and disable keyboard before running each test. r=gbrown
58 bytes, text/x-review-board-request
gbrown
: review+
Details 
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: nobody → nalexander
Status: NEW → ASSIGNED
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)
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)
(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
(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+
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.
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.
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.
Attachment #8612639 - Attachment is obsolete: true
Attachment #8612640 - Attachment is obsolete: true
(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+
https://hg.mozilla.org/integration/fx-team/rev/edfca1614182aa60d23d763a3a2142dbdf966e6f
Bug 1168175 - Turn on screen and disable keyboard before running each test. r=gbrown
https://hg.mozilla.org/mozilla-central/rev/edfca1614182
https://hg.mozilla.org/mozilla-central/rev/80b86fb2bd9d
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Depends on: 1247697
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: