[Java8] Replace ConstantPoolTypeIntrospector #1178

mpkorstanje · 2017-07-09T21:03:54Z

Summary

Replace ConstantPoolTypeIntrospector with TypeResolver and refactor all the Java8 related stuff out of cucumber-java.

Motivation and Context

To determine which argument types a lambda function requires we created a ConstantPoolTypeIntrospector. However it has never functioned quite correctly (#937, #1140, #957), was prone to breaking (#912, #914) and hasn't been tested much (#1048).

It is important to understand that while we will get a properly functioning and tested replacement, TypeResolver uses the same ConstantPool and thus has the same potential to break. However because TypeResolver is used by a much larger audience I expect these problems to be shallow.

Because this changes the interface of Java8StepDefinition it made sense to refactor all the Java8 related stuff out of cucumber-java. This will make it easier in the future to add things like KotlinStepDefintions without creating a separate KotlinBackend.

How Has This Been Tested?

Check if the automated tests were still passing.

Types of changes

Bug fix (non-breaking change which fixes an issue).
- Method references that use primitives are correctly understood.
New feature (non-breaking change which adds functionality).
Breaking change (fix or feature that would cause existing functionality to not work as expected).
- When using anonymous innerclasses with Java8 Stepdefinitions it was possible to use typed lists and maps as an argument. This is no longer possible as TypeResolver only returns classes, not the full type.
- In fact it will not be possible to use anonymous inner classes as step definitions without java8 at all.

Checklist:

I've added tests for my code.
My change requires a change to the documentation.
I have updated the documentation accordingly.

mpkorstanje · 2017-07-14T22:07:24Z

java/pom.xml

-        <dependency>
-            <groupId>io.cucumber</groupId>
-            <artifactId>gherkin</artifactId>
-            <scope>provided</scope>


Both are transitive dependencies through cucumber-core.

mpkorstanje · 2017-07-14T22:08:06Z

java/src/main/java/cucumber/runtime/java/JavaBackend.java

        this.classFinder = classFinder;
-        methodScanner = new MethodScanner(classFinder);
+        this.objectFactory = objectFactory;
+        this.methodScanner = new MethodScanner(classFinder);


Rewrote the constructors to be idiomatic java.

mpkorstanje · 2017-07-14T22:09:02Z

java/src/main/java/cucumber/runtime/java/JavaBackend.java

@@ -157,44 +159,30 @@ void addStepDefinition(Annotation annotation, Method method) {
        }
    }

-    public void addStepDefinition(String regexp, long timeoutMillis, StepdefBody body, TypeIntrospector typeIntrospector) {
-        try {
-            glue.addStepDefinition(new Java8StepDefinition(Pattern.compile(regexp), timeoutMillis, body, typeIntrospector));


This has moved into the generated code.

mpkorstanje · 2017-07-14T22:09:27Z

java/src/main/java/cucumber/runtime/java/JavaBackend.java

            }
        }
    }

