944165 - Implement full JUnit assert*(String, ...) API in AssertionHelper

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Testing, defect)

Description

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

Sorry, a bit tedious, but necessary. :)

Alternatively, this can be (and likely will be) done as we go along.

Comment 1

[Isura Edirisinghe]

I'm starting to work on this. To confirm, this is the API you want to mimic?

http://junit.sourceforge.net/javadoc/org/junit/Assert.html

Comment 2

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

Thanks for clarifying - that looks ideal!

We want to enforce good testing practice, however, so only include the methods that have the `String message` parameter. Also, the deprecated methods are unnecessary, as are the assertThat methods since we don't have any intention of supporting hamcrest matchers.

Good luck!

Comment 3

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

Hey Isura, are you still working on this?

Comment 4

[Isura Edirisinghe]

Hi Michael,

Yes I'm been terribly busy during the holidays. I will get some time to work on it this week.

Comment 5

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

No worries, take your time! I just wanted to make sure that you hadn't decided to stop working on it (in which case, I'd want to unassign it so others can have a chance). As long as you are still committed to working on it, there's no rush!

And as always, thanks for your help! :)

Comment 6

[Isura Edirisinghe]

Attachment: 944165.patch

Comment 7

[Isura Edirisinghe]

Please disregard. I will regenerate the patch when I get home later today.

Comment 8

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

Comment on attachment 8358619 [details] [diff] [review]
944165.patch

Sure, will do.

Comment 9

[Isura Edirisinghe]

Attachment: 944165.patch

Comment 10

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

Comment on attachment 8358739 [details] [diff] [review]
944165.patch

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

This is going in the right direction: just make sure you're using all the available APIs at your disposal!

::: mobile/android/base/tests/helpers/AssertionHelper.java
@@ +22,5 @@
>          sAsserter = context.getAsserter();
>      }
>  
> +    public static void assertArrayEquals(final String message, final byte[] expecteds, final byte[] actuals) {
> +        assertEquals(message, expecteds.length, actuals.length);

Sorry to not mention this before, but Java has a built-in `Arrays.equals` method [1]. Do you mind using that instead? It's less for us to maintain (and may catch some edge cases we don't see here). There should be a version for each of the array types we use below.

[1]: https://developer.android.com/reference/java/util/Arrays.html#equals%28char[],%20char[]%29

@@ +24,5 @@
>  
> +    public static void assertArrayEquals(final String message, final byte[] expecteds, final byte[] actuals) {
> +        assertEquals(message, expecteds.length, actuals.length);
> +        for (int i = 0; i < expecteds.length(); i++) {
> +            assertEquals(message, expecteds[i], actuals[i]);

We should be using a class implementing the Assert interface [1] for all of these calls, rather than assertEquals which adds a level of indirection and makes the resulting assertion failure harder to read. The sAsserter statically defined in this class implements it. Specifically, we'll probably be using the `Assert.is`, `Assert.ok`, and `Assert.isnot` methods.

If you look at the already existing calls, they are a good example.

[1]: https://mxr.mozilla.org/mozilla-central/source/build/mobile/robocop/Assert.java#7

@@ +35,5 @@
> +            assertEquals(message, expecteds[i], actuals[i]);
> +        }
> +    }
> +
> +    public static void assertArrayEquals(final String message, final short[] expecteds, final short[] actuals) {

nit: A bit pedantic on my part, but can you move this below assertArrayEquals(String, Object[], ...)? It's not alphabetized. :P

@@ +72,5 @@
>      public static void assertEquals(final String message, final Object expected, final Object actual) {
>          sAsserter.is(actual, expected, message);
>      }
>  
> +    public static void assertEquals(final String message, final double expected, final double actual, final double delta) {

Good work on this method - there are a lot of cases implemented here I wasn't thinking of!

However, there is also a `Double.compare` method [1] that we can use that takes care of a lot of this (keep in mind it returns an int, like `Comparable.compareTo` and it has some edge cases, see [1]). It doesn't have the delta comparison though, so we'll still need to do that ourselves - the code below looks good for that.

[1]: https://developer.android.com/reference/java/lang/Double.html#compare%28double,%20double%29

@@ -25,5 @@
>      public static void assertEquals(final String message, final Object expected, final Object actual) {
>          sAsserter.is(actual, expected, message);
>      }
>  
> -    public static void assertEquals(final String message, final int expected, final int actual) {

Why did you remove the int version?

@@ +79,5 @@
> +        }
> +        if (Double.isInfinite(expected)) {
> +            assertTrue(message, expected == actual);
> +        }
> +        else {

nit: `} else {`

Comment 11

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

> Why did you remove the int version?

Nevermind, I noticed there's a replacement long version. Nice catch. :)

Comment 12

[Isura Edirisinghe]

Attachment: 944165.patch

Comment 13

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

Comment on attachment 8359406 [details] [diff] [review]
944165.patch

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

lgtm! Don't forget to checkin-needed!

Thanks for your help!

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/de2899c82c0d