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

fix coverage measurement #138

Merged
merged 4 commits into from
Mar 7, 2023
Merged

Conversation

braingram
Copy link
Collaborator

@braingram braingram commented Mar 3, 2023

The coverage measurement does not appear to be working correctly.

For example, this function:

def write(filename, tree, hdulist=None, **kwargs):

is covered by this test:

asdf_in_fits.write(fn, tree, hdulist)

The test does appear to be running (confirmed locally) and coverage reports run locally outside tox (with a development install of the package) show increased coverage.

However this function does not appear to have coverage in the CI.
https://app.codecov.io/gh/spacetelescope/stdatamodels/blob/master/src/stdatamodels/asdf_in_fits.py

I tested a few of the solutions mentioned here, which describe how pytest will test the installed package yet coverage is measured against the current directory (for our current configuration):
https://stackoverflow.com/questions/58696476/tox-0-coverage

and the 'usedevelop' option appears to improve the coverage measurement.

Checklist

  • added entry in CHANGES.rst (either in Bug Fixes or Changes to API)
  • updated relevant tests
  • updated relevant documentation
  • updated relevant milestone(s)
  • added relevant label(s)

@braingram braingram added the no-changelog-entry-needed Trivial change that doesn't need an entry in the change log label Mar 3, 2023
@braingram braingram requested a review from zacharyburnett March 3, 2023 19:15
@braingram braingram marked this pull request as ready for review March 3, 2023 19:15
@codecov
Copy link

codecov bot commented Mar 3, 2023

Codecov Report

Patch coverage has no change and project coverage change: +22.61 🎉

Comparison is base (26d637e) 50.32% compared to head (07c3aa1) 72.93%.

Additional details and impacted files
@@             Coverage Diff             @@
##           master     #138       +/-   ##
===========================================
+ Coverage   50.32%   72.93%   +22.61%     
===========================================
  Files         123       98       -25     
  Lines        7768     5402     -2366     
===========================================
+ Hits         3909     3940       +31     
+ Misses       3859     1462     -2397     
Flag Coverage Δ
unit 72.93% <ø> (+22.61%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
tests/test_util.py
tests/test_resources.py
tests/models.py
...rc/stdatamodels/jwst/datamodels/tests/test_fits.py
.../stdatamodels/jwst/datamodels/tests/test_models.py
tests/test_storage.py
src/stdatamodels/jwst/datamodels/tests/test_wcs.py
tests/test_s3_utils.py
tests/test_schema.py
...ls/jwst/transforms/converters/tests/test_models.py
... and 65 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@braingram
Copy link
Collaborator Author

I'm not sure if this is best strategy but it does appear to fix the issue with the example:
https://github.com/spacetelescope/stdatamodels/actions/runs/4326528146/jobs/7554058631#step:12:178

@zacharyburnett
Copy link
Collaborator

zacharyburnett commented Mar 3, 2023

I think I remember something about ASDF messing with coverage when running pytest-cov

EDIT: here: https://github.com/asdf-format/asdf/blob/3c83a49de6ab7455a4cd2bcc4aea65704a582db3/tox.ini#L27-L29

# coverage run must be used because the pytest-asdf plugin will interfere
# with proper coverage measurement due to the order pytest loads its
# entry points.
commands =
    coverage: coverage run --source=asdf --rcfile={tox_root}/pyproject.toml -m \
    pytest \

Is pytest-asdf used here?

@WilliamJamieson
Copy link
Contributor

I think I remember something about ASDF messing with coverage when running pytest-cov

EDIT: here: asdf-format/asdf@3c83a49/tox.ini#L27-L29

# coverage run must be used because the pytest-asdf plugin will interfere
# with proper coverage measurement due to the order pytest loads its
# entry points.
commands =
    coverage: coverage run --source=asdf --rcfile={tox_root}/pyproject.toml -m \
    pytest \

Is pytest-asdf used here?

This is only relevant to asdf itself. Its because asdf gets loaded by the pytest-asdf plugin, which gets loaded before pytest-cov gets loaded.

tox.ini Outdated Show resolved Hide resolved
Copy link
Collaborator

@zacharyburnett zacharyburnett left a comment

Choose a reason for hiding this comment

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

LGTM, not sure why it works but if it does then I won't complain

@WilliamJamieson
Copy link
Contributor

To make the coverage more accurate you might want to consider configuring coverage to omit some files from consideration, see: https://github.com/asdf-format/asdf/blob/3c83a49de6ab7455a4cd2bcc4aea65704a582db3/pyproject.toml#L123-L152

@braingram
Copy link
Collaborator Author

Also, jwst CI failure expected because of the version conflict and pkg_resources use in stpipe: spacetelescope/stpipe#84

To make the coverage more accurate you might want to consider configuring coverage to omit some files from consideration, see: https://github.com/asdf-format/asdf/blob/3c83a49de6ab7455a4cd2bcc4aea65704a582db3/pyproject.toml#L123-L152

Good idea. Any suggestions for files in src to ignore?

@WilliamJamieson
Copy link
Contributor

Good idea. Any suggestions for files in src to ignore?

Really we should be ignoring any of the test files, so ./tests and then any of the test directories in src. At some point those test directories can probably be factored out into the ./tests directory. Currently they are just an artifact of the move of jwst.datamodels and jwst.transforms.

@WilliamJamieson
Copy link
Contributor

Also, if you do end-up modifying the pyproject.toml file could you sneak an --color=yes into the pytest configuration's adopts? Like: https://github.com/asdf-format/asdf/blob/3c83a49de6ab7455a4cd2bcc4aea65704a582db3/pyproject.toml#L121

I find this handy so that if I browse to an on-going job I can tell if something has already failed in the job because the output will start displaying in red text. This makes it easy to tell if something has gone wrong for long CI jobs.

If this is too out of scope I can make it a separate PR.

@braingram braingram merged commit 6483ee2 into spacetelescope:master Mar 7, 2023
@braingram braingram deleted the ci/coverage branch March 7, 2023 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog-entry-needed Trivial change that doesn't need an entry in the change log
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants