Improve coverage checks #466

l0b0 · 2021-06-19T04:45:40Z

Related Issue(s): #331

Description: Add tests, exclude some lines from coverage count, include tests in coverage count (highlighting unreached test code), and do some minor code changes to make it more testable.

The first PR already contains a bunch of orthogonal improvements. Let me know if you want to split it up into simpler pieces.

PR Checklist:

Code is formatted (run pre-commit run --all-files)
Tests pass (run scripts/test)
Documentation has been updated to reflect changes, if applicable
This PR maintains or improves overall codebase code coverage.
Changes are added to the CHANGELOG. See the docs for information about adding to the changelog.

codecov-commenter · 2021-06-19T04:47:25Z

Codecov Report

Merging #466 (daf22a0) into main (d51dd28) will decrease coverage by 2.96%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #466      +/-   ##
==========================================
- Coverage   91.48%   88.52%   -2.97%     
==========================================
  Files          40       40              
  Lines        5240     5201      -39     
  Branches        0      863     +863     
==========================================
- Hits         4794     4604     -190     
+ Misses        446      418      -28     
- Partials        0      179     +179

Impacted Files	Coverage Δ
pystac/extensions/sar.py	`98.96% <ø> (+1.03%)`	⬆️
pystac/extensions/view.py	`98.38% <ø> (+0.80%)`	⬆️
pystac/serialization/common_properties.py	`80.35% <0.00%> (-12.50%)`	⬇️
pystac/validation/schema_uri_map.py	`76.31% <0.00%> (-10.53%)`	⬇️
pystac/validation/stac_validator.py	`77.01% <0.00%> (-9.20%)`	⬇️
pystac/__init__.py	`92.68% <0.00%> (-7.32%)`	⬇️
pystac/extensions/hooks.py	`80.70% <0.00%> (-5.74%)`	⬇️
pystac/validation/__init__.py	`79.03% <0.00%> (-5.59%)`	⬇️
pystac/serialization/migrate.py	`81.96% <0.00%> (-5.34%)`	⬇️
pystac/collection.py	`90.53% <0.00%> (-4.98%)`	⬇️
... and 26 more

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 d51dd28...daf22a0. Read the comment docs.

duckontheweb

This looks good to me aside from using TestCases.get_path in some places.

I'm not sure what is going on with the Codecov.io reports. When I get the coverage report locally I get 93.17% coverage, but the report that's getting posted here shows 88.52% for this PR. I'll look into that separately and see if I can figure out the cause.

tests/test_catalog.py

tests/test_item_collection.py

tests/test_utils.py

Based on <https://stackoverflow.com/a/13477954/96588>. Also add tests to verify the exception being thrown.

By using the current file rather than the root of the project as the reference, the tests are now runnable for example in an IDE which uses the "tests" directory as the root.

It's already configured in .coveragerc.

Missing coverage in tests means unreached (and usually unreachable) test code, often indicating a test bug.

Stricter than statement coverage.

duckontheweb · 2021-06-26T10:38:35Z

I'm not sure what is going on with the Codecov.io reports. When I get the coverage report locally I get 93.17% coverage, but the report that's getting posted here shows 88.52% for this PR. I'll look into that separately and see if I can figure out the cause.

@l0b0 In #452 I created a separate coverage CI job for uploading to Codecov.io so that we could get coverage reports even when coverage drops to help us identify gaps (this happened specifically in 71f07d9). I did this by directly using the coverage run... command in the new job, but I'm realizing now that I probably should have updated scripts/test to allow us to override --fail-under instead so that we wouldn't have to update that command in 2 places.

I think the quick fix is to update the coverage CI job with the same changes you made to scripts/test. If you want to also update scripts/test so that we can use it in both cases that's great too 😄 , but I can do that separately.

duckontheweb · 2021-06-27T18:02:46Z

pystac/extensions/sar.py

