Use multiple python coverage files #23
Conversation
* Combine everything found at ${PROJECT_BINARY_DIR}/.coverage*
|
@agutenkunst -- I'll test this out this weekend |
|
|
||
| # Create Python coverage report | ||
| add_custom_target(${Coverage_NAME}_py | ||
| COMMAND cp ${PROJECT_BINARY_DIR}/.coverage ${COVERAGE_DIR}/.coverage.nosetests || echo "WARNING: No nosetest coverage!" |
There was a problem hiding this comment.
ok, so this actually changes the functionality - we need to rename the .coverage file generated by catkin nose tests into .coverage.something or it gets overwritten and lost during the combine step below. I'd suggest we keep this line AND add your line to copy other files over. Even though the .coverage file will get copied twice, the second copy will get overwritten during the combine.
There was a problem hiding this comment.
Thanks for the quick reply. Sorry I oversaw this issue. I'll recheck with a repo containing both pytest and nosetest just to make sure and get back at you!
I don't really like that the two line solution would throw a warning if there are only pytest but this works as a pragmatic solution for now.
There was a problem hiding this comment.
Yeah, no issue - I didn't find it until I tested against our private repo - and then I had to think about why it wasn't working before I remembered why I was renamed .coverage to .coverage.nosetest (there should probably be a comment added there too explaining the rename, if you don't end up adding it in this PR, I'll add it in a future one).
There was a problem hiding this comment.
Maybe we can remedy all concerns by writing something like this:
if (EXISTS "${PROJECT_BINARY_DIR}/.coverage")
# Rename .coverage file generated by nosetests to avoid overwriting during combine step
COMMAND mv ${PROJECT_BINARY_DIR}/.coverage ${PROJECT_BINARY_DIR}/.coverage.nosetests
endif()
COMMAND cp ${PROJECT_BINARY_DIR}/.coverage* ${COVERAGE_DIR}/ || echo "WARNING: No python coverage!"
There was a problem hiding this comment.
@hslusarek that should work. Would suggest just adding "during combine step" to end of comment (so it is clear who/what would be overwriting).
cmake/Modules/CodeCoverage.cmake
Outdated
| add_custom_target(${Coverage_NAME}_py | ||
| COMMAND cp ${PROJECT_BINARY_DIR}/.coverage ${COVERAGE_DIR}/.coverage.nosetests || echo "WARNING: No nosetest coverage!" | ||
| # Rename .coverage file generated by nosetests to avoid overwriting during combine step | ||
| COMMAND if [ -f .coverage ]\; then mv ${PROJECT_BINARY_DIR}/.coverage ${PROJECT_BINARY_DIR}/.coverage.nosetests\; fi |
There was a problem hiding this comment.
I'll have to test this - but I'm thinking the "-f .coverage" part needs a ${PROJECT_BINARY_DIR}/ prefix, right?
There was a problem hiding this comment.
I'll have to test this - but I'm thinking the "-f .coverage" part needs a ${PROJECT_BINARY_DIR}/ prefix, right?
Of course, thanks for your comment. I added the prefix see d2c6fe3
|
@mikeferguson What's the status of this PR? Are there open questions from your point of view? |
|
@jschleicher - thanks for pinging - I had some trouble getting our CI to run this branch correctly - but resolved it by pushing the branch to my repo (wierd). Looks good now. |
|
@mikeferguson Thanks for merging. We would be happy if you could trigger a melodic-release for this change. |
|
0.4.3 now released into kinetic/melodic/noetic |
5 participants
Hi Mike!
This PR is related to machinekoder/ros_pytest#6 where it is needed in order to work.
Reasoning:
The current state requires coverage generating CMake-Macros (currently catkin_add_nosetests and add_pytests - if OPTIONS are defined correctly ) to store their coverage results at
${PROJECT_BINARY_DIR}/.coverageThis PR extends this so that multiple files (starting with ".coverage" can reside at
${PROJECT_BINARY_DIR}which all get collected and combined. Otherwise every coverage-generating Macro would have to pre-combine its results.Downwards compatible since
.coverageis still copied.