Closed Bug 1060705 Opened 5 years ago Closed 5 years ago

Add stumbler smoke tests to Robocop

Categories

(Android Background Services Graveyard :: Geolocation, defect)

All
Android
defect
Not set

Tracking

(firefox35 fixed, firefox36 fixed)

RESOLVED FIXED
Firefox 36
Tracking Status
firefox35 --- fixed
firefox36 --- fixed

People

(Reporter: nalexander, Assigned: garvan)

References

Details

Attachments

(2 files, 5 obsolete files)

We'd like to be sure that we can actually instantiate the service and that future code doesn't disable things accidentally.
In combination with the Robocop automation to tick the checkbox to contribute to MLS, the following functions are the only straightforward way to check if the service started/stopped.

import org.mozilla.mozstumbler.service.stumblerthread.StumblerService;
if (context.startService(new Intent(context, StumblerService.class)) == null) {
// test failed, service failed to start
}

And use: 
boolean wasRunning = context.stopService(new Intent(context, StumblerService.class));
Alternatively, I could broadcast intents that the stumbler service has started and stopped, the robocop java test would listen for those intents after toggling the checkbox.
Robocop test to toggle MLS preference, and check the stumbler service received the setting changed message. 
Proves that the stumbler service is hooked up as expected to the Fennec setting.
Attachment #8514561 - Attachment is obsolete: true
Attachment #8514604 - Flags: review?(nalexander)
Comment on attachment 8514604 [details] [diff] [review]
Robocop test: testStumblerSetting.java

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

Not a huge fan of the broadcast intent, but I can't think of anything better.  (Shared flag?  Ugh.)

::: mobile/android/base/tests/testStumblerSetting.java
@@ +1,1 @@
> +package org.mozilla.gecko.tests;

License header?

@@ +7,5 @@
> +import android.content.Intent;
> +import android.content.IntentFilter;
> +import com.jayway.android.robotium.solo.Condition;
> +
> +public class testStumblerSetting extends PixelTest {

Why PixelTest?  This seems wrong.  UITest?

@@ +12,5 @@
> +    boolean mIsDone;
> +
> +    public void testStumblerSetting() {
> +        if (!AppConstants.MOZ_STUMBLER_BUILD_TIME_ENABLED) {
> +            return;

mAsserter.info that you're skipping.  Wish this could be declared in the manifest :(

@@ +30,5 @@
> +        boolean foundText = waitForPreferencesText(itemTitle);
> +        mAsserter.ok(foundText, "Waiting for settings item " + itemTitle + " in section " + section,
> +                "The " + itemTitle + " option is present in section " + section);
> +        waitForEnabledText(itemTitle);
> +        mSolo.clickOnText(itemTitle);

This is fragile.  Why do you expect the pref to be off at this point?  You should make sure it's off in some way, then turn it on.  In fact, I'd include the state in your intent broadcast, and then click the text two times, making sure you see the state toggle.  Then you don't need to know the state at the beginning, and you don't alter the state with your test.

@@ +32,5 @@
> +                "The " + itemTitle + " option is present in section " + section);
> +        waitForEnabledText(itemTitle);
> +        mSolo.clickOnText(itemTitle);
> +
> +        BroadcastReceiver receiver = new BroadcastReceiver() {

Use LocalBroadcastManager to do this so we don't broadcast to the entire phone.  What happens if the service is already running?  Also, register your receiver *before* you flip the switch!

@@ +40,5 @@
> +            }
> +        };
> +
> +        getInstrumentation().getTargetContext().registerReceiver(receiver,
> +                new IntentFilter("stumbler-test-setting-changed"));

Factor this out to a constant, use both places.

@@ +47,5 @@
> +            @Override
> +            public boolean isSatisfied() {
> +                return mIsDone;
> +            }
> +        }, 5000);

Bump this to say 15000; the boards are really, really slow and there's no sense failing fast in automation.

@@ +51,5 @@
> +        }, 5000);
> +
> +        mAsserter.ok(mIsDone, "Checking if stumbler received setting change", "Stumbler did receive setting.");
> +        getInstrumentation().getTargetContext().unregisterReceiver(receiver);
> +        // set the preference back

nit: full sentences, caps, punctuation.

::: mobile/android/stumbler/java/org/mozilla/mozstumbler/service/mainthread/PassiveServiceReceiver.java
@@ +47,5 @@
>              return;
>          }
>  
> +        // For testing service messages were received
> +        context.sendBroadcast(new Intent("stumbler-test-setting-changed"));

Use LocalBroadcastManager, use constant, include enabled flag or something else to help you test state.
Attachment #8514604 - Flags: review?(nalexander) → feedback+
Thanks Nick. 

