Skip to content
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.

Sign up for GitHub

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

ExternalResource: declare after() to throw Exception #1421

Merged
merged 2 commits into from
Feb 27, 2017
Merged

ExternalResource: declare after() to throw Exception #1421

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a6292eb
ExternalResource: declare after() to throw Exception
alb-i986 Feb 19, 2017
97ef37d
throw Throwable, more generic
alb-i986 Feb 20, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/main/java/org/junit/rules/ExternalResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,10 @@ protected void before() throws Throwable {

/**
* Override to tear down your specific external resource.
*
* @throws Exception if teardown fails
*/
protected void after() {
protected void after() throws Exception {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why Exception instead of Throwable like we did for before()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just because AFAIK rarely methods throw Throwable, it's more likely they throw Exception, e.g. AutoCloseable#close.
But ya, we should probably make it more generic and throw Throwable.
Do you want me to?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While it's rare for methods to throw Throwable, when you call a method using reflection you often handle the InvocationTargetException by throwing the cause (a Throwable). That's why Statement.evaluate() is declared to throw Throwable.

I don't think it harms anything to have the method declared to throw Throwable. The calling code catches Throwable and subclasses of ExternalResource can override the method to throw a different exception.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that closing a resource usually involves doing reflection or anything that throws Throwable, but anyway yeah, it's harmless, so I'll do.

// do nothing
}
}