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

create code coverage reports #2102

Closed
wants to merge 9 commits into from

Conversation

tolot27
Copy link
Collaborator

@tolot27 tolot27 commented May 6, 2022

With this PR code coverage reports are created using JaCoCo and uploaded to CodeCov for visualization. An example is shown for my fork at https://codecov.io/gh/tolot27/xDrip.

Also, code coverage comments in the Diff layout together with the impact on each changed file are added to new pull requests to get an impression how the overall code coverage increases/decreases with the suggested changes.

@tolot27 tolot27 added enhancement code:quality code and repository related testing labels May 6, 2022
@codecov-commenter
Copy link

codecov-commenter commented May 6, 2022

Codecov Report

❗ No coverage uploaded for pull request base (master@6434248). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff            @@
##             master   #2102   +/-   ##
========================================
  Coverage          ?   6.72%           
  Complexity        ?    1255           
========================================
  Files             ?     784           
  Lines             ?   64498           
  Branches          ?    9848           
========================================
  Hits              ?    4340           
  Misses            ?   59582           
  Partials          ?     576           

@tolot27 tolot27 requested a review from jamorham May 10, 2022 10:38
@tolot27
Copy link
Collaborator Author

tolot27 commented Oct 25, 2022

@jwoglom Please can you review this PR?

Copy link
Contributor

@jwoglom jwoglom left a comment

Choose a reason for hiding this comment

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

looks good, certainly serves as an incentive to push the number a lot higher :)

@jamorham
Copy link
Collaborator

jamorham commented Nov 2, 2022

I'm not sure of the value of code coverage reports for xDrip. It is certainly good to encourage unit tests to be provided for new components or added for existing ones where appropriate, but there are also many areas which are not unit-testable.

Code coverage reports are also available already in android studio.

I've worked on projects before which demanded 100% unit test coverage and it was quite common to see developers just submit "cooked" tests simply to pass the coverage check and which added little value to the overall code quality and could even make things worse.

I think a much higher priority would be to get integration tests up and running again to test that any changes do not impact on the functional quality of the app. For example if I break the android manifest with a change it might not show up until runtime and we don't currently test for that. I previously had some tests for this but they no longer run due to restrictions of the environment used for automated unit testing.

@tolot27
Copy link
Collaborator Author

tolot27 commented Nov 4, 2022

I'm not sure of the value of code coverage reports for xDrip. It is certainly good to encourage unit tests to be provided for new components or added for existing ones where appropriate, but there are also many areas which are not unit-testable.
Code coverage reports are also available already in android studio.

All of this is true. Nevertheless, code coverage (CC) does not hurt and the impact on the GitHub CI checks is minimal.

it was quite common to see developers just submit "cooked" tests simply to pass the coverage check

For this we have a review process and just one merger. May/the goal of CC is get an overview of the current CC for everyone without pulling a specific PR and looking at the CC in Android Studio. It can be helpful during the review and it can encourage the PR submitters to create/enhance unit tests. It is up to us to enforce unit testing or not. Nevertheless, CC is a good measurement tool.

I think a much higher priority would be to get integration tests up and running again to test that any changes do not impact on the functional quality of the app. For example if I break the android manifest with a change it might not show up until runtime and we don't currently test for that. I previously had some tests for this but they no longer run due to restrictions of the environment used for automated unit testing.

I fully agree and CC is a helpful tool to get an overview of it. Last weekend I get the PowerMock integration test running and started to create unit tests for NS to address #2372.

@tolot27 tolot27 closed this Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code:quality code and repository related enhancement testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants