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

Migrate compiler tests to "normal" unit tests #180

Closed
JakeWharton opened this issue Jul 9, 2023 · 4 comments · Fixed by #210
Closed

Migrate compiler tests to "normal" unit tests #180

JakeWharton opened this issue Jul 9, 2023 · 4 comments · Fixed by #210

Comments

@JakeWharton
Copy link
Collaborator

This is required for #142 as we cannot rely on the embedded Kotlin compiler to the same degree as when targeting the JVM backend, but it also becomes hard or impossible to execute the compiled target's code.

However, Kotlin has a mechanism which is built-in to do this for us: regular tests!

Regular tests written in commonTest will run on all supported targets of the host OS. Since Poko doesn't vary its IR based on backend, we really only need one JVM target, one JS target, and one native target to run each test. This will always be doable on every host OS since linux works on linux, macos on macos, and mingw on windows. JS and JVM work everywhere.

This obviously only works for tests that have successful processing of the annotation. Failing tests can remain executed by the embedded Kotlin compiler, or migrate to a functional test if we want to test it across backends.

I also think this should come with removal of running these tests across multiple JDKs. Since the IR is not varied across different JVM targets, there's little reason to suspect the runtime behavior would be affected by different JVM versions as the reliance of platform APIs is near zero. Technically it is exactly zero as the APIs are all Kotlin APIs which should be guaranteeing behavioral compatibility across JVM versions.

Now maybe in the future when targeting 17 or higher we start using invokedynamic bytecodes on the JVM leveraging Java's ObjectsMethods class which is what records use, but until that day varying the JDK is mostly redundant.

@drewhamilton
Copy link
Owner

regular tests!

I take it you mean, create test @Poko classes and write tests that instantiate them and assert against those instances? I.e. how the sample tests currently work, but covering all the cases the compiler tests currently cover?

That would definitely be easier to read, understand, and write.

We'd lose two pretty nice things though:

  1. The ability to debug the compiler plugin. I almost always cause compiler errors with new IR code, and it's massively valuable to be able to breakpoint the Poko plugin to find out what's happening. Maybe we can leave a skeleton of the current setup so it's easy to spin up temporary tests for debugging?
  2. The ease of running the tests multiple times with different compiler settings. I used to test both IR and non-IR compilation, and soon will want to test both K2 and non-K2 compilation. We could use a Gradle property and multiple CI builds to cover different types of compilation, but is there any way we could still run multi-compilation tests locally with one command?

or migrate to a functional test if we want to test it across backends

Testing failure across backends sounds important, not sure I understand what you're envisioning here though?

Since the IR is not varied across different JVM targets

Are you saying that I'm not varying the IR I've written across different targets, or that the Kotlin compiler doesn't ever convert the same IR to different bytecode based on JVM target? If the latter, do you have a link to where that's decided/described? That kinda bums me out, I assumed the Kotlin compiler would produce JVM target-specific optimizations somewhere along the line.

@JakeWharton
Copy link
Collaborator Author

The ability to debug the compiler plugin.

You can still do this with a normal build but it's definitely more complicated. I tend to put all of the tests in this regular form, but leave one using the old/current mechanism. There will still be a bunch in the current mechanism for testing failures since it's the easiest way.

The ease of running the tests multiple times with different compiler settings.

This is still easy. They become targets!

So for the K2 example you'd do:

jvm()
jvm("jvmK2") {
  compilations.all {
    compilerOptions.freeCompilerArgs += "-use-k2"
  }
}

Can do the same with any variation we want to support provided it can be achieved with compiler options or the DSL.

Testing failure across backends sounds important, not sure I understand what you're envisioning here though?

Nested Gradle builds which are puppeteered by Gradle TestKit.

This is still somewhat testing Kotlin, though. I suspect we can rely on the JVM-based embedded compiler tests for scenarios that fail to compile and reasonably assume it will also work for the other backends.

Are you saying that I'm not varying the IR I've written across different targets, or that the Kotlin compiler doesn't ever convert the same IR to different bytecode based on JVM target?

At this time, yes. Mostly because you don't use anything interesting because the behavior of the generated code is so basic. The only candidate is probably the string concatenation for toString could use invoke-dynamic and StringConcatFactory.

If the latter, do you have a link to where that's decided/described?

I only know from having worked on some of the compiler intrinsics, but they're all available from all versions of the JVM supported now. For example, the Kotlin code 0L.hashCode() used to inline the implementation, but now it calls into Long.hashCode on Java 8 and newer. I don't recall any others that would apply which only work on newer versions.

That being said, this is another thing that still can technically be supported but maybe we reduce the versions tested since it's extremely unlikely anyone is using anything but the generally-agreed-upon LTS releases that JDK vendors put out, or the newest version of the OpenJDK.

@drewhamilton
Copy link
Owner

but leave one using the old/current mechanism

Sounds good 👍

This is still easy. They become targets!

Oh perfect, I didn't know you could duplicate the same target like this.

I suspect we can rely on the JVM-based embedded compiler tests for scenarios that fail to compile and reasonably assume it will also work for the other backends.

Ah you're right. All my failures are in IR compilation, which I believe would happen before anything platform-specific happens anyway. Eventually they'll mostly move to FIR which is even earlier.

The only candidate is probably the string concatenation for toString could use invoke-dynamic and StringConcatFactory.

I think what's not clicking for me is: why doesn't irConcat already result in bytecode using StringConcatFactory when targeting Java 9+? Everything you say about Long.hashCode on Java 8 sounds to me like it should apply to string concat on Java 9.

Anyway, I'm good with this plan!

That being said, this is another thing that still can technically be supported

Let's just focus on the migration and the latest Java target for now. Easy to play with re-adding a few more targets and seeing how that affects CI time later.

Does commonTest need to go in a separate module so the compiler plugin can be applied, or is there a way to apply it within its own module's tests? If the former, probably the new sample module(s) becomes the home of (most of) the tests.

Hopefully migrating the existing tests is fairly easy: All the private compareTwo*Instances signatures should be reusable, but their internals need to be rewritten.

@JakeWharton
Copy link
Collaborator Author

I think what's not clicking for me is: why doesn't irConcat already result in bytecode using StringConcatFactory when targeting Java 9+? Everything you say about Long.hashCode on Java 8 sounds to me like it should apply to string concat on Java 9.

Punting this to #204 or even just a new feature request. I honestly am not 100% sure how it works so will have to find out.

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 a pull request may close this issue.

2 participants