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.

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

Closed
wants to merge 2 commits into from
Closed

Conversation

MatrixFrog
Copy link

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.

@jschneider
Copy link

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.
Maybe the RuntimeException should one be thrown, if a (newly created) parameter is set at DataPoints.

What about the "DataPoint" annotation? I think the same changes should be applied for that anntotation, too.

@MatrixFrog
Copy link
Author

I can't think of one either, so I would love to hear one. :)

You're right about @DataPoint. If it looks like a change along these lines is going to be accepted, I'll definitely make the equivalent change on @DataPoint as well...

@jschneider
Copy link

I have added some checks checking for void methods:

https://github.com/jschneider/junit/commit/2760cf967da1bfb400f07fec818b064411022fde

@dsaff
Copy link
Member

dsaff commented Feb 22, 2012

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})

?

@dsaff
Copy link
Member

dsaff commented Mar 27, 2012

@MatrixFrog, are you still available to work on this?

@dsaff
Copy link
Member

dsaff commented Apr 24, 2012

I'm guessing @MatrixFrog has moved on. Is anyone else on the thread available to fix this up?

@dsaff
Copy link
Member

dsaff commented May 22, 2012

Closing for now, can re-open later

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants