Closed Bug 1069687 Opened 9 years ago Closed 2 years ago

Robocop tests run against the default profile

Categories

(Firefox for Android Graveyard :: Testing, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED INCOMPLETE

People

(Reporter: rnewman, Unassigned, Mentored)

References

Details

(Whiteboard: [has bitrotted patch])

Attachments

(1 file, 2 obsolete files)

Testing current fx-team, so I don't know if this changed recently.

Instrumenting:

diff --git a/mobile/android/base/GeckoSharedPrefs.java b/mobile/android/base/GeckoSharedPrefs.java
--- a/mobile/android/base/GeckoSharedPrefs.java
+++ b/mobile/android/base/GeckoSharedPrefs.java
@@ -96,16 +96,17 @@ public final class GeckoSharedPrefs {
      * flag.
      */
     public static SharedPreferences forProfile(Context context, EnumSet<Flags> flags) {
         String profileName = GeckoProfile.get(context).getName();
         if (profileName == null) {
             throw new IllegalStateException("Could not get current profile name");
         }

+        Log.i("GeckoXXX", "Opening SharedPreferences: " + profileName);
         return forProfileName(context, profileName, flags);
     }

     public static SharedPreferences forProfileName(Context context, String profileName) {
         return forProfileName(context, profileName, EnumSet.noneOf(Flags.class));
     }

     /**

Yields the horror:

09-18 16:12:07.631 I/GeckoXXX(26421): Opening SharedPreferences: default

Similarly, instrumenting browser.js:

GeckoXXX: /storage/emulated/legacy/tests/profile


The profile directory is deleted between app runs, but *SharedPreferences is not*. That means that on developer machines, tests run with a config that's half fresh, half stale.

We should not do this.
And in BaseTest#runTest:

mProfile is /storage/sdcard0/tests/profile
This seems to be the smallest possible change for sanity: clear the profile's prefs prior to running the test.

Testing now.
Attachment #8491921 - Flags: review?(mark.finkle)
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
Now without spurious import.
Attachment #8491930 - Flags: review?(mark.finkle)
Attachment #8491921 - Attachment is obsolete: true
Attachment #8491921 - Flags: review?(mark.finkle)
Comment on attachment 8491930 [details] [diff] [review]
Clear per-profile preferences for the provided profile prior to Robocop test run. v2

Actually, this needs a new version -- this uses a profile path, so *tableflip*
Attachment #8491930 - Flags: review?(mark.finkle)
Here's a better approach.

By passing a profile name, we use a unique prefs file for each test run.

They don't get cleaned up, but that's not a problem on infra, and developers can always use Clear Data.
Attachment #8491948 - Flags: review?(mark.finkle)
Attachment #8491930 - Attachment is obsolete: true
Comment on attachment 8491948 [details] [diff] [review]
Specify a unique profile name for each Robocop test run. v1

Let's hope this does not make TBPL unhappy
Attachment #8491948 - Flags: review?(mark.finkle) → review+
It very probably does, but I'll fix the tests!
How are you launching Robocop?  I wonder if something's up with that.  'cuz I'm pretty sure we're okay on instrumentation.
(In reply to Richard Newman [:rnewman] from comment #5)
> Created attachment 8491948 [details] [diff] [review]
> Specify a unique profile name for each Robocop test run. v1
> 
> Here's a better approach.
> 
> By passing a profile name, we use a unique prefs file for each test run.

I'm not confident that -p and -P interact well together.  Try run, please.

> They don't get cleaned up, but that's not a problem on infra, and developers
> can always use Clear Data.

That's not true -- we run out of inodes on the Pandas occasionally.
(In reply to Nick Alexander :nalexander from comment #8)
> How are you launching Robocop?  I wonder if something's up with that.  'cuz
> I'm pretty sure we're okay on instrumentation.

Mach. And the same is true for Eclipse, IIRC.
(In reply to Nick Alexander :nalexander from comment #9)

> That's not true -- we run out of inodes on the Pandas occasionally.

I was under the impression that we uninstall the app between test runs. That'll clean up these files.
(In reply to Richard Newman [:rnewman] from comment #11)
> (In reply to Nick Alexander :nalexander from comment #9)
> 
> > That's not true -- we run out of inodes on the Pandas occasionally.
> 
> I was under the impression that we uninstall the app between test runs.
> That'll clean up these files.

Ah, I reckon you're correct.
This'll take a few iterations of test fixes to step through, so expect tiny follow-up reviews.
Blocks: 1045053
This has newfound relevance for something I'm doing in Bug 1077590. Marking as blocking that, just 'cos See Also is pretty vague.
Blocks: 1077590
Assignee: rnewman → nobody
Mentor: gbrown, nalexander
Status: ASSIGNED → NEW
Whiteboard: [has bitrotted patch]
We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → INCOMPLETE
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.