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

Added validation that all datapoint methods and fields are public and static to theory initialization #623

Merged

Conversation

pimterry
Copy link
Contributor

This fixes #125. In that issue, tests with non-static or non-public datapoint methods are ignored because they're not checked for when constructing the instance, and when the values are retrieved for theory parameters in getValue() in MethodParameterValue (in AllMembersSupplier) an exception is thrown, but then swallowed in addMultiPointMethods/addSinglePointMethods in AllMembersSupplier.

There's another abandoned pull request trying fixing this in #328, which rethrows all exceptions thrown in datapoint methods, including this one. This is arguably dubious since you could potentially have other exceptions coming from datapoint methods, and it's not clear whether those should actually fail tests. This commit does not affect issue 137 (caused by the exception hiding), which #328 does try to solve.

On the other hand, datapoint methods that are definitely being entirely ignored are a legitimately incorrect use of the datapoints annotation, and that should actually fail, I think.

While there was no error before this for non-static datapoint methods, there was an initialization error for non-static/non-public @DataPoint fields. There wasn't an error for @DataPoints array fields though, for no possible good reason I can see, since this doesn't have any of the same exception hiding issues as the methods do.

With this commit all of the above is fixed, by adding checks on test initialization that look at all the datapoint methods and fields, and confirms that they're public and static. This already existed for fields annotated with @DataPoint, but now also covers array fields annotated with @DataPoints and array and singleton annotated datapoint methods.

@dsaff
Copy link
Member

dsaff commented Jan 29, 2013

Nice work. Thanks!

dsaff pushed a commit that referenced this pull request Jan 29, 2013
…-#125

Added validation that all datapoint methods and fields are public and static to theory initialization
@dsaff dsaff merged commit 27ba66f into junit-team:master Jan 29, 2013
@dsaff
Copy link
Member

dsaff commented Jan 29, 2013

Can you add a note at https://github.com/KentBeck/junit/wiki/4.12-release-notes? Thanks again!

@pimterry
Copy link
Contributor Author

Done.

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.

No error/warning on non static method annotated with @DataPoint
2 participants