1106903 - Move BaseTest/UITest.runTest into BaseRobocopTest

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Testing, defect)

Description

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

The method is duplicated in both sub-classes.

Comment 1

[Jalpreet Singh Nanda [:imjalpreet]]

I can't find runTest in the UITest.java. I think it is present only in BaseTest.java.

Comment 2

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

(In reply to Jalpreet Singh Nanda from comment #1)
> I can't find runTest in the UITest.java. I think it is present only in
> BaseTest.java.

That is currently correct - however, if you look above these comments to the "Depends on" field, you'll see bugs that have to land first in order for this bug to be completed. bug 1105522 is listed there and its patch adds a runTest to UITest.

The reason it has not landed yet is because it actually breaks a few different tests (marked as "Depends On" in that bug).

Once that dependency is fixed, you should get an email from Bugzilla notifying you of the dependency change.

Comment 3

[Jalpreet Singh Nanda [:imjalpreet]]

So, now as the bug 1105522 is fixed the method runTest is also implemented in UITest. So now we just need to move this method to BaseRobocopTest right?

Comment 4

[Vivek Balakrishnan[:vivek]]

@Jalpreet: Yes, you just need to move this method to common super class i.e BaseRobocopTest. I see you have solved similar issue in 1107253.

Thanks for your effort and happy coding.

Comment 5

[Jalpreet Singh Nanda [:imjalpreet]]

Attachment: Bug-1106903-Proposed_Patch

Please assign this bug to me as I have made the patch for the required changes. Please tell if more changes are to be done or not.

Comment 6

[Vivek Balakrishnan[:vivek]]

Comment on attachment 8540001 [details] [diff] [review]
Bug-1106903-Proposed_Patch

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

Hi Jalpreet,

Good effort. Unfortunately this bug is blocked by #1106900. Sorry, I should have informed you earlier.

::: mobile/android/base/tests/BaseRobocopTest.java
@@ +117,5 @@
>          }
>          mAsserter.setLogFile(mLogFile);
>          mAsserter.setTestName(getClass().getName());
>  
> +        //Set up Robotium.solo

nit: space between // and S. Also, comments must end with a '.'

For your future reference, please find android coding guidelines here
https://wiki.mozilla.org/Mobile/Fennec/Android#Coding_Style

You patch to checkin time will be greatly reduced if you follow this guidleline

Comment 7

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

Comment on attachment 8540001 [details] [diff] [review]
Bug-1106903-Proposed_Patch

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

I'll leave this to Vivek for now.

Comment 8

[Vivek Balakrishnan[:vivek]]

Comment on attachment 8540001 [details] [diff] [review]
Bug-1106903-Proposed_Patch

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

Ni Jalpreet,

The patch you submitted would fail compilation. see below for review.

::: mobile/android/base/tests/BaseRobocopTest.java
@@ +54,5 @@
>  
>      protected Assert mAsserter;
>      protected String mLogFile;
>  
> +    protected Solo mSolo;

Please add import for Solo.

@@ +118,5 @@
>          mAsserter.setLogFile(mLogFile);
>          mAsserter.setTestName(getClass().getName());
>  
> +        //Set up Robotium.solo
> +        mSolo = new Solo(getInstrumentation(), mActivity);

There is no reference to mActivity in BaseRobocopTest.

Ideally, these errors would be taken care by bug 1106900

Comment 9

[Vivek Balakrishnan[:vivek]]

Hey, Jalpreet,

I've notified the contributor on bug 1106900 - if he doesn't respond in 24 hours, would you like to be assigned to that dependent bug?

Comment 10

[Jalpreet Singh Nanda [:imjalpreet]]

Yes, I would like to be assigned on that bug.

Comment 11

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

Still working on this, Jalpreet?

Comment 12

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

Unassigning due to inactivity.

Comment 13

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

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

The code 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/tests/browser/robocop 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).

If you need any help, you can reply to this bug, or feel free to message me on IRC - my nick is "mcomella" and you can find me in #mobile. If you need IRC setup instructions, see https://wiki.mozilla.org/IRC

Thanks and happy coding! ^_^

Comment 14

[Alex Johnson (:alex_johnson)]

Attachment: MozReview Request: Bug 1106903 - Move BaseTest/UITest.runTest into BaseRobocopTest. r?mcomella

Bug 1106903 - Move BaseTest/UITest.runTest into BaseRobocopTest. r?mcomella

Comment 15

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

Comment on attachment 8683786 [details]
MozReview Request: Bug 1106903 - Move BaseTest/UITest.runTest into BaseRobocopTest. r?mcomella

https://reviewboard.mozilla.org/r/24423/#review21983

lgtm w/ green try run.

If you want something a little tricky to work on but high visibility, perhaps bug 1208577? If you want something simpler but equally high visibility, may I recommend bug 1203345?

Comment 16

[Pulsebot]

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

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/e4fea01b76e9