Initial commits for gh-6, bringing Theories over to junit.contrib #8

Merged
merged 14 commits into from Jun 4, 2013

Conversation

Projects
None yet
2 participants
Contributor

pholser commented Oct 27, 2011

This pull request in turn brings in code to help match data points and theory parms with generics.

pholser referenced this pull request in junit-team/junit4 Nov 26, 2012

Closed

Theories: Enhancement Autogeneration of datapoints #10

pholser referenced this pull request in junit-team/junit4 Apr 15, 2013

Closed

Theories doesn't honor parameterized types #64

Owner

dsaff commented May 1, 2013

@pholser,

I'm sorry I've been having trouble context-switching to this. How hard would it be to split this pull into two parts:

  1. The copying-over of the current Theories code, unchanged.
  2. The changes since the copy.

If it's not too much effort on your part, I think that would more than halve my efforts in review. But please don't put in a lot of work to do the split if it proves difficult. Thanks,

David

Contributor

pholser commented May 1, 2013

@dsaff A little tough -- sadly, I haven't done a great job of finer-grained commits each with a specific purpose in mind.

By and large the first two commits should represent the state of theories as of JUnit 4.10, with tests, with the addition of being able to pair up data points involving generics with theory parameters also potentially involving generics (this appears to be mostly in methods of ParameterSignature).

Then, there's getting current with 4.11, adding (then later removing) Paranamer/names for ParameterSignatures, and a README.

Thanks for having a look! Happy to answer any questions that come up during review.

--p

@dsaff dsaff commented on an outdated diff May 16, 2013

theories/README.md
@@ -0,0 +1,215 @@
+JUnit Theories Runner
+----
+
+This is a port of the JUnit theories runner into junit.contrib.
+
+In addition to being current with the theories implementation in the latest release of JUnit and depending on its core,
@dsaff

dsaff May 16, 2013

Owner

Can you wrap lines at <= 100 chars? Thanks.

Contributor

pholser commented May 16, 2013

Wrapped lines at 100 chars.

@dsaff dsaff commented on the diff May 16, 2013

theories/README.md
+ @DataPoint public static final int THREE = 3;
+ @DataPoints public static int[] moreExamples = { 4, 5, 6 };
+
+ @DataPoint public static int anotherExample() {
+ return 5;
+ }
+
+ @DataPoints public static int[] stillMoreExamples() {
+ return new int[] { 6, 7, 8, 9 };
+ }
+
+ @Theory public void factorsPassPrimalityTest(int n) {
+ assumeThat(n, greaterThan(0));
+
+ for (int each : PrimeFactors.of(n))
+ assertTrue(BigInteger.valueOf(each).isProbablePrime(1000));
@dsaff

dsaff May 16, 2013

Owner

Wow, I had no idea this existed.

@dsaff dsaff commented on the diff May 16, 2013

theories/README.md
+ assumeThat(m, not(equalTo(n)));
+
+ assertThat(PrimeFactors.of(m), not(equalTo(PrimeFactors.of(n))));
+ }
+ }
+
+To customize how data are fed to a given theory parameter:
+
+* Create a class that extends `ParameterSupplier`
+
+* Create an annotation that is itself annotated with
+`@ParametersSuppliedBy(YourParameterSupplier.class)`
+
+* Mark the desired theory parameter with your annotation
+
+In the example above, we create a `PositiveIntegerParameterSupplier` that gives 100 positive
@dsaff

dsaff May 16, 2013

Owner

Very nice writeup.

@dsaff dsaff and 1 other commented on an outdated diff May 16, 2013

theories/pom.xml
@@ -0,0 +1,30 @@
+<project xmlns="http://maven.apache.org/POM/4.0.0"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <modelVersion>4.0.0</modelVersion>
+
+ <groupId>org.junit.contrib</groupId>
+ <artifactId>junit-theories</artifactId>
+ <version>4.11-SNAPSHOT</version>
+ <packaging>jar</packaging>
+
+ <name>junit-theories</name>
+ <url>http://maven.apache.org</url>
@dsaff

dsaff May 16, 2013

Owner

Shouldn't this point somewhere in github?

@pholser

pholser May 17, 2013

Contributor

Indeed.

