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

improvements for coverage_doctest.py #22429

Open
smichr opened this issue Nov 6, 2021 · 9 comments
Open

improvements for coverage_doctest.py #22429

smichr opened this issue Nov 6, 2021 · 9 comments
Labels
Testing Related to the test runner. Do not use for test failures unless it relates to the test runner itself

Comments

@smichr
Copy link
Member

smichr commented Nov 6, 2021

Some deficiencies in the running of coverage_doctest.py have been noted by @asmeurer in #22426:

  • the SPHINX score is not reporting correctly (or they are not being tested)
======================================================================
TOTAL DOCTEST SCORE for sympy: 83% (13821 of 16531)
TOTAL SPHINX SCORE for sympy: 0% (0 of 16531)
  • the tests are not reported in alphabetical order (like other tests are).
@smichr smichr added the Testing Related to the test runner. Do not use for test failures unless it relates to the test runner itself label Nov 6, 2021
@oscarbenjamin
Copy link
Contributor

I don't understand what this issue is for. Can it be made clearer? Otherwise it isn't useful to have an issue like this and it should be closed.

@asmeurer
Copy link
Member

asmeurer commented Nov 6, 2021

It's for the script bin/coverage_doctest.py. My comments were originally made on #22426

@s-hanko
Copy link
Contributor

s-hanko commented Feb 18, 2022

Hello, since I worked on a previous issue with coverage_doctest I was thinking that I might try this one too.

Just to clarify, this program is supposed to look for example code blocks in the sphinx HTML files, right?

@asmeurer
Copy link
Member

asmeurer commented Apr 7, 2022

For Sphinx coverage, we might consider using https://www.sphinx-doc.org/en/master/usage/extensions/coverage.html instead. I haven't tested it, so we should check that it can do everything that we need.

@s-hanko
Copy link
Contributor

s-hanko commented Apr 15, 2022

I tried adding that extension to conf.py but I couldn’t figure out where it puts its output, unfortunately it doesn’t seem to have much documentation.

It looks like it checks that every module has generated sphinx documentation, so I think it might be useful to add if we can find the output. I’ve been making coverage_doctest.py check specifically whether every module has a code example in the Sphinx files, so they don’t completely overlap, and I think it may make sense to have both.

@asmeurer
Copy link
Member

You need to add it to the Makefile like

diff --git a/doc/Makefile b/doc/Makefile
index 0e54ec0f76..590bba7b93 100644
--- a/doc/Makefile
+++ b/doc/Makefile
@@ -63,6 +63,9 @@ html: $(BUILDDIR)/logo/sympy-notailtext-favicon.ico
        @echo
        @echo "Build finished. The HTML pages are in $(BUILDDIR)/html."

+coverage:
+       $(SPHINXBUILD) -b coverage $(ALLSPHINXOPTS) $(SOURCEDIR) $(BUILDDIR)/html
+
 $(BUILDDIR)/html/pics/*.png: $(SOURCEDIR)/pics/*.png
        mkdir -p $(BUILDDIR)/html
        cp -r $(SOURCEDIR)/pics $(BUILDDIR)/html/

then run make coverage. It looks like it puts the output in _build/html/python.txt (with the above configuration), but if you add coverage_show_missing_items = True to conf.py it will also print it to the terminal. Might be a good idea to use coverage_skip_undoc_in_source = True as well to skip functions that don't have a docstring.

It looks like aside from those two options, it isn't very customizable, so we may end up needing to use coverage_doctest instead if it can't be suited to our needs.

It also doesn't appear to actually return nonzero when there are uncovered functions, so that would need to be fixed as well.

@s-hanko
Copy link
Contributor

s-hanko commented May 27, 2022

I think I have a fix for coverage_doctest finished.

If we're also going to add the sphinx coverage extension, I think it would be best to make a new issue to add it to the makefile, unless you think that should be here too?

@asmeurer
Copy link
Member

As far as issues go I think we can just keep this one. For the pull request, you can do it in the same pull request or separately, it doesn't matter. If it is complicated enough it makes sense to do it separately, but if it is simple it can go together in the same PR.

@s-hanko
Copy link
Contributor

s-hanko commented May 28, 2022

Alright, sounds good! For now I only have the fix for coverage_doctest and I haven't been able to get the extension to return nonzero so I’ll submit a pull request for coverage_doctest first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Testing Related to the test runner. Do not use for test failures unless it relates to the test runner itself
Projects
None yet
Development

No branches or pull requests

4 participants