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 in initial support for code coverage #13880

Merged
merged 15 commits into from
Apr 13, 2022
Merged

Add in initial support for code coverage #13880

merged 15 commits into from
Apr 13, 2022

Conversation

ckipp01
Copy link
Member

@ckipp01 ckipp01 commented Nov 4, 2021

ℹ️ initial description kept for reference, see the follow-up for recent changes

This pr adds in the initial groundwork for code coverage as a compiler phase in response to the conversation here. Much of the work that is here was actually done as a POC by @Qjaquier a few years ago, and this basically continues the great work he started. There is a report that was written that explains much of this approach here as well.

This approach aims to inline the code coverage process up until writing instrumentation to disk, and then the rest of the process from reading up the coverage data, aggregating it, and reporting on it can now all be shared. This logic was de-coupled from the Scala 2 compiler plugin in scalac-scoverage-plugin and is already published as a milestone release to be tested with this. The same goes for the sbt-scoverage sbt plugin. You can read more about the changes that were necessary to make that work here. Most of the work up to this point was actually done there, not in this pr.

The way this works

The give a brief summary of how this will all work together, the initial step would be that the user (through a plugin) would enabled a -coverage flag, and then provide the output directory and also the sourceroot directory (if necessary, most of the time the default works fine). You can see how this is done here in the sbt plugin:

https://github.com/scoverage/sbt-scoverage/blob/d7df279c0a738988b770e529e5b4ba0454170ad6/src/main/scala/scoverage/ScoverageSbtPlugin.scala#L159-L168

Having the -coverage flag will enable the phase defined in CoverageTransformMacro.scala, which takes care of instrumenting the code, which will call out to Invoker.scala which takes care of serializing and writing the coverage data. Don't be alarmed by the diff count here, as 3k lines is actually the expect file, which I'll explain down below since I do have reservations about testing this way. At this point, after the coverage data is written, scoverage will take over and is published for 2.12, 2,13, and 3 (the domain, serializer, and reporter artifacts).

Trying this out

In order to try this out, you'll need to publish this branch locally. After doing that you can utilize the milestone releases via the sbt plugin to test. In order to make this easy I have a couple set up repos to that you can play around with:

Stuff left to figure out

At this point, I'm sort of hitting on the ends of my compiler knowledge and would love some insight into a couple things. My goal ultimately is to try and get this in the compiler not with a full feature-set covering everything, but with some workable basics that we can iterate over, and that hopefully others can help with 😄.

Testing

I tried to follow the way semanticDB is tested. You can do very similiar things:

Regenerate your expect file

scala3-compiler-bootstrapped/test:runMain dotty.tools.dotc.coverage.updateExpect

Run the tests with the bootstrapped compiler

scala3-compiler-bootstrapped/testOnly dotty.tools.dotc.coverage.CoverageTests

While this does work ok and I do think it's important to have some sort of test like this, this produces a giant single file, so any changes make finding what changed pretty awful. There should be a better diff functionality if we stick with this. There is also probably a lot more things we should test, but just to start I just threw in some of the examples from the scala3-example-project

Re-running coverage in sbt shell

One thing I don't fully know how to change is that in Scala 2-land you could enter the interactive sbt shell, run coverage, test, clean and then test again, and the second test would work as expected and again write your instrumentation to disk. The Invoker is short lived, and goes away after each test run. This doesn't work the same here as the Invoker seems to stick around when in interactive mode. So it ends up keeping the state from the last test and won't write to disk again. I'm not fully sure how to get around this or if there is some sort of hook that I can tie into and clean up the invoker to ensure that the next run will work as expected. Again, this only pops up when you are in a long running session. In CI or one-off runs don't hit on this.

Unhandled trees

The set of trees that are instrumented probably aren't complete. I left a warning in there just to show what I mean. For example if you run this you'll see warnings about constructs that aren't handled. There still needs to be a bit of work about which ones are fine not to handle and which ones need to be handled.

Bare bones

For the moment, this is a pretty bare bones feature set. Some things that will likely need to be added in the future are things like ignoring packages, files, symbols. All things that exist in the Scala 2 version, but not yet here.

TODOs

