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

[WIP] TypeClassResolutionTest for code snippets #57

Closed
wants to merge 23 commits into from

Conversation

jansorg
Copy link
Contributor

@jansorg jansorg commented Nov 1, 2019

@i-walker cc @raulraja

This PR adds a very early version of a "heavy" test to test type resolution. A heavy test is a test which configures a full IntelliJ runtime environment and a project on disk before the test methods are run. This is more heavyweight than the light tests we're usually running in idea-plugin.

This is not ready to merge

arrow.meta.ide.plugins.typeclasses.TypeResolutionTest is a wip test to demonstrate how the heavy testing is done.
This needs to happen as soon as it's working:

  • The code to setup the module content root, build folder and src folder will most probably be shared between different tests
  • The method to compile a snippet should be shared too

What's broken

  • running tests with IntelliJ is broken. The reason is that IntelliJ's gradle integration can't handle the shadow plugin. https://youtrack.jetbrains.com/issue/IDEA-163411.
  • Running test TypeClassResolutionTest currently produces an exception about a mismatch of class org.jetbrains.kotlin.idea.KotlinFileType. Both the Kotlin IDE plugin and dependency kotlin-compiler-embedded contain this class.

@jansorg
Copy link
Contributor Author

jansorg commented Nov 1, 2019

@i-walker The heavy test base class HeavyIdeTestSetUp is inheriting from CodeInsightFixtureTestCase. It's also sharing a fixture and this might allow some reuse between light and heavy test logic.
CodeInsightFixtureTestCase is similar to class PlatformTestCase, which I mentioned earlier. It's setting up a fixture to simplify PsiFile creation, etc. Without a fixture setup we'd have to reimplement this in PlatformTestCase.

testing-plugin/build.gradle Outdated Show resolved Hide resolved
testing-plugin/build.gradle Outdated Show resolved Hide resolved
@i-walker
Copy link
Member

i-walker commented Nov 1, 2019

Thanks to both of you I will do a complete cleanUp and close the PR #53

# Conflicts:
#	testing-plugin/src/main/kotlin/arrow/meta/plugin/testing/CompilationAssertions.kt
docs/build.gradle Show resolved Hide resolved
testing-plugin/build.gradle Outdated Show resolved Hide resolved
val actualStatus: CompilationStatus,
val log: String,
val actualGeneratedFilePath: Path,
val classesDirectory: File
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are other directories and this is only the classes directory. For instance:

Paths.get(internalResult.outputDirectory.parent, "sources", "$DEFAULT_FILENAME.meta")

is jumping to the parent in order to access to sources. I think if we talk about classes directory, the source code communicates better.

}
}
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, what's changing here?

@@ -10,14 +10,14 @@ import java.nio.file.Paths

private const val DEFAULT_FILENAME = "Example.kt"

