Closed
Bug 976775
Opened 11 years ago
Closed 11 years ago
Rename AssertionHelper assertions to fAssert*
Categories
(Firefox for Android Graveyard :: Testing, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 30
People
(Reporter: mcomella, Assigned: mcomella)
References
Details
Attachments
(1 file, 1 obsolete file)
34.77 KB,
patch
|
mcomella
:
review+
|
Details | Diff | Splinter Review |
Short for fennec/firefoxAssert*.
Why? UITest is a subclass of JUnit's Assert class, so the JUnit assert methods are accessible by default. In building tests, I'm having collisions with these method names so sometimes AssertionHelper is called and sometimes JUnit is called; I'm not sure why.
Let's just f those assertions and avoid the hassle.
Assignee | ||
Comment 1•11 years ago
|
||
I wasn't sure whether I should change all of the assertions for consistency (e.g. ToolbarComponent.assertVisible()) so I only changed AssertionHelper. Please comment to this.
Attachment #8381732 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → michael.l.comella
Status: NEW → ASSIGNED
Comment 2•11 years ago
|
||
Why not actually override assert*, and get rid of these shenanigans?
Assignee | ||
Comment 3•11 years ago
|
||
After talking with nalexander on IRC, I decided against the idea in comment 2:
* assert* is generally not used within the tests (extending UITest) themselves, but rather the framework. `import static ...UITest.*;` is much messier than an external class and requires much more precise access levels on the members in UITest - it'd be tricky to get right (nevermind future additions where this requirement is not known). If we were to access these methods through the UITestContext instance instead, it'd be duplicated through both the context interface and the UITest method, which is gross :(
* Might screw with Robotium (though it appears they import Assert [1] directly, which I think mitigates this)
I like the idea of overriding these methods to prevent accidental usage though, so I filed bug 976833.
[1]: https://github.com/RobotiumTech/robotium/blob/master/robotium-solo/src/main/java/com/robotium/solo/Asserter.java#L3
Comment 4•11 years ago
|
||
Comment on attachment 8381732 [details] [diff] [review]
Rename AssertionHelper assertions to fAssert*.
Review of attachment 8381732 [details] [diff] [review]:
-----------------------------------------------------------------
I'm a bit concerned about the potential inconsistencies that might arise from that. So, I really think we should make bug 976833 happen.
(This is why I liked the idea of using FEST-Assert as a way to streamline our assertions everywhere. It's a shame I couldn't find a good way to integrated it with out infra).
Attachment #8381732 -
Flags: review?(lucasr.at.mozilla) → review+
Assignee | ||
Comment 5•11 years ago
|
||
(In reply to Lucas Rocha (:lucasr) from comment #4)
> I'm a bit concerned about the potential inconsistencies that might arise
> from that. So, I really think we should make bug 976833 happen.
Potential inconsistencies? What do you mean?
Comment 6•11 years ago
|
||
(In reply to Michael Comella (:mcomella) from comment #5)
> (In reply to Lucas Rocha (:lucasr) from comment #4)
> > I'm a bit concerned about the potential inconsistencies that might arise
> > from that. So, I really think we should make bug 976833 happen.
>
> Potential inconsistencies? What do you mean?
As in inconsistent API: it's not entirely obvious why we'd use assert* when calling directly from the test and fAssert* when calling from, say, a UITestComponent. It would be nice if we did assertions using the same API everywhere.
Assignee | ||
Comment 7•11 years ago
|
||
(In reply to Lucas Rocha (:lucasr) from comment #6)
> As in inconsistent API: it's not entirely obvious why we'd use assert* when
> calling directly from the test and fAssert* when calling from, say, a
> UITestComponent.
Would you prefer it if the components' assertions were changed to fAssert* as well (e.g. ToolbarComponent.fAssertVisible())? Thinking about this now, I think I prefer it so if you agree, I'll make a second patch.
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(lucasr.at.mozilla)
Comment 8•11 years ago
|
||
(In reply to Michael Comella (:mcomella) from comment #7)
> (In reply to Lucas Rocha (:lucasr) from comment #6)
> > As in inconsistent API: it's not entirely obvious why we'd use assert* when
> > calling directly from the test and fAssert* when calling from, say, a
> > UITestComponent.
>
> Would you prefer it if the components' assertions were changed to fAssert*
> as well (e.g. ToolbarComponent.fAssertVisible())? Thinking about this now, I
> think I prefer it so if you agree, I'll make a second patch.
I think it's fine to keep the components' assertions unchanged and only use the prefix for the base assertions. No strong opinion though. Up to you.
Flags: needinfo?(lucasr.at.mozilla)
Assignee | ||
Comment 9•11 years ago
|
||
I'll file a followup if I decide to change it. Rebased (which meant changing the assertions in testNativeCrypto and testInputConnection, two new tests).
Assignee | ||
Updated•11 years ago
|
Attachment #8381732 -
Attachment is obsolete: true
Assignee | ||
Comment 10•11 years ago
|
||
Assignee | ||
Comment 11•11 years ago
|
||
Comment on attachment 8387182 [details] [diff] [review]
Rename AssertionHelper assertions to fAssert*.
Move r+.
Attachment #8387182 -
Flags: review+
Comment 12•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
Updated•4 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
•