-
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
@DataPoints-related fixes #328
Conversation
I am quite sure, this patch will not be accepted (unfortunately), since it changes the current behavior. While I prefer your version, there might exist some use cases, where silent failure is preferred (I can't think of one, but I have read that some people prefer it that way...). Your change in line 79 should probably be modified to stay backwards compatible with the current behavior. What about the "DataPoint" annotation? I think the same changes should be applied for that anntotation, too. |
I can't think of one either, so I would love to hear one. :) You're right about |
I have added some checks checking for void methods: https://github.com/jschneider/junit/commit/2760cf967da1bfb400f07fec818b064411022fde |
Sorry for the delay on this. Indeed, changes that cause currently passing tests to fail (even if they should fail) are handled with kid gloves. However, I think this one does make sense, in some form. I'd like a quick way for people to opt out of the change, however. Perhaps: @DataPoints(ignored={SomeExceptionWeShouldSilentlyIgnore.class, AnotherOne.class}) ? |
@MatrixFrog, are you still available to work on this? |
I'm guessing @MatrixFrog has moved on. Is anyone else on the thread available to fix this up? |
Closing for now, can re-open later |
This fixes both issue 125 and issue 137. There's probably a better exception to throw other than just
RuntimeException
but I don't know what that would be.