Just to keep track of a few things

  • Test the Invoker
  • Finalize how we test the coverage data, is what we have ok?
  • What other trees need to be handled before merging
  • Add some documentation

All in all, I'd love some feedback on this. I've learned a lot diving into this, and I'm excited to hopefully get this into the compiler. I admire the desire of the Dotty team to upstream as much as possible. It'd a decision that will have huge beneficial ramifications for the future of Scala.

@ckipp01
Copy link
Member Author

ckipp01 commented Nov 27, 2021

I wanted to follow back up on this. I'm sort of waiting on some feedback before bringing this any further. Mainly is the approach ok, any thoughts on the testing questions I brought up, etc. Once I sort of get the green light I can then address whatever we feel is remaining in order to get this to a point that it's mergeable.

@nicolasstucki nicolasstucki added the needs-minor-release This PR cannot be merged until the next minor release label Dec 1, 2021
@nicolasstucki nicolasstucki modified the milestone: 3.2.0 Dec 1, 2021
@ches
Copy link

ches commented Dec 5, 2021

Perhaps this remark belongs in Discourse if there is a need for discussion about whether it belongs in the compiler or whether the community can devise an interim migration plan for scoverage until it gets there (including 3.{0,1}.x), but I'd just like to add in support:

In my large organization with Scala projects/services reaching hundreds, scoverage is a quality assurance standard. Coverage is an imperative piece of the tooling story for industrial Scala 3 migration.

Kudos to @ckipp01 for pushing the matter forward!

@ckipp01
Copy link
Member Author

ckipp01 commented Dec 30, 2021

So just to be transparent I probably won't be working on this any time soon. It is quite close to being finished, there are already Milestone releases on both the sbt scoverage plugin and scoverage as outlined in the description, so if someone does want to pick this up, please do. I'll do whatever I can with helping getting the scoverage part reviewed and released.

@anatoliykmetyuk anatoliykmetyuk added the Spree Suitable for a future Spree label Jan 10, 2022
@anatoliykmetyuk
Copy link
Contributor

@ckipp01 would you be interested to mentor someone on the Issue Spree on this one?

@ckipp01
Copy link
Member Author

ckipp01 commented Jan 10, 2022

@ckipp01 would you be interested to mentor someone on the Issue Spree on this one?

Hey @anatoliykmetyuk for sure. However, I think it'd be best to also have someone that knows the actual compiler better than myself also be part of it. The majority of the actual scoverage part is done, it's mainly finishing up dealing with the compiler parts.

@smarter
Copy link
Member

smarter commented Jan 10, 2022

For anyone interested in working on this, note that Chris gave a talk on this work at ScalaCon which is now online! https://www.youtube.com/watch?v=SIkNgemGmYQ

@anatoliykmetyuk anatoliykmetyuk added the stat:unassigned This issue or PR is not assigned to anyone, but should be label Jan 25, 2022
@andreisilviudragnea
Copy link

Does this implementation support interpolated strings too? The Scala 2 one does not: scoverage/scalac-scoverage-plugin#448

@TheElectronWill
Copy link
Contributor

TheElectronWill commented Mar 4, 2022

I intend to finish this!

