1120530 - Reproducible 'waiting for urlbar text to gain focus' when first run experience is visible

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Testing, defect)

Description

[Richard Newman [:rnewman]]

* Clear data.
* Run a Robocop test that waits for URL bar focus -- e.g., testClearPrivateData.
* Observe that the screen stays stuck on the FRE, and the test fails:

Ran 4 tests (1 parents, 3 subtests)
Expected results: 2
Unexpected results: 2 (FAIL: 2)

Unexpected Results
==================

testClearPrivateData
--------------------
FAIL waiting for urlbar text to gain focus
FAIL Exception caught

with these in the log:

01-12 10:27:43.801 D/Telemetry( 4267): SendUIEvent: event = cancel.1 method = dialog timestamp = 989904 extras = firstrun-pane
01-12 10:27:53.954 I/Robocop ( 4267): {"thread":null,"source":"robocop","pid":null,"message":"waitForCondition timeout after 10000 ms.","time":1421087273949,"action":"log","level":"info"}
01-12 10:27:53.960 I/Robocop ( 4267): {"thread":null,"source":"robocop","pid":null,"status":"FAIL","time":1421087273954,"expected":"PASS","action":"test_status","test":"testClearPrivateData","subtest":"waiting for urlbar text to gain focus","message":"urlbar text gained focus"}

Comment 1

[Richard Newman [:rnewman]]

Attachment: Don't show first run experience in Robocop tests. v1

This is the idea I had way back.

Comment 2

[Geoff Brown [:gbrown]]

Comment on attachment 8547709 [details] [diff] [review]
Don't show first run experience in Robocop tests. v1

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

::: mobile/android/base/tests/BaseTest.java
@@ +114,5 @@
>          Intent i = new Intent(Intent.ACTION_MAIN);
>          mProfile = mConfig.get("profile");
> +
> +        // Don't show the first run experience.
> +        i.putExtra(BrowserApp.EXTRA_SKIP_START_PANE, true);

You probably want to do this in UITest too (those tests don't inherit from BaseTest).

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8547709 [details] [diff] [review]
Don't show first run experience in Robocop tests. v1


>diff --git a/mobile/android/base/BrowserApp.java b/mobile/android/base/BrowserApp.java
>+    public static final String EXTRA_SKIP_START_PANE = "skipstartpane";

EXTRA_SKIP_START_PANE -> EXTRA_SKIP_STARTPANE

>-     * Check and show Onboarding start pane if Firefox has never been launched and
>+     * Check and show onboarding start pane if Fennec has never been launched and

s/Firefox|Fennec/browser

Address Geoff's comment too!

Comment 4

[Richard Newman [:rnewman]]

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

Comment 5

[Richard Newman [:rnewman]]

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

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9708afb1e274

Comment 7

[Chenxia Liu [:liuche]]

Instead of making an extra pref, is there a reason why you don't just use PREF_START_PANE_ENABLED and pass false into the intent?

Comment 8

[Richard Newman [:rnewman]]

(In reply to Chenxia Liu [:liuche] from comment #7)
> Instead of making an extra pref, is there a reason why you don't just use
> PREF_START_PANE_ENABLED and pass false into the intent?

I'm not sure I understand your question. This patch didn't make a pref, and the P_S_P_E approach doesn't use the intent.

(Did you mean "intent extra" instead of "extra pref"?)


The code prior to the patch checks the value of a per-profile shared pref to decide whether to show the start pane. After showing the pane, it sets that pref so it isn't shown again.

There was no intent-based aspect to this at all: to use P_S_P_E from tests we'd have to find the profile, open SharedPreferences, and set the pref before launching the activity, and that will persist.


The approach I took was to set an extra on the intent itself. No read, no write; whether to show the start pane is a property of the intent itself.

Comment 9

[Chenxia Liu [:liuche]]

No, you're right - I was stuck thinking about prefs and thought Intents could take prefs or something, which is pretty bogus now that I stop to think about it.

Passing an intent extra seems odd - isn't there a way to mock some SharedPreferences values for tests?

Comment 10

[Richard Newman [:rnewman]]

We definitely want to do that, partly to solve our tests running against the persistent app prefs (touched on in Bug 1069687). But mocking all of our use of SharedPreferences is a much bigger change, so I wanted to fix the bug first.

The intent extra approach isn't that odd; it's basically how you pass arguments via intents, so this is only as awful as any other boolean flag.