Fixes #449, stopping AllMembersSupplier & Theories hiding DataPoints method exceptions #639
Conversation
|
I'm sorry to say, but as I've finally caught up and indicated at #449, this bug/missing feature was in the design phase, so we'll need to figure out some way to have users request the new behavior. |
|
Hmm. That's going to be quite untidy to implement, and a) this is in experimental anyway, b) I think very very few people are going to go out of their way to enable datapoint exception failures if we do add that, even though they should, and c) I really don't think there should be anybody intentionally relying on datapoint methods failing to return values, even if the implementation has allowed that for now. Could we go for the compromise you've proposed on an older pull-request relating to this at #328 (comment)? I.e. instead add an opt-out option as a |
|
I could get behind the proposed compromise. Please note that the current situation is pretty bad: If one has multiple Even when there is only one So yeah, the suggestion in #328 sounds pretty good by comparison :) |
|
OK, I can be reasonable on a good day. :-) If someone can put up a public notice about the suggested change on junit@yahoogroups.com, and no one complains for ~48 hours, we can go for the #328 compromise. |
…thod exceptions Also includes a rewrite of the Theory nullsAccepted code, since that relied on the previous behaviour of this.
|
Posted a message to the mailing list on wednesday (http://tech.groups.yahoo.com/group/junit/message/24356) and got no responses, so I've now added a fix along the lines of #328. DataPoint and DataPoints can now be given an ignoredExceptions list, and any exceptions thrown that are instances of members of that list will not fail tests. Better? |
|
@pimterry, thanks. Can you merge with HEAD? |
|
Merged. |
| DataPoint annotation = fMethod.getAnnotation(DataPoint.class); | ||
| if (annotation != null) { | ||
| for (Class<? extends Throwable> ignorable : annotation.ignoredExceptions()) { | ||
| Assume.assumeThat(t, not(instanceOf(ignorable))); |
|
This is really good. Just one request, and we're good to go. Thanks. |
Fixes #449, stopping AllMembersSupplier & Theories hiding DataPoints method exceptions
|
Thanks for this. Can you update the release notes? |
|
For reference: this change has been documented in the release notes in the interim. |
This also includes a rewrite of the Theory nullsAccepted code, since that relied on the previous behaviour of this. It's now just a synonym for Assume.assumeNotNull(params[]), and is handled in the Theories runner itself, which seems much nicer to me.
Some of the code around this has a good few discrepancies in how it handles datapoint array methods vs single datapoint methods, which make this a little messy, but I'm going to look at fixing these at a later stage and I've left them as they are. I've filed issues for them for now, as #637 and #638.