forked from #450 and improved #544
Conversation
|
For later reference: This pull request addresses #406. |
| public class FailOnTimeout extends Statement { | ||
| private final Statement fOriginalStatement; | ||
|
|
||
| private final TimeUnit fTimeUnit; | ||
| private final long fTimeout; | ||
|
|
||
| public FailOnTimeout(Statement originalStatement, long timeout) { |
There was a problem hiding this comment.
Now that one can specify a TimeUnit, I think millis would be a clearer parameter name. What do you think?
There was a problem hiding this comment.
sure, i will update it. thx
|
@marcphilipp |
| * @param millis the millisecond timeout | ||
| * @since 4.11 | ||
| */ | ||
| public Timeout(long millis) { |
There was a problem hiding this comment.
I think we decided the last time we introduced TimeUnit to a class that the benefit to having three constructors wasn't worth the cognitive load of having to choose which constructor. Let's please not add this constructor.
There was a problem hiding this comment.
@dsaff
Should be this constructor removed now?
thx.
There was a problem hiding this comment.
@marcphilipp
@dsaff
@kcooney
@dharkness
I would be very happy to remove the constructor Timeout(int), but i am worry about what would happen if i do.
So i simulated this scenario, where your tests dependent on 4.10 using Timeout(int) suddenly change the junit version without recompiling them based on junit 4.11.
This will happen:
Exception in thread "main" java.lang.NoSuchMethodError: org.junit.rules.Timeout.(I)V
If it's the rule to update the documentation, then we should do it. If not, then i will remove this constructor.
There was a problem hiding this comment.
I think the suggestion was to remove the constructor taking a long value. Keep the original int constructor for compatibility and add only the long + TimeUnit constructor.
There was a problem hiding this comment.
See #406, which says that casting long to int is an evil due to truncation in numbers.
new Timeout( (int) DateUtils.MILLIS_PER_MINUTE * 5 )
There was a problem hiding this comment.
Another option that I like is to have
@Deprecated Timeout(int) {}
Timeout(long, TimeUnit)
static Timeout millis(long millis);
static Timeout seconds(long seconds);
static Timeout minutes(long minutes);
That way there's easy, non-confusable shortcuts for the most common options.
There was a problem hiding this comment.
@dsaff
I can do it now, but then you have two paradigms in one: a factory and typical object creation.
…tor, added factories, prepared for 4.12
|
@dsaff All requests implemented except for factory #minutes() because we do not have TimeUnit.MINUTES in Java 5:
|
| * } | ||
| * public void run2() { | ||
| * //uninterrupted code | ||
| * for (;;) |
There was a problem hiding this comment.
I think the canonical Java infinite loop is
while (true) {}
I think if we name the method infiniteLoop(), and use that style, we can drop the comment-within-a-javadoc below.
|
@dsaff
|
|
Thanks, @Tibor17! |
|
@Tibor17, can you please add a bullet about this at https://github.com/KentBeck/junit/wiki/4.12-release-notes? Thanks. |
|
@marcphilipp |
1 similar comment
|
@marcphilipp |
|
@Tibor17 ? |
|
@marcphilipp |
|
@marcphilipp Do you need me to update an *.md file as well? |
|
@marcphilipp https://github.com/marcphilipp Looks good, thanks! Do you need me to update an *.md file as well? No, we've decided to update the *.md file right before the release. |
|
ok, thx. Would you take a look at #549 ? |
Unlike in #450 i did not delete Timeout(int) constructor because it is backward compatibility issue (in class bytecode).
Instead Deprecated such one.
Added other two constructors with params:
improved tests and to truly realistic javadoc.
Compiled and tested with JDK 5.