1106900 - Move mSolo and other Robocop components into BaseRobocopTest

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Testing, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Currently, certain Robocop members are duplicated in both UITest and BaseTest (e.g. [1] & [2]). These members can be moved into the parent class, BaseRobocopTest.

I'm pretty sure it doesn't have to be this way, though createActivityIntent (ala UITest) may need to become an abstract method of BaseRobocopTest.

Try it out!

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/UITest.java?rev=63061f730d01#67
[2]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/BaseTest.java?rev=217c7f7d9f48#133

Comment 1

[Manu Jain]

Can you tell, what we need to do in this bug?

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

The basic premise is to prevent any members used by Robotium, in particular mSolo, from being duplicated in both UITest and BaseTest by moving the initialization into BaseRobocopTest (robocop is our wrapper for the Robotium framework). See [1] for Robotium reference. Additionally, mActions and mDriver also look like good candidates to move into BaseRobopcopTest.

It may require some further changes, for example, the Intent initialization in UITest.createActivityIntent [2] also occurs in BaseTest, but in a different way - this may need to be an abstract method of BaseRobocopTest, as per comment 0.

Does this make sense?

[1]: https://robotium.googlecode.com/svn/doc/index.html
[2]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/UITest.java?rev=63061f730d01#63

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

To start, have you set up a build environment yet? If not, you can see the instructions here: https://wiki.mozilla.org/Mobile/Fennec/Android

The tests you would be fixing are in our Robocop test framework, so you'll need to set that up too: https://wiki.mozilla.org/Auto-tools/Projects/Robocop

When you make changes in the mobile/android/base/tests/ directory, you should only need to recompile Robocop, not the entire browser, for these changes.

While you're developing, I recommend running just a single test at a time because the full robocop test suite takes a long time to run (at least 30 minutes).

Manu, would you like to be assigned to this bug?

Comment 4

[Manu Jain]

Yes, I like to be assigned to this bug.
This bug only requires moving duplicate members to parent method?
Can you tell which all members are duplicated?
In the link that you have given you recommend IDE for refactoring. Can you tell, why? Thanks!!

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Manu Jain from comment #4)
> Yes, I like to be assigned to this bug.

Done! :)

> This bug only requires moving duplicate members to parent method?

Not to the parent method, to the parent class. The members will still need to be initialized in the parent constructor. This move may require other changes to the initialization process, as per comment 2.

> Can you tell which all members are duplicated?

mSolo, mActions, and mDriver, as per comment 2. Maybe more (see also bug 1107253).

> In the link that you have given you recommend IDE for refactoring. Can you
> tell, why? Thanks!!

IDEs can manipulate the code in ways that make refactoring easy - e.g. moving a file to a sub-package is more or less one click, whereas this process in a text editor requires updating the package information in the file to be moved and updating the imports in all files where this is used.

In this case, if we're renaming variables, it can be a painful process through sed scripts or by hand - it's much easier to make that one click in the IDE! :)

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Hi, Manu. Are you still working on this? It's blocking progress on bug 1106903.

Comment 7

[Vivek Balakrishnan[:vivek]]

Hi Manu,
Since we're being blocked, I'm going to assign this bug to Jalpreet from the bug 1106903 after 24 hours - Please let us know if you are still working on this, or want to be assigned to another bug.

Comment 8

[Vivek Balakrishnan[:vivek]]

@Jalpreet: Please feel free to NI me if you have any questions.

Comment 9

[Jalpreet Singh Nanda [:imjalpreet]]

Can you explain me what change do I need to do in the UITest.createActivityIntent to move it to the BaseRobocopTest class?

Comment 10

[Jalpreet Singh Nanda [:imjalpreet]]

As far as I have seen the method createActivityIntent is not used in BaseTest, though the same code is directly written in BaseTest.setUp. So should I make a method in BaseRobocopTest or Do it the way it is done in BaseTest?

Comment 11

[Vivek Balakrishnan[:vivek]]

Hi Jalpreet,

As per this bug description [1], create an abstract method createActivityIntent in BaseRobocopTest and override it in UITest and BaseTest. Please let me know if you need any other clarifications

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1106900#c0

Comment 12

[Jalpreet Singh Nanda [:imjalpreet]]