-    public void addBeforeHookDefinition(String[] tagExpressions, long timeoutMillis, int order, HookBody body) {
-        glue.addBeforeHook(new Java8HookDefinition(tagExpressions, order, timeoutMillis, body));


This has moved into the generated code.

mpkorstanje · 2017-07-14T22:12:04Z

java/src/test/java/cucumber/runtime/java/java8test/AnonInnerClassStepdefs.java

-
-import static org.junit.Assert.assertEquals;
-
-public class AnonInnerClassStepdefs implements GlueBase {


moved to cucumber.runtime.java8.test.AnonInnerClassStepdefs

mpkorstanje · 2017-07-14T22:19:34Z

java8/src/main/code_generator/I18n.java8.txt

    }

    default void ${java.text.Normalizer.normalize(kw.replaceAll("[\\s',!]", ""), java.text.Normalizer.Form.NFC)}(final String regexp, final long timeoutMillis, final StepdefBody.A0 body) {
-        JavaBackend.INSTANCE.get().addStepDefinition(regexp, timeoutMillis, body, ConstantPoolTypeIntrospector.INSTANCE);
+        JavaBackend.INSTANCE.get().addStepDefinition(
+            new Java8StepDefinition(regexp, timeoutMillis, StepdefBody.A0.class, body)


Typetools resolves the arguments of the the generic type (StepdefBody.A{n} ) using the type variable information from body (the implementing class) so we pass these in here.

mpkorstanje · 2017-07-14T23:52:40Z

java8/src/test/java/cucumber/runtime/java8/test/LambdaStepdefs.java

    }

    private void methodThatDeclaresException() throws Throwable {
    }
    private void methodWithAnArgument(Integer cuckes) throws Throwable {
        assertEquals(42, cuckes.intValue());
    }
+    private void methodWithAnIntArgument(int cuckes) throws Throwable {
+        assertEquals(42, cuckes);
+    }


Fixed by using TypeResolver

brasmusson

I can't find any problem with this design, but I feel a bit to unfamiliar with Java8 typing to really do a proper review of those aspects.

brasmusson · 2017-07-16T15:45:20Z

java/src/main/java/cucumber/runtime/java/JavaBackend.java

-        this.objectFactory = objectFactory;
+        this(objectFactory,
+            new ResourceLoaderClassFinder(
+                new MultiLoader(


I'm not sure all these line breaks improve readability, especially
new MultiLoader(currentThread().getContextClassLoader()) seems to fit better on one line.

You are right. I was missing a constructor JavaBackend(ObjectFactory, ResourceLoader). This made look into the root cause, why do we have these constructors in the first place. It turns out they're only used by tests which needed them because they were poorly written.

I've added a commit which improves these test somewhat.

brasmusson · 2017-07-16T15:48:22Z

java8/src/main/java/cucumber/runtime/java8/Java8HookDefinition.java

-package cucumber.runtime.java;
+package cucumber.runtime.java8;
+
+import static java.util.Arrays.asList;


I've found that the most common ordering of import in the code base is static imports after the other imports, so I've added that ordering to the Eclipse configuration I use for Cucumber-JVM, but you seems to have another configuration.

I've been using the default that comes with IDEA. What does Eclipse do by default? I'd prefer a minimal amount of configuration if possible.

Looks like I have the ability to quickly format the imports of the entire project. We can pick a standard now and adhere to it.

https://www.jetbrains.com/help/idea/optimizing-imports.html

mpkorstanje · 2017-07-16T18:37:36Z

I am interested in your opinion about separating out the java8 bits from cucumber-java. Moving it out of cucumber-java, out of the JavaBackend, means that any information the Java8StepDefintion uses will have to be either locally available in the generated steps or globally accessible e.g. via JavaBackend.INSTANCE.

With the upcoming CucumberExpressions this might be a problem. CucumberExpressions need the the TypeRegistry to be instantiated.

We kinda have to know how we'll solve that problem to solve this problem.

mpkorstanje · 2017-07-29T11:07:56Z

I have a solution for the CucumberExpressions in mind. Rather then adding the Java8StepDefintion directly to the backend:

JavaBackend.INSTANCE.get().addStepDefinition(
     new Java8StepDefinition(regexp, timeoutMillis, StepdefBody.A0.class, body)
);

We'd provide a Function<TypeRegistry,StepDefinition> (or the java7 equivalent) that will instantiate one:

JavaBackend.INSTANCE.get().addStepDefinition((typeRegistry) -> 
     new Java8StepDefinition(typeRegistry, regexp, timeoutMillis, StepdefBody.A0.class, body)
);

To determine which argument types a lambda function requires we created a ConstantPoolTypeIntrospector. However it has never functioned quite correctly (#937, #1140, #957), was prone to breaking (#912, #914) and hasn't been tested much (#1048). It is important to understand that while we will get a properly functioning and tested replacement, TypeResolver uses the same ConstantPool and thus has the same potential to break. However because TypeResolver is used by a much larger audience I expect these problems to be shallow. Because this change the interface of Java8StepDefinition it made sense to refactor all the Java8 related stuff out of cucumber-java. This will make it easier in the future to add things like KotlinStepDefintions without creating a separate KotlinBackend. Related issues: - #912 - #914 - #937 - #957 - #1140 - #1048 - #1140 Closes #937 Closes #1048

The JavaBackend had several test only constructors cluttering up the interface. Mostly due to poor setup in the tests themselves. By improving the test setup we can remove these constructors.

lock · 2018-10-24T23:13:56Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

mpkorstanje mentioned this pull request Jul 9, 2017

Enhancement: make Java8 lambda steps reflection portable and use stable javac APIs #937

Closed

mpkorstanje force-pushed the replace-constant-type-pool-inspector branch 10 times, most recently from 48dfb8b to 43635cb Compare July 14, 2017 22:03

mpkorstanje commented Jul 14, 2017

View reviewed changes

mpkorstanje force-pushed the replace-constant-type-pool-inspector branch from 43635cb to cf553b7 Compare July 14, 2017 22:28

mpkorstanje requested a review from brasmusson July 14, 2017 22:52

mpkorstanje added this to the 2.0.0 milestone Jul 14, 2017

mpkorstanje commented Jul 14, 2017

View reviewed changes

mpkorstanje force-pushed the replace-constant-type-pool-inspector branch 2 times, most recently from aba7380 to 1939498 Compare July 15, 2017 09:48

brasmusson reviewed Jul 16, 2017

View reviewed changes

mpkorstanje force-pushed the replace-constant-type-pool-inspector branch from 1939498 to 3dc7047 Compare July 23, 2017 15:41

mpkorstanje added 3 commits July 29, 2017 13:11

[Java] Remove test only constructors from JavaBackend

9afd72e

The JavaBackend had several test only constructors cluttering up the interface. Mostly due to poor setup in the tests themselves. By improving the test setup we can remove these constructors.

Update the History.md wrt Replace ConstantPoolTypeIntrospector

7f50aed

mpkorstanje force-pushed the replace-constant-type-pool-inspector branch from 008c653 to 7f50aed Compare July 29, 2017 11:25

mpkorstanje merged commit 7699a58 into master Jul 29, 2017

mpkorstanje deleted the replace-constant-type-pool-inspector branch July 29, 2017 12:09

lock bot locked as resolved and limited conversation to collaborators Oct 24, 2018

mpkorstanje restored the replace-constant-type-pool-inspector branch October 4, 2019 18:19

mpkorstanje deleted the replace-constant-type-pool-inspector branch October 4, 2019 18:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Java8] Replace ConstantPoolTypeIntrospector #1178

[Java8] Replace ConstantPoolTypeIntrospector #1178

mpkorstanje commented Jul 9, 2017 •

edited

Loading

mpkorstanje Jul 14, 2017 •

edited

Loading

mpkorstanje Jul 14, 2017

mpkorstanje Jul 14, 2017

mpkorstanje Jul 14, 2017

mpkorstanje Jul 14, 2017

mpkorstanje Jul 14, 2017 •

edited

Loading

mpkorstanje Jul 14, 2017

brasmusson left a comment

brasmusson Jul 16, 2017

mpkorstanje Jul 23, 2017

brasmusson Jul 16, 2017

mpkorstanje Jul 16, 2017

mpkorstanje Jul 16, 2017

mpkorstanje commented Jul 16, 2017 •

edited

Loading

mpkorstanje commented Jul 29, 2017

lock bot commented Oct 24, 2018


		import static org.junit.Assert.assertEquals;

		public class AnonInnerClassStepdefs implements GlueBase {

[Java8] Replace ConstantPoolTypeIntrospector #1178

[Java8] Replace ConstantPoolTypeIntrospector #1178

Conversation

mpkorstanje commented Jul 9, 2017 • edited Loading

Summary

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

mpkorstanje Jul 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpkorstanje Jul 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brasmusson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpkorstanje commented Jul 16, 2017 • edited Loading

mpkorstanje commented Jul 29, 2017

lock bot commented Oct 24, 2018

mpkorstanje commented Jul 9, 2017 •

edited

Loading

mpkorstanje Jul 14, 2017 •

edited

Loading

mpkorstanje Jul 14, 2017 •

edited

Loading

mpkorstanje commented Jul 16, 2017 •

edited

Loading