Closed Bug 1121651 Opened 9 years ago Closed 9 years ago

Remove static StringHelper.get references in UITest framework

Categories

(Firefox for Android Graveyard :: Testing, defect)

All
Android
defect
Not set
normal

Tracking

(firefox41 fixed)

RESOLVED FIXED
Firefox 41
Tracking Status
firefox41 --- fixed

People

(Reporter: mcomella, Assigned: d.lyons88, Mentored)

References

Details

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

Attachments

(2 files, 1 obsolete file)

bug 938845 makes StringHelper an instance variable in UITest, but other classes that do not inherit from UITest in the framework call StringHelper statically (i.e. StringHelper.get). Fix this.

This can be done by allowing access to the StringHelper instance through UITestContext [1] and adding initializers to the classes that require access to the StringHelper instance (e.g. [2]).

Instances of StringHelper.get in UITest and BaseTest will likely be available at [3] when bug 938845 lands.

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/UITestContext.java
[2]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/helpers/HelperInitializer.java#16
[3]: https://mxr.mozilla.org/mozilla-central/search?string=StringHelper.get%28&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
Hi Michael, I will take a look at this bug as a follow on from my first bug fix.
Great - thanks, Darren!
Assignee: nobody → d.lyons88
Attachment #8597538 - Flags: review?(michael.l.comella)
Not sure about this one. I submitted a patch for review happy to make further changes if it's not what you're looking for.
Comment on attachment 8597538 [details] [diff] [review]
Remove static StringHelper.get references in UITest framework

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

Looking good so far!

::: mobile/android/base/tests/components/ToolbarComponent.java
@@ +55,5 @@
>  
>          final String expected;
>          final String absoluteURL = NavigationHelper.adjustUrl(url);
>  
> +        if (mTestContext.getStringHelper().ABOUT_HOME_URL.equals(absoluteURL)) {

To reduce the verbosity of these calls, you should store StringHelper as a member variable.
Attachment #8597538 - Flags: review?(michael.l.comella) → feedback+
Attachment #8597538 - Attachment is obsolete: true
Attachment #8599661 - Flags: review?(michael.l.comella)
New patch submitted storing StringHelper as a member variable on the base component.
Comment on attachment 8599661 [details] [diff] [review]
Remove static StringHelper.get references in UITest framework

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

Nice!
Attachment #8599661 - Flags: review?(michael.l.comella) → review+
I made a push to our try test servers (see above).

Once it goes green, feel free to add the checkin-needed keyword [1]. Let me know if you need help reading the results. Note that all patches that use "checkin-needed" must also have an associated green try run.

[1]: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Getting_the_patch_checked_into_the_tree
Hey, Darren - the test results look green here, can you add the "checkin-needed" flag?
Flags: needinfo?(d.lyons88)
Given the rebase issues, I'm just going to land this myself instead of using "checkin-needed".
Flags: needinfo?(d.lyons88)
Thanks, forgot about the checkin-needed flag. Sorry about that.
https://hg.mozilla.org/mozilla-central/rev/74b944315521
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
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

Creator:
Created:
Updated:
Size: