-
Notifications
You must be signed in to change notification settings - Fork 3.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Assert#expectThrows #1154
Add Assert#expectThrows #1154
Changes from all commits
20ef04b
8f167ec
a35a0be
0cb4a2f
11ac32b
98bf9a1
fdf060d
a79d449
a74f735
ea6b132
5607a24
d6652ad
db7d23d
883a8f0
f598f0d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,11 +11,14 @@ | |
import static org.junit.Assert.assertSame; | ||
import static org.junit.Assert.assertThat; | ||
import static org.junit.Assert.assertTrue; | ||
import static org.junit.Assert.expectThrows; | ||
import static org.junit.Assert.fail; | ||
|
||
import java.io.IOException; | ||
import java.math.BigDecimal; | ||
|
||
import org.junit.Assert; | ||
import org.junit.Assert.ThrowingRunnable; | ||
import org.junit.ComparisonFailure; | ||
import org.junit.Test; | ||
import org.junit.internal.ArrayComparisonFailure; | ||
|
@@ -170,7 +173,7 @@ public void oneDimensionalDoubleArraysAreNotEqual() { | |
public void oneDimensionalFloatArraysAreNotEqual() { | ||
assertArrayEquals(new float[]{1.0f}, new float[]{2.5f}, 1.0f); | ||
} | ||
|
||
@Test(expected = AssertionError.class) | ||
public void oneDimensionalBooleanArraysAreNotEqual() { | ||
assertArrayEquals(new boolean[]{true}, new boolean[]{false}); | ||
|
@@ -674,4 +677,80 @@ public void assertNotEqualsIgnoresDeltaOnNaN() { | |
public void assertNotEqualsIgnoresFloatDeltaOnNaN() { | ||
assertNotEquals(Float.NaN, Float.NaN, 1f); | ||
} | ||
|
||
@Test(expected = AssertionError.class) | ||
public void expectThrowsRequiresAnExceptionToBeThrown() { | ||
expectThrows(Throwable.class, nonThrowingRunnable()); | ||
} | ||
|
||
@Test | ||
public void expectThrowsIncludesAnInformativeDefaultMessage() { | ||
try { | ||
expectThrows(Throwable.class, nonThrowingRunnable()); | ||
} catch (AssertionError ex) { | ||
assertEquals("expected Throwable to be thrown, but nothing was thrown", ex.getMessage()); | ||
return; | ||
} | ||
fail(); | ||
} | ||
|
||
@Test | ||
public void expectThrowsReturnsTheSameObjectThrown() { | ||
NullPointerException npe = new NullPointerException(); | ||
|
||
Throwable throwable = expectThrows(Throwable.class, throwingRunnable(npe)); | ||
|
||
assertSame(npe, throwable); | ||
} | ||
|
||
@Test(expected = AssertionError.class) | ||
public void expectThrowsDetectsTypeMismatchesViaExplicitTypeHint() { | ||
NullPointerException npe = new NullPointerException(); | ||
|
||
expectThrows(IOException.class, throwingRunnable(npe)); | ||
} | ||
|
||
@Test | ||
public void expectThrowsWrapsAndPropagatesUnexpectedExceptions() { | ||
NullPointerException npe = new NullPointerException("inner-message"); | ||
|
||
try { | ||
expectThrows(IOException.class, throwingRunnable(npe)); | ||
} catch (AssertionError ex) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. BTW, one common convention is to name the exception "expected" to make it 100% clear that you expect it to be thrown. See https://google-styleguide.googlecode.com/svn/trunk/javaguide.html#s6.2-caught-exceptions There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, I'd rather give that name to the parameter that is currently called There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 |
||
assertSame(npe, ex.getCause()); | ||
assertEquals("inner-message", ex.getCause().getMessage()); | ||
return; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why return? The method fail() may go to try block right after the line with expectThrows... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See #1154 (diff) |
||
} | ||
fail(); | ||
} | ||
|
||
@Test | ||
public void expectThrowsSuppliesACoherentErrorMessageUponTypeMismatch() { | ||
NullPointerException npe = new NullPointerException(); | ||
|
||
try { | ||
expectThrows(IOException.class, throwingRunnable(npe)); | ||
} catch (AssertionError error) { | ||
assertEquals("unexpected exception type thrown; expected:<IOException> but was:<NullPointerException>", | ||
error.getMessage()); | ||
assertSame(npe, error.getCause()); | ||
return; | ||
} | ||
fail(); | ||
} | ||
|
||
private static ThrowingRunnable nonThrowingRunnable() { | ||
return new ThrowingRunnable() { | ||
public void run() throws Throwable { | ||
} | ||
}; | ||
} | ||
|
||
private static ThrowingRunnable throwingRunnable(final Throwable t) { | ||
return new ThrowingRunnable() { | ||
public void run() throws Throwable { | ||
throw t; | ||
} | ||
}; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why the junit has still the same tendency to use nested interfaces and nested annotations like
Parameter
.It's really awful code like
Assert.ThrowingRunnable
and using static import forAssert.*
and much easier to use andimport ThrowingRunnable
. Bad example is to design API likeParameterized.Parameter
.The only reason why nested classes is that they are not static and share one instance; otherwise it's useless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no reason you need a star import. You can just static import
Assert.ThrowingRunnable
. Whether or not this interface should be extracted into a top-level file is up to the maintainers.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No I dont think so it's thier decision. This is opensource and code review. The reviewer can be anyone.
The argument is that nested classes share instance; otherwise they are useless. Nested annotations/interfaces are really hard to use if you want to have nice and meaningful code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can comment all you want on matters of code organization, and in accordance with Parkinson's Law of Triviality, people commonly do. My point is that the maintainers (which is not a group I belong to) have the best understanding of what the relevant convention is (if any) for new code, and what conditions would be sufficient for breaking that convention.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there are cases where static nested classes make sense as a kind of organization tool to avoid an explosion of unrelated top-level classes in a package (see, for example,
Map.Entrry
).I personally haven't decided if
ThrowingRunnnable
should be a top-level class. If we think it will be used elsewhere (perhapsErrorCollector
) then it should.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that
ThrowingRunnnable
is a bit of a special case because it's just a shim. It gives us a place to hang the specific functional type signature we want to accept. The interface itself is only intended to be implemented by the compiler; it's not really part of an API or SPI.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And yet extension developers will use it when they develop more complicated APIs that reuse our code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it seems like there's no way to "encapsulate" this interface. We won't be able to move, eliminate, or rename it later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marcphilipp do you have thoughts about whether this should be a top-level class? If we need it in another class, or if API developers want to use our code, then they will either need to import a nested class or introduce their own version. I don't have a strong feeling, but wanted to make sure you saw this thread.