Closed Bug 944165 Opened 8 years ago Closed 8 years ago

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

Categories

(Firefox for Android Graveyard :: Testing, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 29

People

(Reporter: mcomella, Assigned: isurae)

References

Details

(Whiteboard: [mentor=mcomella][lang=java])

Attachments

(1 file, 2 obsolete files)

Sorry, a bit tedious, but necessary. :)

Alternatively, this can be (and likely will be) done as we go along.
Whiteboard: [mentor=mcomella][lang=java]
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
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!
Assignee: nobody → isurae
Status: NEW → ASSIGNED
Hey Isura, are you still working on this?
Flags: needinfo?(isurae)
Hi Michael,

Yes I'm been terribly busy during the holidays. I will get some time to work on it this week.
Flags: needinfo?(isurae)
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! :)
Attached patch 944165.patch (obsolete) — Splinter Review
Attachment #8358619 - Flags: review?(michael.l.comella)
Please disregard. I will regenerate the patch when I get home later today.
Comment on attachment 8358619 [details] [diff] [review]
944165.patch

Sure, will do.
Attachment #8358619 - Flags: review?(michael.l.comella)
Attached patch 944165.patch (obsolete) — Splinter Review
Attachment #8358619 - Attachment is obsolete: true
Attachment #8358739 - Flags: review?(michael.l.comella)
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 {`
Attachment #8358739 - Flags: review?(michael.l.comella) → feedback+
> Why did you remove the int version?

Nevermind, I noticed there's a replacement long version. Nice catch. :)
Attached patch 944165.patchSplinter Review
Attachment #8358739 - Attachment is obsolete: true
Attachment #8359406 - Flags: review?(michael.l.comella)
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!
Attachment #8359406 - Flags: review?(michael.l.comella) → review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/de2899c82c0d
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [mentor=mcomella][lang=java] → [mentor=mcomella][lang=java][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/de2899c82c0d
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [mentor=mcomella][lang=java][fixed-in-fx-team] → [mentor=mcomella][lang=java]
Target Milestone: --- → Firefox 29
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.