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

Add java code decompilation to unit tests #2

Merged
merged 10 commits into from
Jan 2, 2021

Conversation

LachlanMcKee
Copy link
Contributor

@LachlanMcKee LachlanMcKee commented Dec 30, 2020

This introduces Java Decompilation to the unit tests to make it clearer what the end result will be.

Happy to take any suggestions onboard.

@@ -40,6 +47,9 @@ fun main() {

@DebugLog
fun greet(greeting: String = "Hello", name: String = "World"): String {
if (greeting != "Hello") {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought it would be useful to see that the modified code also adds a println before an early return!

Copy link
Owner

Choose a reason for hiding this comment

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

I definitely like the idea of testing early return, but I would prefer to keep this particular source example really straight forward. Could early return (any even exception testing) be done as a different source file? I want the first thing people see in this file to be something really simple so maybe exit testing is a separate test class all together.

@LachlanMcKee
Copy link
Contributor Author

@bnorm hey! I am happy with this PR if you want to take a look. Happy to chat through it if there is anything you want change/elaborated

Copy link
Owner

@bnorm bnorm left a comment

Choose a reason for hiding this comment

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

🎉 This is great! Thank you so much for do this!

Just a few thoughts and only one required change I'd like to see made.

@@ -40,6 +47,9 @@ fun main() {

@DebugLog
fun greet(greeting: String = "Hello", name: String = "World"): String {
if (greeting != "Hello") {
Copy link
Owner

Choose a reason for hiding this comment

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

I definitely like the idea of testing early return, but I would prefer to keep this particular source example really straight forward. Could early return (any even exception testing) be done as a different source file? I want the first thing people see in this file to be something really simple so maybe exit testing is a separate test class all together.

+irDebugExit(function, startTime, irGet(result))
+expression.apply {
value = irGet(result)
if (expression.value.type == typeUnit) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without this change, I noticed that returning early in a function that does not have a return type (therefore Unit) would output "] = " + Unit.INSTANCE rather than "]"

@LachlanMcKee LachlanMcKee force-pushed the add-java-code-decompilation branch 2 times, most recently from 13c1866 to 2c9253a Compare January 1, 2021 21:24
import com.strobel.decompiler.PlainTextOutput
import com.tschuchort.compiletesting.KotlinCompilation
import com.tschuchort.compiletesting.SourceFile
import java.io.ByteArrayOutputStream
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Weirdly ktlint was not passing until I moved the java imports above org. pinterest/ktlint#527

@LachlanMcKee
Copy link
Contributor Author

@bnorm I believe I have made the appropriate changes. Mind taking another look?

Copy link
Owner

@bnorm bnorm left a comment

Choose a reason for hiding this comment

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

Amazing. Exactly what I was thinking. Thank you so much for this change! I'll probably start incorporating this into my other plugins.

@bnorm bnorm merged commit b5cc7ee into bnorm:main Jan 2, 2021
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.

2 participants