Closed Bug 976775 Opened 7 years ago Closed 7 years ago

Rename AssertionHelper assertions to fAssert*

Categories

(Firefox for Android :: Testing, defect)

All
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 30

People

(Reporter: mcomella, Assigned: mcomella)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
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: nobody → michael.l.comella
Status: NEW → ASSIGNED
Why not actually override assert*, and get rid of these shenanigans?
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 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+
(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?
(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.
(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.
Flags: needinfo?(lucasr.at.mozilla)
(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)
I'll file a followup if I decide to change it. Rebased (which meant changing the assertions in testNativeCrypto and testInputConnection, two new tests).
Attachment #8381732 - Attachment is obsolete: true
Comment on attachment 8387182 [details] [diff] [review]
Rename AssertionHelper assertions to fAssert*.

Move r+.
Attachment #8387182 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/2f7dfb5095f1
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
You need to log in before you can comment on or make changes to this bug.