search

go-lang-plugin-org / go-lang-idea-plugin

Google Go language IDE built using the IntelliJ Platform
https://plugins.jetbrains.com/plugin/5047
Other
4.56k stars 570 forks source link

Implement recursive package coverage #2880

Closed lowjoel closed 6 years ago

lowjoel commented 7 years ago

As requested in https://youtrack.jetbrains.com/issue/GO-3620 by @zolotov, this implements support for recursive package coverage with line annotations.

This uses the package-coverage tool: https://github.com/corsc/go-tools/tree/master/package-coverage

zolotov commented 7 years ago

The solution is good, thank you! Although implementation and UX might be improved.

  1. TestRunningState is not the best place for merging coverage report and looking for special executable. It would be better to have this functionality closer to GoCoverageRunner and GoCoverageProgramRunner
  2. User will never know how to run coverage on a directory. It would be better if GoCoverageRunner could recognize running on directory and suggest to download and install missing package and make sure its executable is in PATH.
  3. How good does this work on windows?

As a solution of the second bullet, we can actually bundle package-coverage executable.

zolotov commented 7 years ago

Also, if you're going to continue on this PR, please sign https://www.jetbrains.com/agreements/cla/ first. Thank you.

lowjoel commented 7 years ago

Hi @zolotov yep, I've signed the CLA.

I'll try to refactor the code to solve Point 1; I think for Point 2 I'd need some guidance from you because that process is pretty unfamiliar to me. We can talk over Gitter if that's good for you.

I'm trying everything out on a Windows machine right now, I'll post updates here.

lowjoel commented 7 years ago

@zolotov: I've done some refactoring to the code, hope this is better :)

This works quite well on Windows, once the package-coverage binary is in place.

lowjoel commented 7 years ago

One more change to display a notification when the package-coverage binary isn't installed.

zolotov commented 7 years ago

Thank you, looks much better. Please make some updates in PR and I'll happily merge it

lowjoel commented 7 years ago

@zolotov I've tried my best to address your comments, have a look at one last question in the review.

lowjoel commented 7 years ago

@zolotov let me know if the final point is a deal breaker. If it is, I'll close it and will wait for you all to implement coverage.

lowjoel commented 7 years ago

@zolotov: strangely after running the IDE today, subsequent builds fail with Cannot add a configuration with name 'idea' as a configuration with that name already exists. I've already cleared my Gradle caches... any idea what's up? Google doesn't come up with anything useful.

FAILURE: Build failed with an exception.

* What went wrong:
A problem occurred configuring root project 'intellij-go'.
> A problem occurred configuring project ':google-app-engine:google-app-engine-yaml'.
   > Cannot add a configuration with name 'idea' as a configuration with that name already exists.

* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output.
lowjoel commented 7 years ago

OK, upgrading the Gradle Intellij plugin seems to work.

lowjoel commented 7 years ago

@zolotov: I've removed the process listener and the coverage merger, since goverage handles that internally. How does this look now?

zolotov commented 7 years ago

Looks good so far. I need to take a look how good it work and will merge it after that. Thank you!

lowjoel commented 6 years ago

Sorry, I really didn't have the time to work on this. Since 1.10 will fix this, let's use your implementation in GoLand. Thanks for the review!

zolotov commented 6 years ago

@lowjoel sure. Thanks for an attempt!