TODO:

  • make it compile on latest main
  • cleanup phase definition (InstrumentCoverage)
  • try new, simpler strategy for handling function calls
  • handle more trees
  • see if something can be done about interpolated strings
  • avoid global state
  • test (any advice on how to replace the big "expect" file, if it's not the best solution?)
  • fix the tastyBootstrapTest (and, to do so, improve error reporting)
  • do something about the Invoker's code being 8 years old and not counting properly?: some solutions (to fix the counting) would require to modify scoverage, we'll see
  • fix weird bugs found on the way
  • optimize: instrument literals only where necessary
  • add Synthetic flag to trees where necessary

The coverage option is now named -coverage-out, and coverage-sourceroot is removed (you can use the existing option -sourceroot).

@TheElectronWill TheElectronWill self-assigned this Mar 4, 2022
@TheElectronWill TheElectronWill removed the stat:unassigned This issue or PR is not assigned to anyone, but should be label Mar 4, 2022
@TheElectronWill
Copy link
Contributor

TheElectronWill commented Mar 11, 2022

Update: I'm digging into the tastyBootstrapTest to understand why it fails. Once it's fixed, I'll mark this PR as ready.

I have some proposals to improve the coverage, mainly to make the Invoker count correctly while improving the performance. This require some discussion with the scoverage folks.

The idea is:

  1. Since we generate the lists of statements that can be covered, we know the id range 0...N in advance. Thus, we could use an AtomicIntegerArray per directory, to count how many times each statement is called.

  2. Since scoverage controls the sbt plugin, we can know when the coverage checks start and end, much more reliably than the JVM shutdown hooks (I've seen a comment in the coverage code somewhere about shutdown hooks). Therefore, we can pass N to the invoker at the beginning of the coverage task, and tell the Invoker to dump all its data at the end.

Benefits:

  • accurate count per statement
  • maximal performance, both for counting and for saving the results

ping @ckipp01 Does it seem possible? Is it quick to implement?

Maybe it's best if I finish this PR with an incorrect count (as incorrect as the scoverage for Scala 2), and open others for the new improvements.

@ckipp01
Copy link
Member Author

ckipp01 commented Mar 11, 2022

ping @ckipp01 Does it seem possible? Is it quick to implement?

Maybe it's best if I finish this PR with an incorrect count (as incorrect as the scoverage for Scala 2), and open others for the new improvements.

To be honest I'm not really sure without digging into it. As for incorrect counts in general, I'll be honest, I'm not really sure how large of an issue this is, since I'm unsure if there is actually a lot of complaints or issues related to this. After taking over releases and small maintenance tasks with scoverage, I've never actually done a deep dive into some of these issues.

@godenji
Copy link

godenji commented Mar 11, 2022

Maybe it's best if I finish this PR with an incorrect count (as incorrect as the scoverage for Scala 2), and open others for the new improvements.

👍 the sooner we have usable Scala 3 test coverage reports the better (i.e. don't let perfect be the enemy of the good).

Thanks for pushing this to the finish line @TheElectronWill !!

@TheElectronWill TheElectronWill added in progress and removed Spree Suitable for a future Spree labels Mar 12, 2022
According to my JMH benchmarks, this improves the performance by at least 40%,
even when multiple threads use the Invoker.

Details on the benchmark:
- 10k calls to Invoker.invoked(id,dir) with a % of ids that are repeted in a loop and a % of ids that appear only
once. But this doesn't change the results much.
- 1 thread or 4 threads at the same time. The single-thread performance
is, as expected, much better (3x).
Also add more tests and fix the remaining todo
Now use
scala3-compiler-bootstrapped/testCoverage [--update-checkfiles]
Copy link
Member

@smarter smarter left a comment

Choose a reason for hiding this comment

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

LGTM, huge thanks to everyone who contributed! 🎆

@smarter smarter enabled auto-merge April 13, 2022 22:02
@smarter smarter added the release-notes Should be mentioned in the release notes label Apr 13, 2022
@smarter smarter merged commit 93fc41f into scala:main Apr 13, 2022
@ckipp01
Copy link
Member Author

ckipp01 commented Apr 14, 2022

Really fantastic job bringing this across the finish line @TheElectronWill 🚀

@ckipp01 ckipp01 deleted the coverage branch April 14, 2022 06:35
@TheElectronWill
Copy link
Contributor

Thank you for the initial work!

@jozic
Copy link

jozic commented Oct 13, 2022

Hello folks, are there any plans to support exclusion from coverage feature from original scoverage?
If not, where should one start looking to make it happen?

@ckipp01
Copy link
Member Author

ckipp01 commented Oct 13, 2022

@jozic I don't see a feature request for it yet, so I'd recommend creating one for it in this repo.

@TheElectronWill
Copy link
Contributor

I agree, a new issue should be created for it. It shouldn't be too hard to implement, I think :)

@jozic
Copy link

jozic commented Oct 13, 2022

#17684

@Kordyjan Kordyjan modified the milestones: 3.2.0-RC1, 3.2.0 Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-minor-release This PR cannot be merged until the next minor release release-notes Should be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.