[#3716] Test for an IllegalArgumentException in JavaTemplateJava…

[api-from-the-ion]

…Extension; not running

What's changed?

I wrote the test, but it's not running.

What's your motivation?

To fix the ticket and get no exceptions and job done.

Anything in particular you'd like reviewers to focus on?

Right now, this test can't run. I get

java.lang.IllegalStateException: LST contains missing or invalid type information
Identifier->MethodInvocation->Block->MethodDeclaration->Block->ClassDeclaration->CompilationUnit
/*~~(Identifier type is missing or malformed)~~>*/Assertions

	at org.openrewrite.java.Assertions.assertValidTypes(Assertions.java:87)
	at org.openrewrite.java.Assertions.validateTypes(Assertions.java:57)
	at org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:288)
	at org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:132)
	at org.openrewrite.java.JavaTemplateTest.replaceAnonymousClassObject1(JavaTemplateTest.java:1050)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)

If I try to analyze this test fixture, it looks like that example analysis starts here (Assertions#assertValidTypes -> FindMissingTypes#findMissingTypes). The compile unit analyze (JavaVisitor#visitCompilationUnit) the imports, but in the next step, analyze the classes, the unit and the imports are lost. So when we recursively land at the identifier, the import and the class are unknown. I don't know how to get it. Some other tests in the test class also have imports, and it looks like it works.

Side note: Gradle can't find org.junit.platform:junit-platform-launcher for the Java17 and Java21 submodules. The maven central repository is here, but it can't see it. I have enough troubles beside this, so I just commented them out. But it would be nice to know how to deal with such trouble. It's not a VPN or corporate network - just checked it.

Anyone you would like to review specifically?

No

Have you considered any alternatives or workarounds?

Not at this stage.

Any additional context

I just copy-paste another test and copy the whole equals recepie, because I don't know which part of it causes the bug. The same is about the before and after versions of the code. It could be optimized when this test is running. But feel free to change the name and some unimportant parts as soon as this test can run.

Checklist

• I've added unit tests to cover both positive and negative cases
• I've added the license header to any new files through ./gradlew licenseFormat
• I've used the IntelliJ IDEA auto-formatter on affected files

[knutwannheden]

@api-from-the-ion Can you show us the code for the test case? I suspect that what is missing is a classpath configuration for the Java parser, like in this example:

rewrite/rewrite-java-test/src/test/java/org/openrewrite/java/AddImportTest.java

Lines 148 to 172 in 0d884d4

	@Test
	void dontDuplicateImports3() {
	rewriteRun(
	spec -> spec.recipe(toRecipe(() -> new AddImport<>("org.junit.jupiter.api.Assertions", "assertNull", false)))
	.parser(JavaParser.fromJavaVersion().classpath("junit-jupiter-api"))
	.cycles(1).expectedCyclesThatMakeChanges(1),
	java(
	"""
	import static org.junit.jupiter.api.Assertions.assertFalse;
	import static org.junit.jupiter.api.Assertions.assertTrue;
	import java.util.List;
	class A {}
	""",
	"""
	import static org.junit.jupiter.api.Assertions.*;
	import java.util.List;
	class A {}
	"""
	)
	);
	}

[api-from-the-ion]

Thank you for your reply. As of imports - I already saw another examples and will try to do this also here. Just an extra question: because the import declaration is in the example code and analyzed before the class block, shouldn't it be possible to add them to the compilation unit? So we don't have to add them manually, as in these examples? I think this could make test easier.

As of the bug itself, I think I found the source of the troubles. Just need to write a running tests and fix it. So I hope I can extend this PR with some functioning code in the near future (hopefully today).

Once again about JUnit missing in Java 17 and 21 modules, do you have any idea, why this could happen? It's not a critical question, I'm just curious.

[knutwannheden]

Thank you for your reply. As of imports - I already saw another examples and will try to do this also here. Just an extra question: because the import declaration is in the example code and analyzed before the class block, shouldn't it be possible to add them to the compilation unit? So we don't have to add them manually, as in these examples? I think this could make test easier.

I am not sure I understand what your proposal is here. If you for your tests always have the same set of imports, you could use string templating or string concatenation in your tests. But I probably misunderstood your question here.

Once again about JUnit missing in Java 17 and 21 modules, do you have any idea, why this could happen? It's not a critical question, I'm just curious.

No idea what that could be, no. @timtebeek Do you maybe have any clues?

[timtebeek]

I'm a bit lost on what your plan or expectation here is; you seem to have taken the existing AssertEqualsToAssertThatVisitor, put that in a JUnit test in this project instead, but for as much as I can see from the one commit here are lacking the junit-jupiter-api classpath entry when running the visitor in a test. And that is then exactly what the failing test complains about: not being able to find Assertions.

JavaTemplateTest > replaceAnonymousClassObject1() FAILED
    java.lang.IllegalStateException: LST contains missing or invalid type information
    Identifier->MethodInvocation->Block->MethodDeclaration->Block->ClassDeclaration->CompilationUnit
    /*~~(Identifier type is missing or malformed)~~>*/Assertions

It might help to work in rewrite-testing-frameworks, as that already has the required parserClasspath entry.

At a higher level: what is it you're hoping to do? Do you want to tweak that recipe? Learn from it? Develop your own?

[api-from-the-ion]

Here is the solution for the issue. When you debug this in the JavaTemplateJavaExtension$JavaVisitor#maybeReplaceStatement, where the exception is thrown, you will see, that substitutions.unsubstitute generate some strange substitutions from template. In the original case it was three part with something like
assertThat(__P__.<error><?>/*__p0__*/p())

in other case it was
assertThat(__P__.<error><?>/*__p0__*/p():)

First, a thought that the problem is in the template parser or parameters. But parser works correctly here. We have a combined template class source code with something like this
assertThat(__P__.<?>/*__p0__*/p()).isEqualTo(__P__.<java.lang.String>/*__p1__*/p());

And this is compiled to the state we can see above.

I asked myself where this question mark came from, and this was from the type of the generic variable. So, if we don't know the returning generic type, like in my examples, the correct type here is then ?. But in case when we combine our template we can't use ? and should use Object instead.

First a thought that we should replace the original name, but after the inspection, where the name value is used, I came to conclusion, that this is only place where we should return Object in this case. Maybe there are some other similar places with other generic scenarios, where we should also replace ? with Object, but I don't know about them.

I wrote three tests for the case, but if you think we only need one of them - feel free to transfer only needed tests.

[api-from-the-ion]

I am not sure I understand what your proposal is here. If you for your tests always have the same set of imports, you could use string templating or string concatenation in your tests. But I probably misunderstood your question here.

Never mind, I think, I found where I was wrong. I thought, that because you analyze the imports part first, you can automatically add them to the class path. But we need the artifacts to add (from resources or not) and not the class. And we don't know in which artifact this class is. So we can't avoid the need to add the used artifacts to the test manually.

Once again about JUnit missing in Java 17 and 21 modules, do you have any idea, why this could happen? It's not a critical question, I'm just curious.

Have to repeat this question. If you know the answer, this would be fine, otherwise it is also OK.

[knutwannheden]

Have to repeat this question. If you know the answer, this would be fine, otherwise it is also OK.

No, no idea, sorry.

Anyway, apart from some formatting I think this PR looks good. I've pushed a commit to fix that.

Let us know if there was anything else you were planning to add. Otherwise we can go ahead and merge it.

[api-from-the-ion]

I think I'm ready here and have no additional stuff to put here