Skip to content

Zinc based testQuick command #4787

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

Closed
wants to merge 3 commits into from

Conversation

HollandDM
Copy link
Contributor

@HollandDM HollandDM commented Mar 27, 2025

This is a alternative design for testQuick command, based on Zinc's output. Zinc's incremental compiler is used internally in mill, and the analysis file is saved in the appropriate compile.dest. This file has many interesting information that can be used to identify which test classes need to be re-run

Algorithm

We can get Zinc analysis's stamps to identify which source files are changed between compile runs. With all the changed source files, we can then mark their products as the source of invalidation, and then do a "spanning invalidation process" (pretty similar to codesig's function) using Zinc analysis's relations to collect all invalidated products. From there, using the same relation objects, we can get the corresponding source files, and then collect all the invalidated classes's absolute path.

Having all invalidated absolute path, we can use testClasspath to get the invalidated classname that suitable for the test command to run.

There are some conversion between sources, products, classpaths, classfiles. This is required because Zinc's Analysis object does not have information about Java classnames.

/claim #4109
see also #4731 for call graph analysis based design.

@HollandDM
Copy link
Contributor Author

@lihaoyi This PR is an attempt for the Zinc based design, you can take a look. This uses solely Zinc's Analysis object.

The main advantages of this design is that the class dependencies graph is much smaller than methods call graph. Dependencies graph also based on Scala classes instead of Java classes, so it'd be more native to Scala.

Downside mainly is that we need some conversion between Scala classes and Java classpaths, as Zinc does not expose these relations for our need

@lihaoyi
Copy link
Member

lihaoyi commented Mar 27, 2025

The main advantages of this design is that the class dependencies graph is much smaller than methods call graph. Dependencies graph also based on Scala classes instead of Java classes, so it'd be more native to Scala.

Could you elaborate abit on this statement? I know Zinc works and is able to incrementally compile both Java and Scala modules, so shouldn't it also have some model of the Java file-level dependencies in your project?

@HollandDM HollandDM force-pushed the test-quick-command-2 branch from cb4f45f to b2fd081 Compare April 11, 2025 08:51
@HollandDM HollandDM force-pushed the test-quick-command-2 branch from d3043e8 to 9610b60 Compare April 11, 2025 08:54
@HollandDM HollandDM force-pushed the test-quick-command-2 branch from 9610b60 to bdcbef9 Compare April 11, 2025 08:55
@HollandDM
Copy link
Contributor Author

@lihaoyi I finally have some free time add an integration test for test-quick, please have a quick look to see if it is good enough.

assert(secondRun.out.isEmpty)

// Third run, MyNumber.scala changed, so MyNumberDefaultValueTests & MyNumberCombinatorTests should run
modifyFile(workspacePath / "app" / "src" / "MyNumber.scala", _.replace("def defaultValue: MyNumber = MyNumber(0)", "def defaultValue: MyNumber = MyNumber(1)"))
Copy link
Contributor Author

@HollandDM HollandDM Apr 11, 2025

Choose a reason for hiding this comment

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

This will be the main different between this Zinc based approach and the method call approach. The method call approach is expected to only trigger the MyNumberDefaultValueTests test, not the MyNumberCombinatorTests test

@HollandDM
Copy link
Contributor Author

HollandDM commented Apr 12, 2025

Not sure how to run ad hoc benchmark on this meaningfully, so I just run this with time command for a few time, here is the result:

  • ./mill-assembly.jar scalalib.test.testForked: 136.42s; 140.58s; 141.50s (avg: 139.50s)
  • ./mill-assembly.jar scalalib.test.testQuick: 142.63s; 142.17s; 151.31s (avg: 145.37s)

So in average, the cost to calculate the re-run test is about ~5.87 extra seconds. Compare to the overall runtime of the command, this is about ~4.21% slow down.
The testQuick was triggered by modifying UnitTester.scala file, which will invalidate all test classes

@lihaoyi
Copy link
Member

lihaoyi commented Apr 13, 2025

Thanks @HollandDM ! Manually using the time command is fine. Is the 8.5s above for the Zinc-based analysis or the bytecode-based analysis? Do you have the approximate slowdown for the other one to compare?

@HollandDM
Copy link
Contributor Author

The time is for this PR, which mean it is Zinc based.
I'll do a proper measurement for the other, but from my previous try, bytecode-based analysis will take more time

@HollandDM
Copy link
Contributor Author

@lihaoyi I've update the ad-hoc bench result (the old one is accidentally wrong) and add bench result for the other implementation.

@HollandDM
Copy link
Contributor Author

We decided to move forward with #4731

@HollandDM HollandDM closed this Apr 19, 2025
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