Skip to content

Conversation

@smithtim
Copy link
Collaborator

@smithtim smithtim commented Oct 2, 2020

This PR addresses issue #8 by adding an option skip-unknown-schemas as suggested there, defaulting to false.

@smithtim smithtim requested a review from rkaippully October 2, 2020 19:02
@codecov-commenter
Copy link

Codecov Report

Merging #15 into master will decrease coverage by 0.33%.
The diff coverage is 93.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #15      +/-   ##
==========================================
- Coverage   96.57%   96.24%   -0.34%     
==========================================
  Files           3        3              
  Lines         263      266       +3     
  Branches        8        9       +1     
==========================================
+ Hits          254      256       +2     
  Misses          1        1              
- Partials        8        9       +1     
Impacted Files Coverage Δ
src/scim_patch/core.clj 96.38% <93.33%> (-0.55%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1adc1e5...bb853ff. Read the comment docs.

@smithtim
Copy link
Collaborator Author

smithtim commented Oct 7, 2020

Does this look good, or do we need to address the code coverage?

Copy link
Collaborator

@JasonStiefel JasonStiefel left a comment

Choose a reason for hiding this comment

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

Would be nice to have more coverage, but that shouldn't stop this going through. the functionality is tested.

@rkaippully
Copy link
Owner

I would like to keep coverage as high as possible, preferably above 97%. That's the only way we can make sure we don't introduce regressions. In the current case, it has covered 93.33% of the changes introduced in this PR and there is room for improvement.

If you go to https://codecov.io/gh/rkaippully/scim-patch/compare/eb360c6365d346a44716045a46eec6febd75a46e...f97b22718bc1a45db23f5e4f4763ebd294bdecdb/diff you can see lines/forms that don't have 100% coverage in yellow or red. BTW, you can run this locally with lein cloverage and get the same report.

@smithtim
Copy link
Collaborator Author

I ran lein cloverage for this branch, but I did not get the same report. Here is what I got:

|-------------------+---------+---------|
|         Namespace | % Forms | % Lines |
|-------------------+---------+---------|
|   scim-patch.core |   98.39 |   99.41 |
| scim-patch.filter |   96.69 |  100.00 |
|  scim-patch.paths |   96.36 |  100.00 |
|-------------------+---------+---------|
|         ALL FILES |   97.75 |   99.63 |
|-------------------+---------+---------|

Thus, it seems that the coverage is already above 97%. Am I missing something?

@rkaippully
Copy link
Owner

Sorry, let me clarify.

There are two coverage metrics tracked here - codecov/patch tracks what percentage of lines changed in this PR have test coverage and codecov/project tracks what percentage of the total lines in the project have code coverage.

Besides, there is a difference between the output of lein cloverage and codecov. The former tracks all lines in the source file. The latter omits blank lines, comments etc from the calculation. That's why you are seeing a difference in the numbers.

For now, I'll relax the settings so that this PR can be merged. I am trying to make the situation better in #17 and add some more tests so that the coverage is above 97% (it is arbitrary, but we have to draw the line somewhere).

@rkaippully rkaippully merged commit f6e7eab into master Oct 22, 2020
@rkaippully rkaippully deleted the optional-skip-unknown branch October 22, 2020 03:48
@smithtim
Copy link
Collaborator Author

Thanks, Raghu!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants