1068489 - Robocop: Provide error message for test failure where device screen may simply be sleeping

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Testing, defect)

Description

[Mark Capella [:capella]]

A common issue I've noticed seems to be when triggering a robocop test on a device who's screen has gone into power-save / sleep mode. The standard messages provided in that case are as (below), and nothing jumps out hinting @ that possibility. I wonder if we can identify this situation during testing, and generate something like:

"   The screen on your device may be sleeping to save power ..."
"   If so, turn on the device display and re-start this test"

This might be a "core" bug, but I'll start it here.

-----------------------------------------
Device info: {}
Test root: /storage/sdcard0/tests
Android sdk version '19'; will use this to filter manifests
SUITE-START | Running 1 tests
Error: Could not find debugger None.
pk12util: PKCS12 IMPORT SUCCESSFUL
MochitestServer : launching [u'/home/master/mozilla-central-objff/dist/bin/xpcshell', '-g', '/home/master/mozilla-central-objff/dist/bin', '-v', '170', '-f', '/home/master/mozilla-central-objff/dist/bin/components/httpd.js', '-e', "const _PROFILE_PATH = '/tmp/tmpGyDnjy.mozrunner'; const _SERVER_PORT = '8888'; const _SERVER_ADDR = '192.168.2.5'; const _TEST_PREFIX = undefined; const _DISPLAY_RESULTS = false;", '-f', '/home/master/mozilla-central-droid/_tests/testing/mochitest/server.js']
runtests.py | Server pid: 313
runtests.py | Websocket server pid: 315
runtests.py | SSL tunnel pid: 317
runtests.py | Running tests: start.

Robocop process name: org.mozilla.fennec
INFO | automation.py | Application pid: 7522
SimpleTest START
TEST-START | testSelectionHandler
TEST-PASS | testSelectionHandler | Given message occurred for registered event: {"type":"Gecko:Ready"} - Gecko:Ready should equal Gecko:Ready
EventExpecter: no longer listening for Gecko:Ready
TEST-PASS | testSelectionHandler | url is not null - chrome://roboextender/content/testSelectionHandler.html should not equal null
TEST-PASS | testSelectionHandler | url is not null - chrome://roboextender/content/testSelectionHandler.html should not equal null
TEST-PASS | testSelectionHandler | The toolbar is not in the editing state - 
TEST-UNEXPECTED-FAIL | testSelectionHandler | Waiting for Toolbar to enter editing mode. - 
TEST-OK | testSelectionHandler | took 8931ms
TEST-START | Shutdown
Passed: 4
Failed: 1
Todo: 0
SimpleTest FINISHED

INFO | automation.py | Application ran for: 0:00:12.230976
INFO | zombiecheck | Reading PID log: /tmp/tmpLxhkNOpidlog
Contents of /data/anr/traces.txt:

MOZ_UPLOAD_DIR not defined; tombstone check skipped
Stopping web server
Stopping web socket server
Stopping ssltunnel
WARNING | leakcheck | refcount logging is off, so leaks can't be detected!
runtests.py | Running tests: end.

Comment 1

[Nick Alexander :nalexander [he/him]]

I've seen this, and have a partial fix that works on some of my devices -- see Bug 929654, specifically the patch labeled:

Bug 929654 - Part 3: Turn off keyguard and turn on screen when waiting for Gecko:Ready. r=jmaher

I'd be pretty happy to just fail the test (in setup) if we can detect that the screen is off.  I have a patch on that ticket to die if we can't reach the HTTP server.

The reason we need the screen is that we don't start Gecko completely until we get onAttachedToWindow, which doesn't happen if the screen is not on.

Comment 2

[Mark Capella [:capella]]

Attachment: bug1068489.diff

Something really simple like this? Not knowing the entire test suite architecture I'd assume checking in UITest and probably PixelTest would be a safe bet.

But is there a blanket rule we can use to determine which tests get protected this way? Not all tests that inherit from BaseTest for example ContentProviderTest/JavascriptTest(s) seem to need this (for ex: testSharedPreferences runs just fine with the screen off -or- on).

Comment 3

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8492694 [details] [diff] [review]
bug1068489.diff

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

I would do this for every test, needed or not.  Let's make one of the requirements of Robocop be that the screen is on.

::: mobile/android/base/tests/UITest.java
@@ +77,5 @@
>          // Helpers depend on components so initialize them first.
>          initComponents();
>          initHelpers();
> +
> +        PowerManager pm = (PowerManager) activity.getSystemService(Context.POWER_SERVICE);

nit: final.

And break out a little helper: throwIfScreenNotOn().

@@ +79,5 @@
>          initHelpers();
> +
> +        PowerManager pm = (PowerManager) activity.getSystemService(Context.POWER_SERVICE);
> +        if (!pm.isScreenOn()) {
> +            dumpLog(LOGTAG, "The display on your test device doesn\'t appear to be active,");

Is the \' necessary? I think not.

Also, why not just mAssert.ok(pm.isScreenOn(), "Helpful message that includes the word Robocop.")?

Comment 4

[Mark Capella [:capella]]

Attachment: bug1068489.diff

This seems to do the trick :-) I went ahead and ran it through try ...
https://tbpl.mozilla.org/?tree=Try&rev=d20e77974fcf

Comment 5

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8497321 [details] [diff] [review]
bug1068489.diff

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

Thanks for the try build.  You'd be doing a public service if you updated the patch that tried to connect to the (IP address) of mochi.test and early aborted if it failed.  That would help most local immediate failures.

::: mobile/android/base/tests/BaseRobocopTest.java
@@ +114,5 @@
> +     * Ensure that the screen on the test device is powered on during tests.
> +     */
> +    public void throwIfScreenNotOn() {
> +        final PowerManager pm = (PowerManager) getActivity().getSystemService(Context.POWER_SERVICE);
> +        mAsserter.ok(pm.isScreenOn(), "The display on your test device needs to be powered on.",

Include the magic words "Robocop" in the message, please.  (Can't remember the log tag.)

@@ +115,5 @@
> +     */
> +    public void throwIfScreenNotOn() {
> +        final PowerManager pm = (PowerManager) getActivity().getSystemService(Context.POWER_SERVICE);
> +        mAsserter.ok(pm.isScreenOn(), "The display on your test device needs to be powered on.",
> +                                      "\nIf it's in sleep mode, turn it on, then Re-start this test.\n");

I'd just go short and sweet: "Robocop tests need the test device screen to be powered on."

Comment 6

[Nick Alexander :nalexander [he/him]]

Flagging jmaher (gbrown is on PTO, IIRC) just in case this is a bad idea on infra.

Comment 7

[Joel Maher ( :jmaher ) (UTC -8)]

I don't see anything wrong with this approach :)

Comment 8

[Mark Capella [:capella]]

https://hg.mozilla.org/integration/fx-team/rev/aa1bad455192

Comment 9

[Carsten Book [:Tomcat]]

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