Use checkstyle to forbid JUnit 3 and some other internal packages.

[TWiStErRob]

Fix for #106 and more

Example checkstyle messages:

[ant:checkstyle] D:\...\glide\...Test.java:10:1: Import from illegal package - sun.internal.HackyReflectionClass. Programs that contain direct calls to the sun.* packages are not 100% Pure Java.
[ant:checkstyle] D:\...\glide\...Test.java:11:1: Import from illegal package - org.mockito.internal.matchers.Matches. Use org.mockito.Matchers to instantiate argument matchers; or org.hamcrest.Matchers for assertThat.
[ant:checkstyle] D:\...\glide\...Test.java:14:1: Import from illegal package - junit.framework.Assert.assertEquals. Tests are written in JUnit 4, use org.junit.* equivalent.

Example assertion errors achieved by using JUnit 4's assertThat and hamcrest-library Matchers.

To produce the error: commented out the harness.doLoad(); line

assertThat(harness.runners, hasEntry(equalTo((Key) harness.cacheKey), notNullValue(ResourceRunner.class)));

java.lang.AssertionError: 
Expected: map containing [<Mock for EngineKey, hashCode: 436418>->not null]
     but: map was []
    at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
    at org.junit.Assert.assertThat(Assert.java:865)
    at org.junit.Assert.assertThat(Assert.java:832)
    at com.bumptech.glide.load.engine.EngineTest.testRunnerIsPutInRunnersWithCacheKeyWithRelevantIds(EngineTest.java:384)

To produce the error: multiplied transformedAspectRatio by 100 making 10 -> 1000

assertThat("nearly identical aspect ratios", transformedAspectRatio, closeTo(originalAspectRatio, 0.05));

java.lang.AssertionError: Expected nearly identical aspect ratios
Expected: is a numeric value within <0.05> of <10.0>
     but: <1000.0> differed by <989.95>
    at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
    at org.junit.Assert.assertThat(Assert.java:865)
    at com.bumptech.glide.util.TransformationUtilsTest.assertHasOriginalAspectRatio(TransformationUtilsTest.java:99)
    at com.bumptech.glide.util.TransformationUtilsTest.testFitCenterWithSmallWideBitmap(TransformationUtilsTest.java:42)

To produce the error: multiplied width by 100 making 13 -> 1300

assertThat("width", width, lessThanOrEqualTo(maxSide));

java.lang.AssertionError: width
Expected: a value less than or equal to <500>
     but: <1300> was greater than <500>
    at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
    at org.junit.Assert.assertThat(Assert.java:865)
    at com.bumptech.glide.util.TransformationUtilsTest.assertBitmapFitsExactlyWithinBounds(TransformationUtilsTest.java:106)
    at com.bumptech.glide.util.TransformationUtilsTest.testFitCenterWithSmallTallBitmap(TransformationUtilsTest.java:67)

To produce the error: subtracted 1 from both width and height

assertThat("one side must match maxSide", maxSide, either(equalTo(width)).or(equalTo(height)));

java.lang.AssertionError: one side must match maxSide
Expected: (<499> or <49>)
     but: was <500>
    at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
    at org.junit.Assert.assertThat(Assert.java:865)
    at com.bumptech.glide.util.TransformationUtilsTest.assertBitmapFitsExactlyWithinBounds(TransformationUtilsTest.java:106)
    at com.bumptech.glide.util.TransformationUtilsTest.testFitCenterWithSmallWideBitmap(TransformationUtilsTest.java:41)

[sjudd]

Another nit: you can use assertThat(shadowObserver.getPreDrawListeners(), hasSIze(1)). http://hamcrest.org/JavaHamcrest/javadoc/1.3/org/hamcrest/Matchers.html#hasSize(int).

Now that I think about it, there are actually probably a bunch of places where I'm using assertTrue or assertFalse and hamcrest has better alternatives available. Assuming that's the case, don't feel obligated to fix all of that now.

[TWiStErRob]

don't feel obligated to fix all of that now.

I already did :)
As for the hasSize, I know about it, but I didn't see the right of the line, I was focused on getting consistent imports. But good point, I'll take a look at all .size() usages as well.

[sjudd]

Ah I just saw a few imports for assertTrue/assertFalse and didn't look more closely. Thanks for doing this!

[sjudd]

Awesome, looks great. I just started looking at Hamcrest yesterday after some of your previous pull requests, and it's a really nice library. This is much easier to read and it's great to have explicit error messages.

[sjudd]

If you want to try to rebase this on top of master I'll wait to merge this, otherwise if it's all set let me know and I'll hit the button.

[TWiStErRob]

Isn't it already on master?

[sjudd]

Yes, but you can make the history slightly cleaner by rebasing on top of the most recent changes. It's a git thing.

If you branch at commit A, and create commit B on your branch, but in the meantime, commits C and D are added on top of A in master:

A B
C
D

You get a branch in the commit history when you merge:

master
A
B
merge B (see e3524cc)
C
D

If you instead rebase and then merge, you get a 'fast forward' and have a linear history:

master
A
C
D
B

No big deal either way, some people think all 'merges' in git should be fast forwards and therefore all branches should be rebased before the merge. Doesn't matter much to me either way, if you don't care, I'm happy to just merge.

[TWiStErRob]

Hmm, that's new, - and finally I know what it means if a commit has 2 parents :) - but I think that's the way it should be... if you draw your commit history a little differently:

master
  A
 / \
|   |
C   B (on branch)
D   |  |
|  /   ˇ
Merge B
|
...

This way it's clear what was the order of the commits in reality.
This is what I'm missing in SVN, when you merge you just get merge B out of thin air, it looks like GIT at least shows what was merged separately and altogether which is an advantage.

<rant>
As I see this is a way to push the merging and testing the latest to the owner of the branch/PR instead of the master owner dealing with it. Also "some people think all 'merges' in git should be fast forwards" sounds like some strong voiced urban legend started a tendency, and now people just follow it without questioning it - this may be the definition of de-facto standard and/or "historical reasons". But as you know I'm not a GIT expert, my first commit was a few days ago :)
</rant>

I would keep the "Merge" commits as well if I had a repo, but here yo' da boss. I'll rebase it now.

[sjudd]

Awesome, nice work, thank you!