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

Fix testing as Java application #3432

Merged
merged 6 commits into from
Sep 4, 2024

Conversation

AnthonyGrod
Copy link
Contributor

@AnthonyGrod AnthonyGrod commented Aug 29, 2024

Hello, I'm currently working on integrating Mill over the BSP plugin for IntelliJ (https://github.com/JetBrains/hirschgarten - now the BSP plugin code is merged with Bazel-related plugins).

I have stumbled upon an issue, where the Test <module.name> as Java application action did not work with a testing suite. It only performed a Query JVM environment task and tests were not run (later on it turned out that it was caused by the zincWorker in MillJvmBuildServer#jvmRunTestEnvironment, which could not find the correct test main class).

After digging into firstly the BSP plugin's code and later on Mill's code, I discovered that in the MillJvmBuildServer#jvmRunTestEnvironment there is testing logic lacking. It did not manage to fetch the correct test-specific main class, classpath and main arguments necessary for the testing as a local Java application.

I figured out that in order to make it work, I needed to leverage the TestModule's logic - I reproduced the MillJvmBuildServer#testTask logic of creating the java process' arguments and used it in a new TestModule#getTestEnvironmentVars function, which is then used in the MillJvmBuildServer#jvmRunTestEnvironment in case of a TestModule with JavaModule module (note: the code reproduction I've done might not be the most graceful solution - please let me know if I can do it better). Then the test-specific main class, classpath and main arguments I mentioned are assigned to adequate fields of the function's result - JvmEnvironmentItem, which is finally used on the BSP plugin side, which launches the actual local Java application.

I tested the solution with an example Mill project (generated with the g8 template). With BSP plugin installed, I imported the project with the new importing via BSP plugin option (it's still WIP and not yet available in official Scala plugin distribution). Then I checked that indeed the Test <module.name> as Java application gutter triggers the tests and their result is displayed in the terminal.

If you need any help with e.g. setting up the BSP plugin, I'm happy to help

@AnthonyGrod AnthonyGrod marked this pull request as draft August 29, 2024 09:33
@AnthonyGrod AnthonyGrod marked this pull request as ready for review August 29, 2024 10:24
@lihaoyi
Copy link
Member

lihaoyi commented Aug 29, 2024

@AnthonyGrod can you revert all non-essential formatting changes in the PR so it is easier to review exactly what you are trying to do

@AnthonyGrod
Copy link
Contributor Author

@lihaoyi sorry, done.

@lihaoyi lihaoyi requested a review from lefou August 30, 2024 00:31
@lihaoyi
Copy link
Member

lihaoyi commented Aug 30, 2024

Thanks @AnthonyGrod !

@lihaoyi
Copy link
Member

lihaoyi commented Aug 30, 2024

I think this looks reasonable, would be good if @lefou could take a look before I merge it

@lihaoyi
Copy link
Member

lihaoyi commented Sep 1, 2024

@AnthonyGrod can you update the PR description to include how you tested this? Manual testing is fine, but we should at least write down how it was done in case someone needs to reproduce the test in future. After that I can merge this

@AnthonyGrod
Copy link
Contributor Author

@lihaoyi I have updated the description.

@lihaoyi lihaoyi merged commit 29188c2 into com-lihaoyi:main Sep 4, 2024
33 checks passed
@lihaoyi
Copy link
Member

lihaoyi commented Sep 4, 2024

Thanks!

@lihaoyi lihaoyi added this to the 0.12.0-RC1 milestone Sep 4, 2024
This was referenced Sep 17, 2024
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