(In reply to Vivek Balakrishnan[:vivek] from comment #11)

> As per this bug description [1], create an abstract method
> createActivityIntent in BaseRobocopTest and override it in UITest and
> BaseTest. Please let me know if you need any other clarifications

So should I still keep the createActivityIntent method in UITest and create this method in BaseTest and as an abstract method in BaseRobocopTest? Am I right or do I have to do it in a different way?

Comment 13

[Vivek Balakrishnan[:vivek]]

Yes you are right.
So the changes you have to do are:
* create an abstract method (createActivityIntent) in BaseRobocopTes
* mark the createActivityIntent in UITest with @Override annotation
* In BaseTest, move the relevant intent creation code in setup to the newly overridden creativeActivityIntent.
* Move the duplicate Robocop mmembers to BaseRobocopTest as detailed in this bug description.

While you are developing, you can also run a couple of UITest and BaseTest to ensure that your changes don't break anything. testSessionHistory and testNewTab can be considered as candidates for this.

As you have solved the bug 1107253, I hope do know how to run these test. Please feel free to NI me if you are struck.

Happy coding.

Comment 14

[Jalpreet Singh Nanda [:imjalpreet]]

Attachment: Bug-1106900-Proposed_Patch

I have done all the changes as expected. I just have one doubt where should I keep the **mActivity = getActivity();**, in BaseRobocopTest or in both UITest and BaseTest ?

Comment 15

[Vivek Balakrishnan[:vivek]]

Comment on attachment 8541508 [details] [diff] [review]
Bug-1106900-Proposed_Patch

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

Hi Jalpreet, 

mSolo, mActivity, mDriver initialization requires mActivity to be initialized. So getActivity must be called after setActivityIntent in BaseRobocopTest setup().

However, I just noticed that BaseRobocopTest is also the base type for testBrowserProviderPerf, which is a talos test [1]. So we need to override the 
createActivityIntent in testBrowserPerfTest  as well. The testBrowserPerfTest setup calls getActivity without calling setActivityIntent, so we can return null for overridden createActivityIntent. Null intent here means the default intent [2]. 

I'm NIing mcomella to shed some light on the talos tests.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testBrowserProviderPerf.java#211
[2] http://developer.android.com/reference/android/test/ActivityInstrumentationTestCase2.html#setActivityIntent(android.content.Intent)

::: mobile/android/base/tests/BaseRobocopTest.java
@@ +16,5 @@
>  import org.mozilla.gecko.FennecInstrumentationTestRunner;
>  import org.mozilla.gecko.FennecMochitestAssert;
>  import org.mozilla.gecko.FennecNativeDriver;
>  import org.mozilla.gecko.FennecTalosAssert;
> +import org.mozilla.gecko.Actions;

Alphabetical order please

@@ +72,5 @@
> +    protected Actions mActions;
> +
> +    public Activity mActivity;
> +
> +    public abstract Intent createActivityIntent(final Map<String, String> config);

method access must be protected

@@ +129,5 @@
>          }
>          mAsserter.setLogFile(mLogFile);
>          mAsserter.setTestName(getClass().getName());
>  
> +        // Set up Robotium.solo and Driver objects

createActivityIntent and getActivity must be called before this.

::: mobile/android/base/tests/BaseTest.java
@@ +108,3 @@
>          // Start the activity.
> +        final Intent intent = createActivityIntent(mConfig);
> +        setActivityIntent(intent);

mSolo initialization requires the mActivity. So we move this to super class setup.

Comment 16

[Jalpreet Singh Nanda [:imjalpreet]]

Attachment: Bug-1106900-Proposed_Patch

I have done the changes as you suggested. Please tell if any changes are still to be done. :)

Comment 17

[Vivek Balakrishnan[:vivek]]

Comment on attachment 8542137 [details] [diff] [review]
Bug-1106900-Proposed_Patch

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

Hi Jalpreet,

Thanks for your quick patches. We are almost there. There are some minor corrections and I would like some clarification regarding mochitest.
see below for my review comments.

I spoke with mcomella via IRC and we speculate that createActivityIntent may not be relevant to talos tests.
However, we are unsure about the intent provided to mochitests. Just to be sure I'm NIing gbrown for his opinion.

@gbrown: Is there something special about the intent given to mochitests?

::: mobile/android/base/tests/BaseTest.java
@@ -14,5 @@
>  
>  import org.json.JSONArray;
>  import org.json.JSONException;
>  import org.json.JSONObject;
> -import org.mozilla.gecko.Actions;

We need to retain Actions import. However, you can go ahead remove the imports for FennecNativeActions and FennecNativeDriver here along with the Driver.

@@ +75,1 @@
>      protected String mProfile;

Pull mProfile to Base class.

@@ +164,5 @@
> +    @Override
> +    protected Intent createActivityIntent(final Map<String, String> config) {
> +        final Intent intent = new Intent(Intent.ACTION_MAIN);
> +
> +        final String profile = config.get("profile");

Use mProfile instead as it is initialized here and used elsewhere within BaseTest.

@@ +168,5 @@
> +        final String profile = config.get("profile");
> +        intent.putExtra("args", "-no-remote -profile " + profile);
> +
> +        final String envString = config.get("envvars");
> +        if (!TextUtils.isEmpty(envString)) {

I like this version of string check instead of the != check.

::: mobile/android/base/tests/UITest.java
@@ -41,5 @@
>      private static final String JUNIT_FAILURE_MSG = "A JUnit method was called. Make sure " +
>          "you are using AssertionHelper to make assertions. Try `fAssert*(...);`";
>  
> -    private Solo mSolo;
> -    private Driver mDriver;

You can remove the unused imports for FennecNativeActions, FennecNativeDriver and Activity.

If you are using IDE for developing, it would highlight the unused imports :)

@@ +182,3 @@
>          final Intent intent = new Intent(Intent.ACTION_MAIN);
>  
>          final String profile = config.get("profile");

Changes this to mProfile even though it is not used anymore in UITest.

::: mobile/android/base/tests/testBrowserProviderPerf.java
@@ +218,5 @@
>          mHistoryURI = prepUri(BrowserContract.History.CONTENT_URI);
>          mBookmarksURI = prepUri(BrowserContract.Bookmarks.CONTENT_URI);
>          mFaviconsURI = prepUri(BrowserContract.Favicons.CONTENT_URI);
>  
> +        mResolver = mActivity.getApplicationContext().getContentResolver();

Nice.

@@ +317,5 @@
>      }
> +
> +    @Override
> +    protected Intent createActivityIntent(final Map<String, String> config) {
> +        return null;

Add a comment stating that null intent is equivalent to default intent and it shouldn't impede activity launch

Comment 18

[Geoff Brown [:gbrown]]

(In reply to Vivek Balakrishnan[:vivek] from comment #17)
> I spoke with mcomella via IRC and we speculate that createActivityIntent may
> not be relevant to talos tests.
> However, we are unsure about the intent provided to mochitests. Just to be
> sure I'm NIing gbrown for his opinion.
> 
> @gbrown: Is there something special about the intent given to mochitests?

In general - for both mochitest-based and talos-based robocop tests - the intent needs to set up the -profile argument to point to the test profile and set environment variables for the crashreporter, etc. (For Talos, note that testCheck2 and testPan inherit from BaseTest. testBrowserProviderPerf is different and I notice that it sets its own mProfile...I'm not sure of the history there.)

To make sure that things are working right, we might check something like http://hg.mozilla.org/mozilla-central/annotate/2c0ef7315830/mobile/android/base/GeckoApp.java#l1161 with the expectation that the profile is being set to something like /mnt/sdcard/tests/profile there.

Comment 19

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8542137 [details] [diff] [review]
Bug-1106900-Proposed_Patch

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

I'll wait for the final patch to give it a look over.

::: mobile/android/base/tests/BaseTest.java
@@ -14,5 @@
>  
>  import org.json.JSONArray;
>  import org.json.JSONException;
>  import org.json.JSONObject;
> -import org.mozilla.gecko.Actions;

This no longer compiles because Actions was removed.

Make sure you're building and testing the Robocop APK [1] when you change this test code (in mobile/android/base/tests/). Note that because the test code is a separate APK, you don't need to rebuild the browser when making changes to this code.

[1]: https://wiki.mozilla.org/Mobile/Fennec/Android#Robocop

Comment 20

[Jalpreet Singh Nanda [:imjalpreet]]

Yes, I do test the code but I think Attached the wrong version of the patch that's why that import was missing. I will make the required changes and get back to you.

Comment 21

[Jalpreet Singh Nanda [:imjalpreet]]

Attachment: Bug-1106900-Proposed_Patch

I have the changes as suggested by you. Please tell me if any more changes are required.

Comment 22

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8542541 [details] [diff] [review]
Bug-1106900-Proposed_Patch

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

Vivek will flag me for review when he r+'s.

Comment 23

[Vivek Balakrishnan[:vivek]]

Comment on attachment 8542541 [details] [diff] [review]
Bug-1106900-Proposed_Patch

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

Hi Jalpreet,

I've one minor nit for you. Apart from that it looks good to me.

I've started a try server run for your patch:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1c2861e3da54

I can give you r+ if it is green.

Thanks for your patience.

::: mobile/android/base/tests/BaseRobocopTest.java
@@ +70,5 @@
> +    protected Solo mSolo;
> +    protected Driver mDriver;
> +    protected Actions mActions;
> +
> +    public Activity mActivity;

Change mActivity protected access

Comment 24

[Jalpreet Singh Nanda [:imjalpreet]]

Attachment: Bug-1106900-Proposed_Patch

I have made that protected.

Comment 25

[Vivek Balakrishnan[:vivek]]

Comment on attachment 8542672 [details] [diff] [review]
Bug-1106900-Proposed_Patch

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

Hi,
Thanks for the quick turn-around.

lgtm.

I'll change this to r+ if the try run is green.

Comment 26

[Jalpreet Singh Nanda [:imjalpreet]]

Now, what should I do as Three tests have failed?

Comment 27

[Vivek Balakrishnan[:vivek]]

Hi Jalpreet, 

I'm currently investigating the reason for the crash and wil have an update for you by monday. 

This may be a bit of a challenge for someone starting new. But, are you interested in taking the challenge? It is totally voluntary :)

If you are interested, then you should start analyzing why testSessionOOMRestore fails in Android 4.0 and Android 4.2. Try running them locally. Note : this particular test overrides the setActivityIntent [1].

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testSessionOOMRestore.java#19

Thanks for your patience.

Comment 28

[Jalpreet Singh Nanda [:imjalpreet]]

I will try but what should I do if I don't have a android 4.0 or a 4.2 device with me? Actually I only have a android 4.4 device, would this test fail in that too?

Comment 29

[Vivek Balakrishnan[:vivek]]

Hi Jalpreet,

Thanks for the help.
I guess you can use your device to analyze it.

For your information: you can find the complete log of a run when you click on it from here.
https://tbpl.mozilla.org/?tree=Try&rev=1c2861e3da54

Comment 30

[Jalpreet Singh Nanda [:imjalpreet]]

Is there any help I can do?

Comment 31

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Hey, Jalpreet - sorry for keeping you waiting! Thanks for being patient!

(In reply to Vivek Balakrishnan[:vivek] from comment #23)
> I've started a try server run for your patch:
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1c2861e3da54

Seems this has gone stale - here's another push to our test servers: 
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad19aa1ea231

Do you know how to analyze the results? If not, let me know!

If the failure still occurs, there are two approaches to debugging it:

  1) Read the remote logs from the push - there should be both an abridged version and the raw log - let me know if you have difficulty finding these.
  2) Run the test locally and see if it crashes. If so, diagnose! You can view the test results (and thus the crash) in the terminal you ran robocop from. To run robocop locally, see [1].

Best to combine both approaches, in my opinion.

[1]: https://wiki.mozilla.org/Auto-tools/Projects/Robocop#Running_tests

Comment 32

[Vivek Balakrishnan[:vivek]]

Comment on attachment 8542672 [details] [diff] [review]
Bug-1106900-Proposed_Patch

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

Hi Jalpreet, 

Thanks for being patient.

The issue with testSessionOOMRestore failure is that the mBaseHostnameURl is not initialized when we call setActivityIntent [1].
In testSessionOOMRestore.setActivityIntent(), we create the session tabs which depends on the above field to correctly form the page url [2] [3].
The following change should solve the issue.

I've created a try push with the proposed changes.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d529039ea71

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testSessionOOMRestore.java#19
[2] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/SessionTest.java#51
[3] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/SessionTest.java#358

::: mobile/android/base/tests/BaseRobocopTest.java
@@ +144,1 @@
>          mBaseHostnameUrl = mConfig.get("host").replaceAll("(/$)", "");

mBaseHostnameUrl and mBaseIpUrl value depends only on mConfig. This  can be done immediately after mConfig is initialized i.e before calling createActivityIntent()

Comment 33

[Jalpreet Singh Nanda [:imjalpreet]]

Still one test is getting failed in this. What can be done to pass that test?

Comment 34

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Jalpreet Singh Nanda [:imjalpreet] from comment #33)
> Still one test is getting failed in this. What can be done to pass that test?

It looks like Vivek's change may be a common issue (if you click on the run, on the bottom-right similar intermittent failures will be shown) so he may have caused this particular failure but maybe not! I've rerun that particular test suite so we should know the results in a few hours. NI Jalpreet to check on the results! :)

