1125449 - Remove mActivity from BaseRobocopTest

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Testing, defect)

Description

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

The base Android test classes provide our overridden test classes (e.g. BaseTest, UITest, BaseRobocopTest) with getActivity() to get a reference to the Activity to test. After bug 1106900 lands, BaseRobocopTest keeps a mirror reference to this value as mActivity. This is nitty, but this (duplication) is not ideal on principle because:

  1) The reference to getActivity() could change, invalidating our mirrored references (in practice, we don't expect the activity we're testing to change during the test, so it doesn't happen)
  2) Since getActivity() initializes the test activity (and we may have other setup to do first), mActivity must always be initialized second - an issue came up in the past regarding this and is possible to come up again
  3) (negligibly) the duplicate mActivity reference takes up additional space

The tests you would be fixing are in our Robocop test framework, so you'll need to set that up: 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).

Comment 1

[Alexander Dimitrov [:alexxdim94]]

Hey, it's me again. :) I can see the mActivity variable in BaseTest, not in BaseRobocopTest. What am I missing?

Comment 2

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

Hey, Alexander - do you want to be assigned to this bug?

(In reply to Alexander Dimitrov [:alexxdim94] from comment #1)
> Hey, it's me again. :) I can see the mActivity variable in BaseTest, not in
> BaseRobocopTest. What am I missing?

Seems fine to me (i.e. [1]). Perhaps your local checkout is out-of-date? Try `hg pull && hg update` (make sure your working directory is clean and you've `hg qpopped` any patches first though!).

Comment 3

[Alexander Dimitrov [:alexxdim94]]

> Hey, Alexander - do you want to be assigned to this bug?
> 
> (In reply to Alexander Dimitrov [:alexxdim94] from comment #1)
> > Hey, it's me again. :) I can see the mActivity variable in BaseTest, not in
> > BaseRobocopTest. What am I missing?
> 
> Seems fine to me (i.e. [1]). Perhaps your local checkout is out-of-date? Try
> `hg pull && hg update` (make sure your working directory is clean and you've
> `hg qpopped` any patches first though!).

Yeah, my local checkout was not up to date. So, I should remove the mActivity field and replace any references in BaseRobocopTest to it with "getActivity()". Right? Then I should recompile and run all robocop tests to see if everything's fine?

P.S. Yes, I'd like to be assigned to this bug. :) Thanks!

Comment 4

[Vivek Balakrishnan[:vivek]]

Hi Alexander,

Yes, you should remove the mActivity field and replace the reference with getActivity() in BaseRobocopTest.
Do note that we use this inherited field in BaseTest as well. All those reference should also be replaced.

Running full the robocop test would be time consuming (appro. 30 mins). As Michael recommended run some of those tests to verify that your changes doesn't break things.

happy coding

Comment 5

[Alexander Dimitrov [:alexxdim94]]

Attachment: bug-1125449-fix.diff

I ran the following tests: testNewTab, testOSLocale, testPasswordProvider, testPrefsObserver, testPromptGridInput. All ran fine and returned 0 unexpected results.

Comment 6

[Alexander Dimitrov [:alexxdim94]]

Do you want me to do a try-server push since I have L1 access now. :)

Comment 7

[Vivek Balakrishnan[:vivek]]

Comment on attachment 8557059 [details] [diff] [review]
bug-1125449-fix.diff

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

Hi Alexander,
Thanks for your effort. 
your change looks good to me.
I'll wait for Comella's review.
In mean time, it would be great if you can schedule a try server push :)

Comment 8

[Alexander Dimitrov [:alexxdim94]]

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

Here's the try-server push results. :)

Comment 9

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

Comment on attachment 8557059 [details] [diff] [review]
bug-1125449-fix.diff

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

r+ w/ nits - this means upload another patch to address the review comments, but provided you explain why you don't want to correct the nits, or correct them, I don't need to see the next patch (already r+'d). Don't worry about adding another try run either.

Thanks, Alexander!

Let me know if you want more follow-up bug suggestions.

::: mobile/android/base/tests/BaseRobocopTest.java
@@ +142,5 @@
>  
>          // Set up Robotium.solo and Driver objects
> +        mSolo = new Solo(getInstrumentation(), getActivity());
> +        mDriver = new FennecNativeDriver(getActivity(), mSolo, mRootPath);
> +        mActions = new FennecNativeActions(getActivity(), mSolo, getInstrumentation(), mAsserter);

nit: Use a temporary variable here for getActivity().

Comment 10

[Alexander Dimitrov [:alexxdim94]]

Attachment: bug-1125449-fix.diff

Thanks guys! :) Here's the try-server push results:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7799c010989

I'm a bit concerned by the high amount of retries, but I'm not sure if that's a problem. :)

Comment 11

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

Comment on attachment 8558676 [details] [diff] [review]
bug-1125449-fix.diff

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

lgtm. Don't forget to add "checkin-needed"!

(In reply to Alexander Dimitrov [:alexxdim94] from comment #10)
> I'm a bit concerned by the high amount of retries, but I'm not sure if
> that's a problem. :)

Blue runs are infrastructure errors - e.g. device failures, bad network - which is nothing your code has any control over so don't worry about the failures. The orange runs are test failures - i.e. an assertion was thrown. However, the code you changed shouldn't affect the failures that took place in the orange runs in your try link so we're good here.

By the way, if you're working on Firefox for Android chrome, you generally only need to run Robocop when pushing try runs - everything else is (usually) affected by platform code.

Also, when flagging a reviewer, you only need an r? flag, not a NEEDINFO too.

Comment 12

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

Thanks again!

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/06587e963dfc

Comment 14

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

https://hg.mozilla.org/mozilla-central/rev/06587e963dfc