Throw TestFailedOnTimeoutException instead of plain Exception on timeout. Fixes #771 #777
Conversation
| @@ -0,0 +1,11 @@ | |||
| package org.junit.internal.runners.statements; | |||
|
|
|||
| public class TestFailedOnTimeoutException extends Exception { | |||
and provide getters for timeout and time unit
|
|
||
| private static final long serialVersionUID = 31935685163547539L; | ||
|
|
||
| private TimeUnit fTimeUnit; |
There was a problem hiding this comment.
could you please make these fields final?
|
Thanks for the changes! A few very minor issues @dsaff do you have a preference for which package this new exception type should live in, and what it should be named? |
| evaluateWithWaitDuration(TIMEOUT + 50); | ||
| } | ||
|
|
||
| private BaseMatcher<TestFailedOnTimeoutException> exceptionWithTimeout(final long timeout, final TimeUnit unit) { |
There was a problem hiding this comment.
This feels like a big one-off effort to avoid try/catch in a single class...
| } | ||
|
|
||
| /** | ||
| * @return the time passed before the test was interrupted |
There was a problem hiding this comment.
If you don't think you need both a description and a @return it's best to just have a description and omit the @return. The first sentence of the description used in the Javadoc as the short description. So I would replace this line with "Gets the time passed before the test was interrupted"
There was a problem hiding this comment.
I'd say I don't agree with you. I think its the other way around. Specify the return statement, it makes it clear what you will receive from the method. No further description indicates that the method doesn't do anything interesting otherwise... Most developers don't use the generated javadoc html page, but the popup in their IDE and there you see the return statement clearly.
But, I will do as you suggest since this is starting to be a long running pull request...
|
looks good to me. Thanks! Some very minor style notes. |
|
@askoog It won't let me merge. Could you merge from head? Thanks. Feel free to squash to one commit or not as you like. |
Conflicts: src/test/java/org/junit/tests/AllTests.java
|
Tried to merge changes, but I think i screw up something. Seems I committed some files with new line endings(?)... Any good ideas how to fix that? The merge conflict before was in the AllTests class btw |
|
@askoog I'm guessing your IDE was set to use different line endings. I suggest figuring out how to fix that, going back to the commit before the merge, fixing the line endings, and merging again. Sorry, I can't pull in a request where it looks like every line has changed |
|
@askoog We would like to ship this in 4.12. Do you have time to work on it? |
|
@marcphilipp Is there still a merge problem? Seems I messed up the branch earlier, might be easier to create a new PR from another branch. I'll see what I can do. When is 4.12 due for shipping? |
|
@marcphilipp @kcooney I've created a new branch without the newline accident. It's in PR #823 #823. Hope that one merges better, the change set looks better anyway. Tell me when it's merged and I will close this PR. |
|
Closing in favor of #823. |
Throw TestFailedOnTimeoutException instead of plain Exception on timeout. Fixes #771