-
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
Added named datapoint(s) support to theories, fixing #65. #621
Conversation
Linking to #65. |
I love the thrust of the idea. Lots of hard work in here, thanks. Before I get too deep in, did you look into getting this behavior through the existing ParameterSupplier override? Doing so might create something easier for end-users to tweak. |
Yes, underneath this is just a ParameterSupplier. I've subclassed AllMembersSupplier (which gives all the valid datapoints available on a class for a parameter) and extracted out into methods the code that finds all annotated methods/fields of a given class. The subclass (SpecificDataPointsSupplier) then overrides those methods to filter the methods & fields to just the ones annotated with the right name for the parameter. You can very nearly go whole hog and just put Tl;dr unless there's some other way to inject the TestClass into a ParameterSupplier that I've missed, I don't think you can implement this just with the existing ParameterSupplier model. |
How would you feel about adding a way to inject the TestClass? The easiest way might be to check if the ParameterSupplier class has a TestClass-accepting constructor, and use that to build it. I think it's always a win if the next person who wants to do something similar can do it without making another change to the core. |
Sounds reasonable, done. |
private final TestClass fClass; | ||
} | ||
protected final TestClass fClass; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you leave this as private, with a protected accessor? Just a bit of future-proofing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
(Actually, it's now just back to being a private field; turns out after some refactoring I'd done previously this isn't being directly accessed at all by SpecificDataPointsSupplier, which just pulls fields & methods from AllMembersSupplier.getDataPointsMethods() etc instead, and filters things by name out of that, so an accessor to the test class itself isn't necessary.)
Really good stuff here. In general, I like to make sure that before anything runs, we're pretty sure there aren't malformed structures, so I've mentioned some places that look like they need extra validation. Thanks. |
@Retention(RetentionPolicy.RUNTIME) | ||
@Target(ElementType.PARAMETER) | ||
@ParametersSuppliedBy(SpecificDataPointsSupplier.class) | ||
public @interface FromDataPoints { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dsaff What do you think about the idea of making the name of this annotation more general, similar to @Named
in Guice? This annotation is essentially a binding annotation (see https://code.google.com/p/google-guice/wiki/BindingAnnotations), qualifying which value is injected. Where it the injected value comes from can be orthogonal. Here the data is provided by DataPoints
, but in the future we could allow data to come from other locations (parameters, a custom runner, the parent suite, etc). As long as something complains if there is more than one possible value that maps to the given name, we could allow that kind of flexibility.
In fact, we could use the @Named
(http://docs.oracle.com/javaee/6/api/javax/inject/Named.html) because it is part of JSR 330 (http://jcp.org/en/jsr/detail?id=330).
@kcooney, it's an interesting idea. Allowing myself to rabbit-hole even a bit more, I wonder if pushing the similarities between this feature and DI would be helpful or harmful? For example, in DI, there's only one best thing to inject--here, we want a plurality of things to inject. This can lead to confusion--for example, you're assuming that there should be only one possible value that maps to the given name, whereas, if I remember the code in this pull correctly, there can be many values. It might be interesting to try some things in a sandbox runner which directly depended on Guice or something similar. I'm unlikely to have the expertise to know whether punning between Guice and JUnit is likely to be helpful. Was |
Actually, with DI there could be many possible values. If I have an object in a web application that represents a user, which instance is injected depends on the user associated with the request that the web server is processing. Of course, I could have a global like a database connection pool where the same value is always injected. The value injected depends on the scope. One obvious possible scope in JUnit could be test class scope. A
My main point is declaring which data value corresponds with which method/constructor parameter does not seem specific to Theories. Don't we have the same problem with parameterized tests? |
Last one in this stream of comments: it's your call, and I'll happily tweak this either way, but I think the DI analogy is a bit forced and doesn't offer much for the extra complexity it'll add. In addition |
@pimterry I don't quite understand the point about specifying multiple names. Perhaps if you added Javadoc to the I wasn't suggesting replacing both |
I think we're getting close enough that it would be worth the time to javadoc FromDataPoints and the new attributes on DataPoint and DataPoints. Iterating on that documentation may provide some final fine points on design and implementation. |
Also refactored tests for internal elements of theory code into a matching internal test package en route, to make the two package heirarchies correspond properly.
…lier as a normal ParameterSupplier, by allowing ParameterSuppliers to take TestClasses in their constructors. Also added a simple parameter supplier test case, since it appears there weren't any at all until now.
Mostly revolves around validation of theory class configuration and some reorganising and tidy up of bits of testing code.
Javadoc added (with rebase onto current master). @kcooney Ah I see sorry, I was assuming you were considering replacing both. As in the example linked above, that's how the JSR defines On top of that, after some further investigation, I don't think anything similar is useful in parameterized tests anyway. I haven't used them that much, but afaik the current model for them requires every test in the class to run with every set of test values, and only one set of data can exist per test class (looking at the implementation of Parameterized.getParametersMethod()) so there's no possible confusion between datasets whatsoever, and thus no need to name them. Given that that's not something JUnit is looking to change I'd agree with @kcooney's last post; we should stick with this current implementation and potentially reconsider later once we've seen usage in the wild. |
GitHub seems unhappy taking comments at the moment, perhaps related to the David Saff On Tue, Feb 5, 2013 at 7:47 PM, Tim Perry [email protected] wrote:
|
I still can't seem to add comments in the code itself. Can you see if you have the same problem, or if it's just my account? One thing I'd like to see is explicit reference in the javadoc to the meaning of un-annotated items. Will datapoints without a value be applied to all @FromDataPoints parameters, or none of them? Will un-annotated parameters take values from @DataPoints that have names, or not? |
No, I can't either comment on the code either. A quick look in the chrome dev console shows failed requests to KentBeck/junit, suggesting it's definitely an issue with the move. I've sent a message to github support. In the meantime though, I've now updated this javadoc, hopefully covering your concerns. Does this look better? |
Sorry, another issue: have you tried compiling under Java 5? I don't think ReflectiveOperationException will fly (as much as I'd like to use it). |
…port older Java versions
Yes, you're right, fixed. I've opted for updating this with a signature throwing bare exception instead. Debatable style, but the alternative is throwing two solid lines of exceptions (all of IllegalArgumentException, InstantiationException, InvocationTargetException, IllegalAccessException) on all three methods involved, the only calling method (Theories.runWithIncompleteAssignment()) throws Throwable already anyway, and this class isn't really for external use, so it seems cleaner this way all in all. Shout if you disagree! |
Works for me. |
Added named datapoint(s) support to theories, fixing #65.
Thanks! Can you add a comment at https://github.com/junit-team/junit/wiki/4.12-release-notes? Thanks. |
Done. |
This patch adds named datapoint/datapoints to theories, so that you can do:
This works for all single or array valued methods/fields.
(Also refactored tests for internal elements of theory code into a matching internal test package en route, to make the two package heirarchies correspond better)