Comment 35

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Jalpreet, Vivek - looks like we're still having issues: do you mind taking a look?

Comment 36

[Jalpreet Singh Nanda [:imjalpreet]]

Yeah, I checked the results. I think now that test has passed! :)
So, now after this is merged I can return to bug 1106903 or is there something still left?

Comment 37

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Vivek, can you post your revisions on the patch so I can review?

Comment 38

[Vivek Balakrishnan[:vivek]]

Attachment: 1106900.patch

New Patch attached with my revisions from https://bugzilla.mozilla.org/show_bug.cgi?id=1106900#c32

Comment 39

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8552668 [details] [diff] [review]
1106900.patch

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

::: mobile/android/base/tests/UITest.java
@@ +181,5 @@
>  
>          // Don't show the first run experience.
>          intent.putExtra(BrowserApp.EXTRA_SKIP_STARTPANE, true);
>  
> +        mProfile = config.get("profile");

mProfile's assignment can be moved to BaseRobocopTest.

::: mobile/android/base/tests/testBrowserProviderPerf.java
@@ +218,5 @@
>          mHistoryURI = prepUri(BrowserContract.History.CONTENT_URI);
>          mBookmarksURI = prepUri(BrowserContract.Bookmarks.CONTENT_URI);
>          mFaviconsURI = prepUri(BrowserContract.Favicons.CONTENT_URI);
>  
> +        mResolver = mActivity.getApplicationContext().getContentResolver();

nit: I'd prefer if this was getActivity() - keeping a reference to mActivity mirrors a reference that already exists in the framework's super class.

In my opinion, a good reason to mirror the variable is perf (e.g. a variable access is faster than a function call).

Comment 40

[Vivek Balakrishnan[:vivek]]

Attachment: 1106900.patch

Patch with mcomella's comments addressed.

Comment 41

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8554133 [details] [diff] [review]
1106900.patch

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

Cool beans. Thanks Jalpreet and Vivek!

Comment 42

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Vivek, can you add a try push for this?

Comment 43

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Jalpreet, once the try push goes green, don't forget to add checkin-needed!

I know you have a few other bugs on your plate - would you like some suggestions for a few more?

Comment 44

[Vivek Balakrishnan[:vivek]]

Try logs:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce6143905e42

Comment 45

[Jalpreet Singh Nanda [:imjalpreet]]

Yeah, I would like some more suggestions.

Comment 46

[Carsten Book [:Tomcat]]

Thanks Jalpreet for contributing to Mozilla - i just did checkin for this change!

Comment 47

[Carsten Book [:Tomcat]]

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

Comment 48

[Jalpreet Singh Nanda [:imjalpreet]]

Now, I think we can proceed with Bug 1106903, right?

Comment 49

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/31c45bffae2a

Comment 50

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Jalpreet Singh Nanda [:imjalpreet] from comment #48)
> Now, I think we can proceed with Bug 1106903, right?

Yep, go for it!

Comment 51

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

If you see all of the bugs in the "Depends on:" field crossed out, generally that means the bug is ready to be worked on!