-
Notifications
You must be signed in to change notification settings - Fork 454
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
Native coverage integration for the CTest test controller #4094
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TSonono This looks pretty interesting, so I believe we'd be willing to consider taking this addition into the code, but we would definitely want tests.
From my reading, it looks like this is the test coverage tool for GCC, is that correct? Or does this work for other compilers?
What exactly does this do? I see that you've added another ctest run helper, as well as settings for what target to build before and after .What does running with this coverage do? Why do we need to specify targets to be built before and after?
Just some additional context and knowledge would be great for me to learn as we consider this. Thanks!
Some screenshots of this working on your end locally, would also be great to put in the description of the PR. |
Glad to hear! And I agree that tests make sense to include.
This implementation supports lcov coverage data files. lcov is a graphical front-end for GCC's coverage testing tool gcov. It's worth noting however that other compiler toolchains such as llvm clang are able to produce lcov coverage data files, for example from clang's documentation:
However, if this were to be merged, from my point of view one could also welcome adding support for other coverage data types in addition to the lcov format.
The settings for targets to be build before and after the tests are executed with coverage are intended for the user to specify utility targets in their CMake setup that prepares for a run with coverage as well as generating the appropriate coverage data files. See "Recommended procedure when capturing data for a test case:" in https://man.archlinux.org/man/lcov.1.en. Essentially, the user is expected to have utility targets that does something similar to this procedure. This is not something I would think the extension can handle, since it will probably look different for different projects. Therefore, it makes sense for the project to have utility targets that would have handle these operations. The extension essentially only handles step 2 (from the recommended procedure) and then the parsing of the files from the
Sure, I'll provide some screenshots or GIFs to show you how it works in the near future as I also work on adding tests. Let me know if there any more things I should clarify, or if there are any considerations regarding the design of the implementation. |
@TSonono Great! Thanks for the explanation. I'll wait on reviewing this further until tests are added, and then it could be useful to add some of the context you just mentioned, into a comment somewhere in the code you added. It doesn't have to be overly verbose, just enough so that when people come back to this for any implementation around this can easily understand. Thanks! |
Used to parse lcov coverage info files.
Needed for accessing the Test Coverage API in vscode
Test coverage implementation for the CTest test controller. It relies on lcov coverage info files being specefied by the user in the settings.json of the project. Optionally, the user can specify CMake (utility) targets that should be built before and/or after the tests are/have been executed. These targets could reasonably zero the coverage counters (pre) and filter the coverage info files (post).
a321e5d
to
2d53bdb
Compare
Also removed dynamic `--dynamic-list-data` linker flag
864215d
to
a488223
Compare
I've now added tests and some rationale notes to the code. Finally, I've added a GIF to the description showing how it works. @gcampbell-msft |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, this looks excellent! I appreciate the gif in the description as well.
I have just a couple minor questions that I'd like answers to before merging. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have disabled the coverage end-to-end tests on Windows since MSVC compiler is not able to produce gcov based coverage output from what I can tell.
MSVC can not produce gcov based coverage data, therefore the coverage end-to-end tests are disabled on Windows.
fd8886b
to
08020d9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last thing, please add a CHANGELOG.md entry. Thanks
Done |
This change addresses item #4040
This changes visible behavior and documentation
The following changes are proposed:
Other Notes/Information
This implementation only adds support for GCOV/LCOV based coverage data, but the implementation could be expanded in the future to support additional coverage data.