Closed Bug 1125449 Opened 6 years ago Closed 6 years ago

Remove mActivity from BaseRobocopTest

Categories

(Firefox for Android :: Testing, defect)

All
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 38

People

(Reporter: mcomella, Assigned: alexxdim94, Mentored)

References

Details

(Whiteboard: [lang=java][good second bug])

Attachments

(1 file, 1 obsolete file)

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).
Hey, it's me again. :) I can see the mActivity variable in BaseTest, not in BaseRobocopTest. What am I missing?
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!).
Flags: needinfo?(alexxander.dimitrov)
> 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!
Flags: needinfo?(alexxander.dimitrov)
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
Attached patch bug-1125449-fix.diff (obsolete) — Splinter Review
I ran the following tests: testNewTab, testOSLocale, testPasswordProvider, testPrefsObserver, testPromptGridInput. All ran fine and returned 0 unexpected results.
Attachment #8557059 - Flags: review?(michael.l.comella)
Do you want me to do a try-server push since I have L1 access now. :)
Assignee: nobody → alexxander.dimitrov
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 :)
Attachment #8557059 - Flags: feedback+
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().
Attachment #8557059 - Flags: review?(michael.l.comella) → review+
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. :)
Attachment #8557059 - Attachment is obsolete: true
Flags: needinfo?(michael.l.comella)
Attachment #8558676 - Flags: review?(michael.l.comella)
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.
Attachment #8558676 - Flags: review?(michael.l.comella) → review+
Thanks again!
Flags: needinfo?(michael.l.comella) → needinfo?(alexxander.dimitrov)
Flags: needinfo?(alexxander.dimitrov)
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/06587e963dfc
Keywords: checkin-needed
Whiteboard: [lang=java][good second bug] → [lang=java][good second bug][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/06587e963dfc
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [lang=java][good second bug][fixed-in-fx-team] → [lang=java][good second bug]
Target Milestone: --- → Firefox 38
You need to log in before you can comment on or make changes to this bug.