943703 - Create helpers.HelperInitializer for UITests

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Testing, defect)

Description

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

Some classes import AssertionHelper statically which imports its init method. The access level on init should be changed to protected and a new initializer class should be added to the package to call its init method.

To be consistent it should call all of the helpers init methods.

Then UITest.initHelpers should call this new classes' init method instead.

Comment 1

[Isura Edirisinghe]

Attachment: 943703.patch

Is this close to what you had in mind?

Comment 2

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

Comment on attachment 8346679 [details] [diff] [review]
943703.patch

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

Exactly what I was thinking! Good work!

Just elaborate on that comment, fix those nits, and I'll give you that r+!

By the way, when you want to work on a bug, you should leave a comment to be assigned so that someone else doesn't start working on the bug while you're working on it.

::: mobile/android/base/tests/helpers/HelperInitializer.java
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public 

nit: whitespace.

@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public 
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +package org.mozilla.gecko.tests.helpers; 

nit: whitespace

@@ +6,5 @@
> +
> +import org.mozilla.gecko.tests.UITestContext;
> +
> +/**
> + * Helper that initializes the other helper classes.

Additionally explain why we're doing this this way (i.e. that we want to statically import AssertionHelper for assertions, but that means we import the init method too).

@@ +8,5 @@
> +
> +/**
> + * Helper that initializes the other helper classes.
> + */
> +

nit: rm newline.

Comment 3

[Isura Edirisinghe]

Can you explain the whitespace issue?

Comment 4

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

Sure. Many developers are bothered by any excess, missing, or inconsistent whitespace - this is in the form of newlines, spaces between various syntactic elements (e.g. `if (myVar  == true){` as opposed to the common style `if (mVar == true) {`), and extra spaces at the end of a line.

In this case, I'm commenting about end of line whitespace. Honestly, it can be a bit over obsessive to care about these details but there are some (appropriately over-obsessive) reasons for it:
  * File size: extra whitespace means more disk usage. When you add in thousands of developers checking in their changes, this can add up (though still is honestly fairly negligible)
  * Version control: It's easy to add extra whitespace to a line you didn't mean to, which will change the blame commits to your name! Being diligent about removing whitespace means this shouldn't accidentally happen
  * Ease of editing: Certain keyboard-centric editors (e.g. vim, emacs) will respond unpredictably to unseen whitespace so it's nice to respect those users
  * (This one can be considered a stretch, but is my favorite anyway) Discipline: It's part of your coding style. If you can't maintain whitespace, how can you hope to maintain your if's, &&'s, and ||'s? By practicing a good and comprehensive coding style that specifically pays attention to details, you're more likely to stay on your toes and catch similarly minute details that could be flawed in your code's logic!

If you go into the review tool here on Bugzilla, you'll notice this space highlights in red. Many editors and source control systems can also be set to highlight (or automatically remove) these spaces too. We avoid automatically removing them because that could create a lot of source control blame churn. Linters will also often tell you about these details.

In reviews, they are often marked with "nit:" because they're very obsessive details that shouldn't always hold up an r+ (to save the reviewer some time for a second pass) but should probably be fixed anyway.

Comment 5

[Isura Edirisinghe]

Attachment: 943703.patch

Fixed issues from review.

Comment 6

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

Comment on attachment 8346740 [details] [diff] [review]
943703.patch

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

::: mobile/android/base/tests/helpers/HelperInitializer.java
@@ +7,5 @@
> +import org.mozilla.gecko.tests.UITestContext;
> +
> +/**
> + * AssertionHelper is statically imported in many places. Thus we want to make
> + * its init method hidden from clients. For consistently we initialize all the

nit: consistently -> consistency

Also, before, "For consistency...", I would add, "By calling init within the same package, we can make it protected and thus not imported." or something similar.

Comment 7

[Isura Edirisinghe]

Attachment: 943703.patch

Fix comment.

Comment 8

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

Comment on attachment 8346754 [details] [diff] [review]
943703.patch

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

::: mobile/android/base/tests/helpers/HelperInitializer.java
@@ +8,5 @@
> +
> +/**
> + * AssertionHelper is statically imported in many places. Thus we want to make
> + * its init method hidden from clients. We initialize the remaining helper
> + * classes from here so that all the init methods can be hidden from clients.

Sorry I'm being so pedantic, but this still doesn't quite get the thought the idea I wanted to add. Essentially, I want to add *why* having this HelperInitializer allows us to hide the init method from those importing classes (i.e. package protection).

Also, the term "client" usually refers more to outside users, usually multiple, trying to obtain some resource from us. While this is true, it's a stretch. I see there's no real good word to replace it (maybe "importing classes" but that's icky), but worded correctly, I think you can avoid using one (e.g. "AssertionHelper is statically imported in many places and so we want to make its init method hidden outside of this package.").

I would r+ w/ just the top change though.

We can do this back and forth on IRC if you want to make it go a little faster.

Comment 9

[Isura Edirisinghe]

/**
 * AssertionHelper is statically imported in many places. Thus we want to hide
 * its init method outside of this package. We initialize the remaining helper
 * classes from here so that all the init methods are package protected.
 */

How is that?

Comment 10

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

lgtm! Put it in the patch! :)

Comment 11

[Isura Edirisinghe]

Attachment: 943703.patch

Fixed comment.

Comment 12

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

Comment on attachment 8347217 [details] [diff] [review]
943703.patch

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

lgtm! Thanks for you help!

Comment 13

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Patch fails to apply on m-c changeset c0f85061c7d3:

patching file mobile/android/base/tests/UITest.java
Hunk #2 FAILED at 115
1 out of 2 hunks FAILED -- saving rejects to file mobile/android/base/tests/UITest.java.rej
unable to find 'mobile/android/base/tests/helpers/GestureHelper.java' for patching
1 out of 1 hunks FAILED -- saving rejects to file mobile/android/base/tests/helpers/GestureHelper.java.rej

Please upload a rebased patch. Thanks!

Comment 14

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

Attachment: Moved helper initialization to a separate static function.

Comment 15

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

Rebased patch and pushed: https://hg.mozilla.org/integration/fx-team/rev/96088a8ba738

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/96088a8ba738