internal data class CompilationResult(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this change breaks the purpose of our testing library. I'm still trying to figure out what's the need but I'm thinking about 2 options:

  • Thinking about an assertion and improving the testing library with that assertion.
  • If it's only necessary to get the classes after the compilation, it seems that's not a goal of our testing library. We could add a function for compiling sources with the use of the compiler plugin in this module.

Our testing library is a step further from kotilin-compiler-testing. It doesn't show the result of the compilation to the user and does all the assertions according to the compilation data. If we need to manage the result of a compilation, for instance, get the directory of classes, it's not necessary our testing library. kotlin-compiler-testing can be used directlty.

Does it make sense for you, @jansorg , @i-walker ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I propose that we focus on implementing this deliverable and negotiate with @raulraja what the best approach is. I do understand that you value the set of principles in testing-plugin and I am certain there is a solution, which works for everyone.
Though for now, we need the time to iterate and fulfill our deadlines.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@i-walker CompilationResult should remain internal unless you have a valid reason to make it public as the compiler tests are hiddng that behind asserts. Why do you need this method public? We are attempting to make the DSL cohesive with the rest of the APIs and both compiler and ide should share the design but this model was not intended to be part of the public API

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to provide typeclass resolution tests we need the metadata, that the compiler-plugin produces. Within an IdeEnvironment we can only reference and create files, but without the engine of a compiler. Naturally, we would lean on something which we already use in Meta We are in need of a function in testing-plugin, where we can utilize a function (Source) -> (List<Config>) -> CompilationResult or something isomorphic.

We're only proposing a collaboration between testing-plugin and idea-plugin, unless we're dropping tests for SyntheticResolution. Consequently, we can not guarantee issues that the current scala-idea-plugin has e.g.: redlines on Syntheticmembers and other unresolved references we add to the language.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What kind of metadata is that and how is it used? We can just add a new assert for that that gives you that metadata or analyzes.it in the interpreter of the test DSL.

Copy link
Member

@i-walker i-walker Nov 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need .class files and everything generated in the build folder.
We also need the actual files and directories, no interpretation or assertion.
Thus, the editor in the TestEnvironment is able to index generated members and screens out unresolved ones.

Copy link
Member

@i-walker i-walker Nov 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The underlying library has the property `outputDirectory, which includes some of the files we need. But we're not able to compile it with the current settings.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We grab the metadata and copy it into the buildFolder of each IdeTestEnviroment. After that, we can initiate indexing as it would happen if a user would start a gradle build task in Idea.

@i-walker
Copy link
Member

i-walker commented Nov 1, 2019

This looks good. We can now work on it 😄

@i-walker i-walker changed the title [WIP] Test type resolution for code snippets [WIP] Typeclassresolutiontest for code snippets Nov 2, 2019
@rachelcarmena
Copy link
Member

See proposal in #65

@jansorg
Copy link
Contributor Author

jansorg commented Nov 4, 2019

@i-walker I fixed a few of the classloader issues. The problem is, that the JetBrains plugin already comes with kotlin-stdlib, kotlin-compiler-embedded and a lot of the other kotlin libs used in idea-plugin's dependencies.
Because testing-plugin was transitively adding a dependency on kotlin-compiler-embedded this library was included twice in the classpath. This confused the classloader which now had multiple separate .class files for KotlinFileType. I've not investigated much further, but this fixed the class cast exceptions I was seeing.

As a side-effect, this brings down the side of idea-plugin.zip from 44mb to 6.4mb.

There's one (last?) exception which I can't solve:
IntelliJ complains about project in metaPlugin.registerMetaComponents(project, configuration) used in MetaPluginRegistrarComponent. project of MetaPluginRegistrarComponent is from the IntelliJ SDK, but the expected type of project is from kotlin-compiler, afaik.
I don't know why this compiles (shadowed?), but still raises an exception when executed.
Do you know how to fix this?

I've made sure that the Kotlin plugin is bundling 1.3.50 by checking META-INF/MANIFEST.MF of IDEA-U/ch-1/193.4932.9/plugins/Kotlin/lib/kotlin-stdlib.jar.
Here's the full listing of what's included with the Kotlin plugin:

-rw-r--r-- 1 jansorg users    20448 19. Sep 20:04 allopen-ide-plugin.jar
-rw-r--r-- 1 jansorg users   347398 19. Sep 20:04 android-extensions-compiler.jar
-rw-r--r-- 1 jansorg users      339 19. Sep 20:04 android-extensions-ide.jar
-rw-r--r-- 1 jansorg users    17536 11. Sep 06:23 annotations-13.0.jar
-rw-r--r-- 1 jansorg users   611568 11. Sep 06:23 javaslang-2.0.6.jar
-rw-r--r-- 1 jansorg users    26418 11. Sep 06:23 javaslang-match-2.0.6.jar
-rw-r--r-- 1 jansorg users     2497 11. Sep 06:23 javax.inject-1.jar
drwxr-xr-x 2 jansorg users        3 19. Sep 20:04 jps
-rw-r--r-- 1 jansorg users    24351 19. Sep 20:04 kapt3-idea.jar
-rw-r--r-- 1 jansorg users    13442 19. Sep 20:04 kotlin-allopen-compiler-plugin.jar
-rw-r--r-- 1 jansorg users  1466574 19. Sep 20:04 kotlin-compiler-client-embeddable.jar
-rw-r--r-- 1 jansorg users   679560 19. Sep 20:04 kotlin-daemon-client.jar
-rw-r--r-- 1 jansorg users   793532 19. Sep 20:04 kotlin-daemon-client-new.jar
-rw-r--r-- 1 jansorg users  1182434 19. Sep 20:04 kotlin-daemon.jar
-rw-r--r-- 1 jansorg users   134181 19. Sep 20:04 kotlin-gradle-tooling.jar
-rw-r--r-- 1 jansorg users    24367 19. Sep 20:04 kotlin-noarg-compiler-plugin.jar
-rw-r--r-- 1 jansorg users 47508125 19. Okt 21:04 kotlin-plugin.jar
-rw-r--r-- 1 jansorg users  2793141 19. Sep 20:04 kotlin-reflect.jar
-rw-r--r-- 1 jansorg users    11773 19. Sep 20:04 kotlin-sam-with-receiver-compiler-plugin.jar
-rw-r--r-- 1 jansorg users   167775 19. Sep 20:04 kotlin-scripting-common.jar
-rw-r--r-- 1 jansorg users   292936 19. Sep 20:04 kotlin-scripting-compiler-impl.jar
-rw-r--r-- 1 jansorg users     2992 19. Sep 20:04 kotlin-scripting-intellij.jar
-rw-r--r-- 1 jansorg users   157602 19. Sep 20:04 kotlin-scripting-jvm.jar
-rw-r--r-- 1 jansorg users    42123 19. Sep 20:04 kotlin-script-runtime.jar
-rw-r--r-- 1 jansorg users    73668 19. Sep 20:04 kotlin-script-util.jar
-rw-r--r-- 1 jansorg users   170949 19. Sep 20:04 kotlin-stdlib-common.jar
-rw-r--r-- 1 jansorg users  1326290 19. Sep 20:04 kotlin-stdlib.jar
-rw-r--r-- 1 jansorg users     3125 19. Sep 20:04 kotlin-stdlib-jdk7.jar
-rw-r--r-- 1 jansorg users    15472 19. Sep 20:04 kotlin-stdlib-jdk8.jar
-rw-r--r-- 1 jansorg users    43214 19. Sep 20:04 kotlin-util-io.jar
-rw-r--r-- 1 jansorg users   123558 19. Sep 20:04 kotlin-util-klib.jar
-rw-r--r-- 1 jansorg users  1115113 19. Sep 20:04 kotlinx-coroutines-core-1.2.1.jar
-rw-r--r-- 1 jansorg users    15183 19. Sep 20:04 kotlinx-coroutines-jdk8-1.2.1.jar
-rw-r--r-- 1 jansorg users   453068 19. Sep 20:04 kotlinx-serialization-compiler-plugin.jar
-rw-r--r-- 1 jansorg users   393002 11. Sep 06:23 markdown-0.1.25.jar
-rw-r--r-- 1 jansorg users    22333 19. Sep 20:04 noarg-ide-plugin.jar
-rw-r--r-- 1 jansorg users    20223 19. Sep 20:04 sam-with-receiver-ide-plugin.jar

@rachelcarmena
Copy link
Member

@jansorg , maybe it's solved with #65 because it replaces testing-plugin by kotlin-compile-testing.

#65 is not a pull request on masterbranch but on this branch: ja-heavy-testing.

@jansorg
Copy link
Contributor Author

jansorg commented Nov 4, 2019

@rachelcarmena Thanks! Probably only partially. Most of the dependencies excluded from testing-plugin are inherited from kotlin-compile-testing. I‘d be glad to replace testing-plugin, though!
My last commit also added more necessary excludes.

@jansorg jansorg changed the title [WIP] Typeclassresolutiontest for code snippets [WIP] TypeClassResolutionTest for code snippets Nov 8, 2019
Simplify kotlin compiler integration with the proposal of @rachelcarmena
@jansorg
Copy link
Contributor Author

jansorg commented Nov 8, 2019

I've merged #65. Now there are a few more remaining problems.

  • Compilation of a Kotlin code snippet starts, but breaks with an exception. I've update the description of this PR. The cause is that KotlinFileType is available twice on the classpath, once in the Kotlin IDE plugin and once in the dependency kotlin-compiler-embeddable.jar. The dependency is required and can't be removed, though.

Exception:

 java.lang.ClassCastException: org.jetbrains.kotlin.idea.KotlinFileType cannot be cast to org.jetbrains.kotlin.com.intellij.openapi.fileTypes.FileType
	at org.jetbrains.kotlin.cli.jvm.compiler.KotlinCoreEnvironment$Companion.registerApplicationServices(KotlinCoreEnvironment.kt:657)
	at org.jetbrains.kotlin.cli.jvm.compiler.KotlinCoreEnvironment$Companion.createApplicationEnvironment(KotlinCoreEnvironment.kt:523)
	at org.jetbrains.kotlin.cli.jvm.compiler.KotlinCoreEnvironment$Companion.getOrCreateApplicationEnvironmentForProduction(KotlinCoreEnvironment.kt:476)
	at org.jetbrains.kotlin.cli.jvm.compiler.KotlinCoreEnvironment$Companion.createForProduction(KotlinCoreEnvironment.kt:430)
	at org.jetbrains.kotlin.cli.jvm.K2JVMCompiler.createCoreEnvironment(K2JVMCompiler.kt:253)
	at org.jetbrains.kotlin.cli.jvm.K2JVMCompiler.doExecute(K2JVMCompiler.kt:151)
	at org.jetbrains.kotlin.cli.jvm.K2JVMCompiler.doExecute(K2JVMCompiler.kt:54)
	at org.jetbrains.kotlin.cli.common.CLICompiler.execImpl(CLICompiler.kt:84)
	at org.jetbrains.kotlin.cli.common.CLICompiler.execImpl(CLICompiler.kt:42)
	at org.jetbrains.kotlin.cli.common.CLITool.exec(CLITool.kt:104)
	at com.tschuchort.compiletesting.KotlinCompilation.compileKotlin(KotlinCompilation.kt:545)
	at com.tschuchort.compiletesting.KotlinCompilation.compile(KotlinCompilation.kt:713)

@rachelcarmena
Copy link
Member

@jansorg, we've just merged a fix on master. There was an error when shadowing compiler-plugin.

@jansorg
Copy link
Contributor Author

jansorg commented Nov 8, 2019

@rachelcarmena Thanks! I've merge master. It's still broken.
The kotlin-compiler-embedded dependency comes from com.github.arrow-kt:kotlin-compile-testing:1.3.0.

@jansorg
Copy link
Contributor Author

jansorg commented Jan 6, 2020

as far as I can tell, this will be superseded by #252 . Heavy tests won't be needed anymore when that wip PR is finished and merged.

@rachelcarmena
Copy link
Member

as far as I can tell, this will be superseded by #252 . Heavy tests won't be needed anymore when that wip PR is finished and merged.

Thanks @jansorg !! 🎉 So, let's close it 👍

@jansorg jansorg deleted the ja-heavy-testing branch April 29, 2020 09:31
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.

None yet

5 participants