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 doctests to work with pytest. #802

Merged
merged 10 commits into from
Oct 30, 2021
Merged

Fix doctests to work with pytest. #802

merged 10 commits into from
Oct 30, 2021

Conversation

bdice
Copy link
Member

@bdice bdice commented Jun 24, 2021

Description

Examples in docstrings were not being tested since the switch to pytest.

This is because the load_tests function is not recognized by pytest, only unittest.

I attempted to use pytest --doctest-modules but that isn't quite what we want, and it appears to only work with pure Python (not Cython) modules.

Instead, I adopted a variation on the strategy proposed in this StackOverflow post:
https://stackoverflow.com/questions/16982514/python-test-discovery-with-doctests-coverage-and-parallelism

Motivation and Context

Ensures that docstrings are runnable. I noticed the issue while reviewing #793.

How Has This Been Tested?

Tests should pass and show output like test_docstrings[freud.box.Box.center].

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds or improves functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation improvement (updates to user guides, docstrings, or developer docs)

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • I have updated the documentation (if relevant).
  • I have added tests that cover my changes (if relevant).
  • All new and existing tests passed.
  • I have updated the credits.
  • I have updated the Changelog.

@bdice bdice requested a review from a team as a code owner June 24, 2021 13:22
@bdice bdice requested review from alacour and removed request for a team June 24, 2021 13:22
@bdice bdice added bug Something isn't working testing & validation labels Jun 24, 2021
@vyasr
Copy link
Collaborator

vyasr commented Jun 25, 2021

The current error looks to be related to the upload of coverage artifacts I think. FYI, the Windows test is a false positive, the tests actually failed on import but it's reporting a success on this dashboard anyway.

For some reason all doctests are doubled up, with a copy in some *.__test__.* namespaces. I suspect it's related to pytest-dev/pytest#1558, but I've never used this feature of pytest to skip tests before so I'm not sure. It looks like all doctests are somehow being added twice.

@bdice
Copy link
Member Author

bdice commented Jun 25, 2021

@vyasr I couldn’t figure out how the __test__ module attribute is created. The previous tests were also doubled for the same reason. I think it might be a Cython thing. The docstring tests don’t point to line numbers but the __test__ tests do have pyx line info, so it is probably being generated at Cython compilation time.

I’ll look at the failing tests later, I have a lot going on right now. Anyone can feel free to take a look or push to this branch.

@bdice
Copy link
Member Author

bdice commented Oct 17, 2021

I got the doctests to run successfully, but I am seeing an unrelated error in CI from either coverage or pytest-cov. I commented on this related issue to document the problem: nedbat/coveragepy#1160

@glotzerlab/freud-maintainers Since I'm not sure if our code coverage is being reported correctly anyway, I think the next step is to disable coverage/codecov before merging this PR. Thoughts on this? See also: codecov deprecation: glotzerlab/signac#630

@bdice
Copy link
Member Author

bdice commented Oct 19, 2021

The issue upstream in coverage has been fixed:

This PR can be merged once a new version of coverage is released and we upgrade the pinning. I don't really want to disable coverage in our CI (even though it's not currently working), but that would be another option.

@bdice bdice added the blocked label Oct 19, 2021
@bdice bdice added this to the v2.8.0 milestone Oct 19, 2021
@bdice bdice self-assigned this Oct 19, 2021
@bdice bdice removed the blocked label Oct 30, 2021
@codecov
Copy link

codecov bot commented Oct 30, 2021

Codecov Report

Merging #802 (1c42014) into master (76b2ced) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #802   +/-   ##
=======================================
  Coverage   55.34%   55.34%           
=======================================
  Files          16       16           
  Lines        2564     2564           
  Branches       34       34           
=======================================
  Hits         1419     1419           
  Misses       1131     1131           
  Partials       14       14           
Impacted Files Coverage Δ
freud/diffraction.pyx 61.70% <ø> (ø)

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 76b2ced...1c42014. Read the comment docs.

@bdice bdice merged commit 78cde33 into master Oct 30, 2021
@bdice bdice deleted the fix/doctests-pytest branch October 30, 2021 19:19
@bdice
Copy link
Member Author

bdice commented Oct 30, 2021

@tommy-waltmann I went ahead and merged this since it affects tests and had been stalled. We want any new PRs to have their documentation examples tested. This was blocked until coverage 6.1 was released, which fixed the failing tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working testing & validation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants