Add coverage test - Githubissues

robotology / gz-sim-yarp-plugins

YARP plugins for Modern Gazebo (gz-sim).

BSD 3-Clause "New" or "Revised" License

8 stars 2 forks source link

Add coverage test #32

Closed lucapa17 closed 1 year ago

lucapa17 commented 1 year ago

Fix #21

lucapa17 commented 1 year ago

A few considerations:

sometimes macos test fails, like windows did. Maybe should we fix it together with #31 ?
the coverage test shows that we don't use yarp::dev::IAnalogSensor interface and its methods that we implemented taking inspiration from gazebo-yarp-plugins. Indeed we use only yarp::dev::ISixAxisForceTorqueSensors . Should we remove or keep it?
there are other lines of code that are not covered, such as the error handling ones. We don't know if we should try to cover also them

traversaro commented 1 year ago

Good job! I really like that you did not modified anything in the CMake files to add coverage support. For what regards IAnalogSensor methods, feel free to remove them, those interfaces are deprecated. For the rest, I think that for this software there is no strict need for 100% line coverage.

codecov[bot] commented 1 year ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (main@f498935). Click here to learn what that means. Patch has no changes to coverable lines.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #32 +/- ## ======================================= Coverage ? 78.78% ======================================= Files ? 5 Lines ? 231 Branches ? 0 ======================================= Hits ? 182 Misses ? 49 Partials ? 0 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

traversaro commented 1 year ago

Cool, it seems to work! Probably the error "❗ No coverage uploaded for pull request base (main@f498935). Click here to learn what that means." it is due to the fact that at the moment the base branch does not have coverage information, but that should be solved once we merge this PR.

traversaro commented 1 year ago

@lucapa17 @GiacomoBisio please remember either to cleanup the history of PRs before merging or (easier) merging PRs as squash, otherwise the history of the repo will become really difficult to navigate, thanks!

traversaro commented 1 year ago

@lucapa17 @GiacomoBisio please remember either to cleanup the history of PRs before merging or (easier) merging PRs as squash, otherwise the history of the repo will become really difficult to navigate, thanks!

To prevent errors, at the moment I forced "Squash and merge", if you want to allow merge commits let me know!