> This is fragile.  Why do you expect the pref to be off at this point? 

This test just toggles the pref, and if the service code receives _any_ message as a result, it responds. Ensuring that this UI is connected to stumbler code.
As the end it toggles the pref back. Admittedly a tad primitive.

> @@ +40,5 @@
> > +            }
> > +        };
> > +
> > +        getInstrumentation().getTargetContext().registerReceiver(receiver,
> > +                new IntentFilter("stumbler-test-setting-changed"));
> 
> Factor this out to a constant, use both places.

I'd like to, the robocop test would need to see the stumbler classpath. Can you advise on that setup?
It also then makes it simpler/cleaner for the robocop tests to check the state of the service.
Comment on attachment 8515094 [details] [diff] [review]
Part 1: Expose stumbler jar to Robocop. r=garvank

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

Works great thanks
Attachment #8515094 - Flags: review+
This test checks the MLS setting on/off state against a testing response intent from the Stumbler.
Attachment #8515142 - Attachment is obsolete: true
Attachment #8515142 - Flags: review?(nalexander)
Attachment #8515156 - Flags: review?(nalexander)
Comment on attachment 8515156 [details] [diff] [review]
Part 2: Robocop test: turn stumbler on/off

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

So the local broadcast manager works?  If it's green locally and on try, lgtm.

::: mobile/android/base/tests/testStumblerSetting.java
@@ +1,1 @@
> +package org.mozilla.gecko.tests;

license header!

@@ +13,5 @@
> +
> +public class testStumblerSetting extends BaseTest {
> +    boolean mIsEnabled;
> +
> +    public void testStumblerSetting() {

Say a short word in the method comment about what this test does.

@@ +15,5 @@
> +    boolean mIsEnabled;
> +
> +    public void testStumblerSetting() {
> +        if (!AppConstants.MOZ_STUMBLER_BUILD_TIME_ENABLED) {
> +            mAsserter.info("Checking stumbler build config.", "Stumbler is not enabled in this build.");

say what you're doing: ... skipping test.

@@ +42,5 @@
> +                    mIsEnabled = true;
> +                } else {
> +                    mIsEnabled = false;
> +                }
> +

nit: delete extra newlines.

@@ +47,5 @@
> +            }
> +        };
> +
> +        Context context = getInstrumentation().getTargetContext();
> +        IntentFilter intentFilter =  new IntentFilter(AppGlobals.ACTION_TEST_SETTING_ENABLED);

nit: what's up with these double spaces?  Maybe just format this code in Eclipse/IntelliJ?

@@ +76,5 @@
> +        }, 15000);
> +
> +        mAsserter.ok(!mIsEnabled, "Checking if stumbler became disabled.", "Stumbler is disabled.");
> +
> +        context.unregisterReceiver(enabledDisabledReceiver);

Wrap this in a try/finally block.  It's not a huge deal 'cuz the process gets killed between tests, but let's not be sloppy.

::: mobile/android/stumbler/java/org/mozilla/mozstumbler/service/mainthread/PassiveServiceReceiver.java
@@ +61,5 @@
>              return;
>          }
>  
> +        // For testing service messages were received
> +        context. sendBroadcast(new Intent(AppGlobals.ACTION_TEST_SETTING_ENABLED));

nit: space?
Attachment #8515156 - Flags: review?(nalexander) → review+
> So the local broadcast manager works?  

No it doesn't, my assumption is that robo testing is in a different process than Fennec proper.
Thanks for the review Nick, addressed the comments (file header, spacing, wrap in try/finally), and marking r+.
Attachment #8515156 - Attachment is obsolete: true
Attachment #8517634 - Flags: review+
Oops, part 1 got accidentally included in part 2 in comment 14, replacing the patch.
Attachment #8517634 - Attachment is obsolete: true
Attachment #8517644 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f7289c407888
https://hg.mozilla.org/mozilla-central/rev/7125511a8714
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 36
Assignee: nobody → nalexander
Assignee: nalexander → gkeeley
Depends on: 986738
Comment on attachment 8515094 [details] [diff] [review]
Part 1: Expose stumbler jar to Robocop. r=garvank

Approval Request Comment
[Feature/regressing bug #]: Stumbler for Fennec
[User impact if declined]: None. 
[Describe test coverage new/current, TBPL]: Test that the stumbler module is properly hooked up to Fennec preferences system.
[Risks and why]: Without this, there is a risk that a change to the preferences UI could unhook stumbler start/stop.
[String/UUID change made/needed]: None.

Also need to land its buddy bug: Bug 1096517, which disables this test on 2.3 where it doesn't run.
Attachment #8515094 - Flags: approval-mozilla-aurora?
Attachment #8515094 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
tracking-fennec: --- → ?
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.