Closed Bug 1120530 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox for Android Graveyard :: Testing, defect)

ARM
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 38

People

(Reporter: rnewman, Assigned: rnewman)

References

()

Details

(Keywords: reproducible)

Attachments

(1 file)

* 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"}
Assignee: nobody → rnewman
Blocks: 1059792
Status: NEW → ASSIGNED
This is the idea I had way back.
Attachment #8547709 - Flags: review?(mark.finkle)
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 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!
Attachment #8547709 - Flags: review?(mark.finkle) → review+
https://hg.mozilla.org/mozilla-central/rev/9708afb1e274
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
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?
Flags: needinfo?(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.
Flags: needinfo?(rnewman)
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?
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.
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.