@dsaff dsaff commented on an outdated diff May 16, 2013

...c/main/java/org/junit/contrib/theories/DataPoint.java
@@ -0,0 +1,8 @@
+package org.junit.contrib.theories;
+
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+
+@Retention(RetentionPolicy.RUNTIME)
@dsaff

dsaff May 16, 2013

Owner

Two meta-notes:

  1. I'm not comparing to the JUnit core code, so it's quite likely that at some point, I'll ask for a change in code that I originally wrote, and you merely faithfully copied over. My apologies in advance. Always feel free to point out if a request will take a long time proportional to its benefit. Especially...

  2. Since this is such a big check-in, and github is not 100% ideal for reviews of long code snippets, I think in many cases, the right thing to do will be to add a TODO on this pass-through, and then address them soon later.

@dsaff dsaff commented on an outdated diff May 16, 2013

...c/main/java/org/junit/contrib/theories/DataPoint.java
@@ -0,0 +1,8 @@
+package org.junit.contrib.theories;
+
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+
+@Retention(RetentionPolicy.RUNTIME)
+public @interface DataPoint {
@dsaff

dsaff May 16, 2013

Owner

And to the point: would it make sense to add ElementType to these?

@dsaff dsaff commented on the diff May 16, 2013

.../main/java/org/junit/contrib/theories/DataPoints.java
@@ -0,0 +1,8 @@
+package org.junit.contrib.theories;
+
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+
+@Retention(RetentionPolicy.RUNTIME)
+public @interface DataPoints {
@dsaff

dsaff May 16, 2013

Owner

Could we add javadoc to all of these annotations, even if it's only pointing to the README?

@pholser

pholser May 17, 2013

Contributor

Wondering if it'd make sense to hold of on that until rolling in the 4.12 changes. @pimterry has added Javadoc for lots of these classes.

@dsaff

dsaff May 17, 2013

Owner

Works for me!

@dsaff dsaff and 1 other commented on an outdated diff May 16, 2013

...va/org/junit/contrib/theories/ParameterSignature.java
+ public static List<ParameterSignature> signatures(Constructor<?> constructor) {
+ return signatures(constructor.getGenericParameterTypes(),
+ constructor.getParameterAnnotations());
+ }
+
+ private static ArrayList<ParameterSignature> signatures(Type[] parameterTypes,
+ Annotation[][] parameterAnnotations) {
+
+ ArrayList<ParameterSignature> sigs = new ArrayList<ParameterSignature>();
+ for (int i = 0; i < parameterTypes.length; i++) {
+ sigs.add(new ParameterSignature(parameterTypes[i], parameterAnnotations[i]));
+ }
+ return sigs;
+ }
+
+ private final Type type;
@dsaff

dsaff May 16, 2013

Owner

Are you planning to maintain the "f" prefix throughout?

@pholser

pholser May 17, 2013

Contributor

I'll do that, if for no other reason that to honor its lineage. junit.contrib is ambivalent -- some modules use the Hungarian-esque, some do not.

@dsaff

dsaff May 17, 2013

Owner

Thanks. I should probably push harder for JUnit style in contrib, for easier back-and-forth between the projects (I'm ambivalent because it's never been my personal favorite style.)

@dsaff dsaff and 1 other commented on an outdated diff May 16, 2013

...va/org/junit/contrib/theories/ParameterSignature.java
+
+ public List<Annotation> getAnnotations() {
+ return Arrays.asList(annotations);
+ }
+
+ public boolean canAcceptArrayType(Type type) {
+ org.javaruntype.type.Type<?> typeToken = Types.forJavaLangReflectType(type);
+ return typeToken.isArray() && canAcceptType(typeToken.getComponentClass());
+ }
+
+ public boolean hasAnnotation(Class<? extends Annotation> type) {
+ return getAnnotation(type) != null;
+ }
+
+ public <T extends Annotation> T findDeepAnnotation(Class<T> annotationType) {
+ Annotation[] annotations2 = annotations;
@pholser

pholser May 17, 2013

Contributor

Indeed. Will inline.

@dsaff dsaff commented on the diff May 16, 2013

...rc/main/java/org/junit/contrib/theories/Theories.java
+ errors.add(new Error("DataPoint field " + field.getName() + " must be public"));
+ }
+ }
+ }
+
+ @Override
+ protected void validateConstructor(List<Throwable> errors) {
+ validateOnlyOneConstructor(errors);
+ }
+
+ @Override
+ protected void validateTestMethods(List<Throwable> errors) {
+ for (FrameworkMethod each : computeTestMethods()) {
+ if (each.getAnnotation(Theory.class) != null) {
+ each.validatePublicVoid(false, errors);
+ each.validateNoTypeParametersOnArgs(errors);
@dsaff

dsaff May 16, 2013

Owner

Is the goal to eventually remove this requirement?

@pholser

pholser May 17, 2013

Contributor

I'd love to, if I could figure a way that doesn't suck.

For example: if my theory method looks like:

@Theory public <T> void shouldHold(List<T> items) {
    // ...
}

a data point that is of type List or any subtype of List is eligible, no matter the type of T. But getting the machinery correct to substitute T properly in javaRuntype is tricky at best. Worse, if T comes from the enclosing class, who's to say what T should be?

@dsaff

dsaff May 17, 2013

Owner

Ugh. Good point. Challenge for another day.

@dsaff dsaff and 1 other commented on an outdated diff May 16, 2013

...rc/main/java/org/junit/contrib/theories/Theories.java
+ testMethods.removeAll(theoryMethods);
+ testMethods.addAll(theoryMethods);
+ return testMethods;
+ }
+
+ @Override
+ public Statement methodBlock(FrameworkMethod method) {
+ return new TheoryAnchor(method, getTestClass());
+ }
+
+ public static class TheoryAnchor extends Statement {
+ private final FrameworkMethod fTestMethod;
+ private final TestClass fTestClass;
+ private final List<AssumptionViolatedException> fInvalidParameters =
+ new ArrayList<AssumptionViolatedException>();
+ private int successes = 0;
@dsaff

dsaff May 16, 2013

Owner

f prefix?

@pholser

pholser May 17, 2013

Contributor

Yeppers.

@dsaff dsaff and 1 other commented on an outdated diff May 16, 2013

...rc/main/java/org/junit/contrib/theories/Theories.java
+ public void evaluate() throws Throwable {
+ try {
+ statement.evaluate();
+ handleDataPointSuccess();
+ } catch (AssumptionViolatedException e) {
+ handleAssumptionViolation(e);
+ } catch (Throwable e) {
+ reportParameterizedError(e, complete.getArgumentStrings(nullsOk()));
+ }
+ }
+ };
+ }
+
+ @Override
+ protected Statement methodInvoker(FrameworkMethod method, Object test) {
+ return methodCompletesWithParameters(method, complete, test);
@dsaff

dsaff May 16, 2013

Owner

s/Completes/Completed/?

@pholser

pholser May 17, 2013

Contributor

or maybe methodWithCompleteParameters?

@dsaff

dsaff May 17, 2013

Owner

Even better.

@dsaff dsaff and 1 other commented on an outdated diff May 16, 2013

...rc/main/java/org/junit/contrib/theories/Theories.java
+ protected void runWithAssignment(Assignments parameterAssignment) throws Throwable {
+ if (!parameterAssignment.isComplete()) {
+ runWithIncompleteAssignment(parameterAssignment);
+ } else {
+ runWithCompleteAssignment(parameterAssignment);
+ }
+ }
+
+ protected void runWithIncompleteAssignment(Assignments incomplete) throws Throwable {
+ for (PotentialAssignment source : incomplete.potentialsForNextUnassigned()) {
+ runWithAssignment(incomplete.assignNext(source));
+ }
+ }
+
+ protected void runWithCompleteAssignment(final Assignments complete) throws Throwable {
+ new BlockJUnit4ClassRunner(getTestClass().getJavaClass()) {
@dsaff

dsaff May 16, 2013

Owner

I think I'd like to extract a method that just constructs and returns the runner.

@pholser

pholser May 17, 2013

Contributor

Extracting a method into TheoryAnchor for creating the runner means that the caller in TheoryAnchor won't have access to the protected methodBlock() method. I may extract the construction of the runner and the invocation of methodBlock() into a method on TheoryAnchor -- compiler happier...

@dsaff dsaff and 1 other commented on an outdated diff May 16, 2013

...nit/contrib/theories/internal/AllMembersSupplier.java
+ addArrayValues(field.getName(), list, getStaticFieldValue(field));
+ } catch (Throwable e) {
+ // ignore and move on
+ }
+ } else if (sig.canAcceptType(type)
+ && field.getAnnotation(DataPoint.class) != null) {
+ list.add(PotentialAssignment.forValue(field.getName(),
+ getStaticFieldValue(field)));
+ }
+ }
+ }
+ }
+
+ private void addArrayValues(String name, List<PotentialAssignment> list, Object array) {
+ for (int i = 0; i < Array.getLength(array); i++) {
+ list.add(PotentialAssignment.forValue(name + '[' + i + ']', Array.get(array, i)));
@dsaff

dsaff May 16, 2013

Owner

Could probably extract a method here that could also be used below.

@pholser

pholser May 17, 2013

Contributor

Absolutely.

@dsaff dsaff and 1 other commented on an outdated diff May 16, 2013

.../org/junit/contrib/theories/internal/Assignments.java
+import org.junit.contrib.theories.ParameterSignature;
+import org.junit.contrib.theories.ParameterSupplier;
+import org.junit.contrib.theories.ParametersSuppliedBy;
+import org.junit.contrib.theories.PotentialAssignment;
+import org.junit.contrib.theories.PotentialAssignment.CouldNotGenerateValueException;
+import org.junit.runners.model.TestClass;
+
+import java.lang.reflect.Method;
+import java.util.ArrayList;
+import java.util.List;
+
+/**
+ * A potentially incomplete list of value assignments for a method's formal parameters
+ */
+public class Assignments {
+ private List<PotentialAssignment> fAssigned;
@dsaff

dsaff May 16, 2013

Owner

Can this also be final?

@pholser

pholser May 17, 2013

Contributor

It can. Will change.

@dsaff dsaff and 1 other commented on an outdated diff May 16, 2013

.../org/junit/contrib/theories/internal/Assignments.java
@@ -0,0 +1,117 @@
+package org.junit.contrib.theories.internal;
+
+import org.junit.contrib.theories.ParameterSignature;
+import org.junit.contrib.theories.ParameterSupplier;
+import org.junit.contrib.theories.ParametersSuppliedBy;
+import org.junit.contrib.theories.PotentialAssignment;
+import org.junit.contrib.theories.PotentialAssignment.CouldNotGenerateValueException;
+import org.junit.runners.model.TestClass;
+
+import java.lang.reflect.Method;
+import java.util.ArrayList;
+import java.util.List;
+
+/**
+ * A potentially incomplete list of value assignments for a method's formal parameters
@dsaff

dsaff May 16, 2013

Owner

Probably worth mentioning that this is intended to be immutable.

@pholser

pholser May 17, 2013

Contributor

Certainly.

Owner

dsaff commented May 16, 2013

Some comments on the non-test code. I'll probably have to wait a day before getting to the tests. Thanks again!

Contributor

pholser commented May 21, 2013

@dsaff No worries, thank you! I've made the changes per our review from last week.

Contributor

pholser commented Jun 1, 2013

@dsaff Sorry to keep tapping you on the shoulder about this one -- anything else needed to pull this req in?

Owner

dsaff commented Jun 3, 2013

@pholser, no problem about the shoulder taps. I'm actually a good bit through it on the latest readthrough--haven't found anything to change yet, which is why it's been quiet. Might still be a day or two before I'm finished and ready to merge.

Owner

dsaff commented Jun 4, 2013

Paul,

I didn't find anything else worth holding this up further. I might have some quibbles to consider, but I'll log those as feature requests when I get back from vacation next week.

@dsaff dsaff added a commit that referenced this pull request Jun 4, 2013

@dsaff dsaff Merge pull request #8 from pholser/master
Initial commits for gh-6, bringing Theories over to junit.contrib
c4c7306

@dsaff dsaff merged commit c4c7306 into junit-team:master Jun 4, 2013

Contributor

pholser commented Jun 4, 2013

Ok, great! I will set about getting junit.contrib's theories' build machinery set up to deploy to Maven central, etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment