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)
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)
5.23 KB,
patch
|
mcomella
:
review+
|
Details | Diff | Splinter Review |
5.40 KB,
patch
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•9 years ago
|
||
https://mail.mozilla.org/pipermail/mobile-firefox-dev/2015-February/001090.html
Whiteboard: [lang=java][good second bug] → [lang=java][good next bug]
Assignee | ||
Comment 2•9 years ago
|
||
Hi Michael, I will take a look at this bug as a follow on from my first bug fix.
Assignee | ||
Comment 4•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8597538 -
Flags: review?(michael.l.comella)
Assignee | ||
Comment 5•9 years ago
|
||
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.
Reporter | ||
Comment 6•9 years ago
|
||
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+
Assignee | ||
Comment 7•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8597538 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8599661 -
Flags: review?(michael.l.comella)
Assignee | ||
Comment 8•9 years ago
|
||
New patch submitted storing StringHelper as a member variable on the base component.
Reporter | ||
Comment 9•9 years ago
|
||
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+
Reporter | ||
Comment 10•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f17298b4e85b
Reporter | ||
Comment 11•9 years ago
|
||
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
Reporter | ||
Comment 12•9 years ago
|
||
Hey, Darren - the test results look green here, can you add the "checkin-needed" flag?
Flags: needinfo?(d.lyons88)
Reporter | ||
Comment 13•9 years ago
|
||
Reporter | ||
Updated•9 years ago
|
Attachment #8599661 -
Attachment is obsolete: true
Reporter | ||
Updated•9 years ago
|
Attachment #8599661 -
Attachment is obsolete: false
Reporter | ||
Comment 14•9 years ago
|
||
Given the rebase issues, I'm just going to land this myself instead of using "checkin-needed".
Flags: needinfo?(d.lyons88)
Reporter | ||
Comment 15•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/74b944315521
Reporter | ||
Comment 16•9 years ago
|
||
Thanks, Darren! :)
Assignee | ||
Comment 17•9 years ago
|
||
Thanks, forgot about the checkin-needed flag. Sorry about that.
Reporter | ||
Comment 18•9 years ago
|
||
No worries, it happens!
https://hg.mozilla.org/mozilla-central/rev/74b944315521
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•