-                f"SAR extension does not apply to type {type(obj)}"
+                f"SAR extension does not apply to type '{type(obj).__name__}'"


It looks like this change could be made for a number of other extensions as well. Is there a reason to only change this for the sar and view extensions, or should we update the ext method for all extensions in this PR?

I can do that, but I'd need to be sure that you're OK with changing all of them so it's not wasted effort. What do you think?

I could also pull these changes into a separate PR if that helps.

l0b0 · 2021-06-29T03:35:20Z

Closing in favour of smaller PRs.

l0b0 marked this pull request as ready for review June 21, 2021 23:38

l0b0 changed the title ~~Bump coverage~~ Improve coverage checks Jun 21, 2021

duckontheweb reviewed Jun 23, 2021

View reviewed changes

tests/test_catalog.py Outdated Show resolved Hide resolved

tests/test_item_collection.py Outdated Show resolved Hide resolved

tests/test_utils.py Outdated Show resolved Hide resolved

l0b0 added 9 commits June 24, 2021 08:09

tests: Verify empty observation_direction property

4d62151

feat: Print human readable type name in error messages

efa4923

Based on <https://stackoverflow.com/a/13477954/96588>. Also add tests to verify the exception being thrown.

tests: Fix test class name

9eb51ad

tests: Bump coverage minimum to 92%

75e97ac

tests: Exclude mypy-specific code from coverage

29395f6

tests: Make tests runnable from anywhere

c2dc9e5

By using the current file rather than the root of the project as the reference, the tests are now runnable for example in an IDE which uses the "tests" directory as the root.

refactor: Don't override source in test script

cbf574c

It's already configured in .coveragerc.

tests: Include tests in coverage metric

ace726c

Missing coverage in tests means unreached (and usually unreachable) test code, often indicating a test bug.

tests: Verify branch coverage

f5d9ce7

Stricter than statement coverage.

l0b0 force-pushed the bump-coverage branch 2 times, most recently from 3cada57 to f5d9ce7 Compare June 23, 2021 20:26

duckontheweb reviewed Jun 27, 2021

View reviewed changes

duckontheweb mentioned this pull request Jun 29, 2021

Run test script with/without coverage #486

Merged

5 tasks

This was referenced Jun 29, 2021

Verify empty observation direction property #488

Merged

tests: Fix test class name #489

Merged

l0b0 closed this Jun 29, 2021

l0b0 deleted the bump-coverage branch June 29, 2021 04:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve coverage checks #466

Improve coverage checks #466

l0b0 commented Jun 19, 2021 •

edited

Loading

codecov-commenter commented Jun 19, 2021 •

edited

Loading

duckontheweb left a comment

duckontheweb commented Jun 26, 2021

duckontheweb Jun 27, 2021 •

edited

Loading

l0b0 Jun 29, 2021 •

edited

Loading

l0b0 commented Jun 29, 2021 •

edited

Loading

		f"SAR extension does not apply to type {type(obj)}"
		f"SAR extension does not apply to type '{type(obj).__name__}'"

Improve coverage checks #466

Improve coverage checks #466

Conversation

l0b0 commented Jun 19, 2021 • edited Loading

codecov-commenter commented Jun 19, 2021 • edited Loading

Codecov Report

duckontheweb left a comment

Choose a reason for hiding this comment

duckontheweb commented Jun 26, 2021

duckontheweb Jun 27, 2021 • edited Loading

Choose a reason for hiding this comment

l0b0 Jun 29, 2021 • edited Loading

Choose a reason for hiding this comment

l0b0 commented Jun 29, 2021 • edited Loading

l0b0 commented Jun 19, 2021 •

edited

Loading

codecov-commenter commented Jun 19, 2021 •

edited

Loading

duckontheweb Jun 27, 2021 •

edited

Loading

l0b0 Jun 29, 2021 •

edited

Loading

l0b0 commented Jun 29, 2021 •

edited

Loading