Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Move tests out of package #1459

Merged
merged 11 commits into from
May 2, 2024
Merged

Move tests out of package #1459

merged 11 commits into from
May 2, 2024

Conversation

flying-sheep
Copy link
Member

@flying-sheep flying-sheep commented Apr 9, 2024

To be determined if done now or later:

  • replace toplevel conftest with testing.anndata._pytest (or so) private pytest plugin

To do later

  • create documented public test helpers as part of testing.anndata

Copy link

codecov bot commented Apr 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.25%. Comparing base (20535bf) to head (1ed873f).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1459      +/-   ##
==========================================
+ Coverage   84.22%   84.25%   +0.03%     
==========================================
  Files          35       35              
  Lines        5685     5685              
==========================================
+ Hits         4788     4790       +2     
+ Misses        897      895       -2     

see 1 file with indirect coverage changes

@ivirshup
Copy link
Member

ivirshup commented Apr 9, 2024

What's up with the coverage?

@flying-sheep
Copy link
Member Author

pytest --cov instead of coverage run. Although because of nedbat/coveragepy#1696, coverage run is also not optimal.

coverage run always works, whereas pytest --cov breaks if you somehow get pytest to import your module before it initializes the plugin.

Copy link
Member

@ivirshup ivirshup left a comment

Choose a reason for hiding this comment

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

More specifically, the coverage is different here due to the changes to filterwarnings in the pyproject.toml. I significantly prefer being able to run tests in parallel (which I believe isn't really possible with coverage) locally over specifying warnings to filter by class in the pyproject.toml.

I've made a suggestion which still allows parallel testing and coverage collection.

pyproject.toml Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
@flying-sheep
Copy link
Member Author

flying-sheep commented Apr 25, 2024

OK, I think this looks good. It’s blocked on scverse/scanpy#2993, which is in turn blocked on pytest’s 8.2 release (pytest-dev/pytest#12213).

/edit: unblocked: Pytest 8.2 is released

Copy link
Contributor

@ilan-gold ilan-gold left a comment

Choose a reason for hiding this comment

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

I still get CoverageWarning: Module anndata was previously imported, but not measured (module-not-measured) locally. Is this intentional or expected? Should we catch it?

Otherwise I was able to run the tests locally, in parallel, getting a codecov report that has non-zero line-rate for the various submodules. Is there a tool for visualizing this xml? I couldn't find anything

@flying-sheep
Copy link
Member Author

flying-sheep commented Apr 29, 2024

You can use coverage html instead of coverage xml to get a visual report locally. But you can also just check if the newest commit hash matches what the codecov comment says, and then check the link from the codecov comment: https://app.codecov.io/gh/scverse/anndata/pull/1459

Seems like somehow this PR increases coverage very very slightly?

I still get CoverageWarning: Module anndata was previously imported, but not measured (module-not-measured) locally.

No clue why that would be. Maybe something to do with running stuff parallel. Are you sure that warning is new?

As said, ba3c054 fixed the issue that resulted in anndata being imported before pytest-cov, so now this PR doesn’t negatively affect coverage anymore.


The only reason coverage is relevant to reviewing this PR is because it caused me to add def pytest_configure instead of some lines of filterwarnings, and because a subtle bug in earlier commits here caused me to bump the minimum in .codecov.yml.

@ilan-gold
Copy link
Contributor

As said, ba3c054 fixed the issue that resulted in anndata being imported before pytest-cov, so now this PR doesn’t negatively affect coverage anymore.

I am seeing it in parallel, but can't find the issue about it which I saw earlier so am not 100% sure if that's expected.

My coverage looks good in parallel and similar to the one not in parallel, with the only coverage change being the test helpers. So to me, this PR seems fine.

@flying-sheep flying-sheep merged commit b3763f8 into main May 2, 2024
16 checks passed
@flying-sheep flying-sheep deleted the test-dir branch May 2, 2024 09:32
flying-sheep added a commit that referenced this pull request May 2, 2024
(cherry picked from commit b3763f8)
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move tests out of the package
3 participants