976775 - Rename AssertionHelper assertions to fAssert*

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Testing, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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.

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Rename AssertionHelper assertions to fAssert*.

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.

Comment 2

[Nick Alexander :nalexander [he/him]]

Why not actually override assert*, and get rid of these shenanigans?

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

[Lucas Rocha (:lucasr)]

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).

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(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

[Lucas Rocha (:lucasr)]

(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.

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(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.

Comment 8

[Lucas Rocha (:lucasr)]

(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.

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Rename AssertionHelper assertions to fAssert*.

I'll file a followup if I decide to change it. Rebased (which meant changing the assertions in testNativeCrypto and testInputConnection, two new tests).

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://hg.mozilla.org/integration/fx-team/rev/2f7dfb5095f1

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8387182 [details] [diff] [review]
Rename AssertionHelper assertions to fAssert*.

Move r+.

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2f7dfb5095f1