Move mBaseUrl and mRawBaseUrl from BaseTest into BaseRobocopTest

RESOLVED FIXED in Firefox 37

Status

()

Firefox for Android
Testing
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mcomella, Assigned: imjalpreet, Mentored)

Tracking

unspecified
Firefox 37
All
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 1 obsolete attachment)

mBaseHostnameUrl and mBaseIpUrl in UITest [1] and mBaseUrl and mRawBaseUrl in BaseTest [2] are the same values. Thus, these can be moved into their parent class, BaseRobocopTest.

Note that the variable names in BaseTest are not as descriptive as the ones in UITest so the names in UITest should be used in BaseRobocopTest.

I highly recommend using an IDE (e.g. [3]) for this refactoring.

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/UITest.java?rev=63061f730d01#71
[2]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/BaseTest.java?rev=217c7f7d9f48#116
[3]: http://www.ncalexander.net/blog/2014/10/23/building-fennec-with-gradle-and-intellij-first-steps/
(Assignee)

Comment 1

4 years ago
Hi, I am new into this and I would like to take this bug as I think it's not assigned to anybody.If it's assigned to me, I want to ask do we only have to move the mBaseUrl and mRawBaseUrl to it's parent class, i.e, BaseRobocopTest or do I have to do something more than that? Secondly, then I would have to change the variable names to mBaseHostnameUrl and mBaseIpUrl in BaseTest, right?
Hey, Jalpreet! Welcome to Bugzilla! :) You've been assigned!

Have you already set up a build environment? 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, to test your changes, 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

To answer your question...

(In reply to Jalpreet Singh Nanda from comment #1)
> I want to ask do we only have to move the mBaseUrl and mRawBaseUrl to it's
> parent class, i.e, BaseRobocopTest or do I have to do something more than
> that?

Sorry if I was unclear - mBaseHostnameUrl/mBaseIpUrl in UITest and mBaseUrl/mRawBaseUrl in BaseTest are instantiated the same way in both classes and thus have the same value, i.e.:

UITest:
  mBaseHostnameUrl = mConfig.get("host").replaceAll("(/$)", "");
  mBaseIpUrl = mConfig.get("rawhost").replaceAll("(/$)", "");

BaseTest:
  mBaseUrl = mConfig.get("host").replaceAll("(/$)", "");
  mRawBaseUrl = mConfig.get("rawhost").replaceAll("(/$)", "");

Therefore, we are duplicating code to do this in each class when they share a common super class, BaseRobocopTest, that can do this work. So you only need move mBaseHostnameUrl to the parent class and...

> Secondly, then I would have to change the variable names to
> mBaseHostnameUrl and mBaseIpUrl in BaseTest, right?

Right - any expression that uses BaseTest's mBaseUrl and mRawBaseUrl should be changed to use mBaseHostnameUrl and mBaseIpUrl instead (respectively).

To summarize, if using an IDE, I recommend taking the following steps (in this order):
  1) Rename BaseTest.mBaseUrl to BaseTest.mBaseHostnameUrl and BaseTest.mRawBaseUrl to BaseTest.mBaseIpUrl
  2) Move UITest/BaseTest.mBaseHostnameUrl and UITest/BaseTest.mBaseIpUrl to the super class, BaseRobocopTest.

Thanks and happy coding! ^_^
Assignee: nobody → jalpreetnanda
Status: NEW → ASSIGNED
(Assignee)

Comment 3

4 years ago
Created attachment 8534057 [details] [diff] [review]
Bug-1107253-Proposed_Patch

I have done the refactoring as suggested by you.
Attachment #8534057 - Flags: review?(michael.l.comella)
Comment on attachment 8534057 [details] [diff] [review]
Bug-1107253-Proposed_Patch

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

Looking pretty good! Just a few small changes and you'll all be set to go!

Here is a run of your patch on our Try test servers: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ed9f09871723

Note that you'll always need a green run in order to check-in your patch through the "checkin-needed" keyword (see [1]).

[1]: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Getting_the_patch_checked_into_the_tree

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

nit: Usually, statements are separated by whitespace because of their relation to the lines around them. These new Strings don't have much to do with mAssert/mLogFile, nor should they be joining mConfig/mRootPath with those Strings - can you add some whitespace above and below the Strings you added here?

@@ +112,5 @@
>              mAsserter = new FennecMochitestAssert();
>          }
>          mAsserter.setLogFile(mLogFile);
>          mAsserter.setTestName(getClass().getName());
> +        mBaseHostnameUrl = mConfig.get("host").replaceAll("(/$)", "");

nit: Similar - can you add whitespace above this line?

::: mobile/android/base/tests/BaseTest.java
@@ +77,5 @@
>      protected Solo mSolo;
>      protected Driver mDriver;
>      protected Actions mActions;
> +    //protected String mBaseUrl;
> +    //protected String mRawBaseUrl;

Instead of commenting out these lines to remove them, just remove them! If we need them again, we have version control. Same with the instantiation and the equivalent UITest code.
Attachment #8534057 - Flags: review?(michael.l.comella) → feedback+
(Assignee)

Comment 5

4 years ago
Created attachment 8534619 [details] [diff] [review]
Bug-1107253-Proposed_Patch

I have done the changes as suggested by you.
Attachment #8534619 - Flags: review?(michael.l.comella)
Comment on attachment 8534619 [details] [diff] [review]
Bug-1107253-Proposed_Patch

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

Great, looks good to me!

It looks like the try push ran into a few intermittent issues that our test server has so I've retrigger the failing jobs. When that goes green, feel free to add the "checkin-needed" keyword [1] to get your patch landed. Alternatively, if you need me to retrigger more jobs, let me know (unfortunately, it happens sometimes :\).

If you're looking for a good followup bug, I think bug 938845 would be straight-forward (but perhaps tedious D: !), while bug 926234 requires a bit of SQL knowledge to help untangle our database and bug 1108084 requires digging into our favicon code. If none of these work for you, let me know!

One tip: when you upload a new patch, it is best practice to mark your older patches as obsolete so it is obvious which patches are final and fix the bug. To do this, go to the "Attachments" table, click "Details", click "edit details" (near the patch name), click the "obsolete" checkbox that should have appeared, and hit "Submit" at the bottom!

Thanks for the patch!

[1]: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Getting_the_patch_checked_into_the_tree
Attachment #8534619 - Flags: review?(michael.l.comella) → review+
(Assignee)

Comment 7

4 years ago
Thanks, I will look into these bugs.
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(Assignee)

Updated

4 years ago
Attachment #8534057 - Attachment is obsolete: true
https://hg.mozilla.org/integration/fx-team/rev/1558ea234fb2
Keywords: checkin-needed
Whiteboard: [lang=java][good first bug] → [lang=java][good first bug][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/1558ea234fb2
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [lang=java][good first bug][fixed-in-fx-team] → [lang=java][good first bug]
Target Milestone: --- → Firefox 37
You need to log in before you can comment on or make changes to this bug.