Closed Bug 1106903 Opened 5 years ago Closed 4 years ago

Move BaseTest/UITest.runTest into BaseRobocopTest

Categories

(Firefox for Android :: Testing, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 45
Tracking Status
firefox45 --- fixed

People

(Reporter: mcomella, Assigned: alex_johnson, Mentored)

References

Details

(Whiteboard: [lang=java][good first bug][see comment 13])

Attachments

(2 files)

The method is duplicated in both sub-classes.
I can't find runTest in the UITest.java. I think it is present only in BaseTest.java.
Flags: needinfo?(michael.l.comella)
(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.
Flags: needinfo?(michael.l.comella)
Mentor: vivekb.balakrishnan
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?
Flags: needinfo?(vivekb.balakrishnan)
Flags: needinfo?(michael.l.comella)
@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.
Flags: needinfo?(vivekb.balakrishnan)
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.
Attachment #8540001 - Flags: review?(vivekb.balakrishnan)
Attachment #8540001 - Flags: review?(michael.l.comella)
Assignee: nobody → jalpreetnanda
Status: NEW → ASSIGNED
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 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.
Attachment #8540001 - Flags: review?(michael.l.comella)
Flags: needinfo?(michael.l.comella)
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
Attachment #8540001 - Flags: review?(vivekb.balakrishnan) → feedback+
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?
Flags: needinfo?(jalpreetnanda)
Yes, I would like to be assigned on that bug.
Flags: needinfo?(jalpreetnanda)
Still working on this, Jalpreet?
Flags: needinfo?(jalpreetnanda)
Unassigning due to inactivity.
Assignee: jalpreetnanda → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(jalpreetnanda)
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! ^_^
Whiteboard: [lang=java][good first bug] → [lang=java][good first bug][see comment 13]
Bug 1106903 - Move BaseTest/UITest.runTest into BaseRobocopTest. r?mcomella
Attachment #8683786 - Flags: review?(michael.l.comella)
Assignee: nobody → mrjohnsonalex
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?
Attachment #8683786 - Flags: review?(michael.l.comella) → review+
Keywords: checkin-needed
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/e4fea01b76e9
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
You need to log in before you can comment on or make changes to this bug.