Closed Bug 1107253 Opened 9 years ago Closed 9 years ago

Move mBaseUrl and mRawBaseUrl from BaseTest into BaseRobocopTest

Categories

(Firefox for Android Graveyard :: Testing, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 37

People

(Reporter: mcomella, Assigned: imjalpreet, Mentored)

References

Details

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

Attachments

(1 file, 1 obsolete file)

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/
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
Attached patch Bug-1107253-Proposed_Patch (obsolete) — Splinter Review
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+
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+
Thanks, I will look into these bugs.
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
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [lang=java][good first bug][fixed-in-fx-team] → [lang=java][good first bug]
Target Milestone: --- → Firefox 37
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: