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

ENH: Test full output and coverage #230

Merged
merged 7 commits into from
Aug 9, 2019
Merged

ENH: Test full output and coverage #230

merged 7 commits into from
Aug 9, 2019

Conversation

larsoner
Copy link
Collaborator

@larsoner larsoner commented Aug 2, 2019

Over in Sphinx-gallery for about a year we've been testing full builds of sphinx using Pytest:

https://github.com/sphinx-gallery/sphinx-gallery/blob/master/sphinx_gallery/tests/test_full.py

It's convenient because:

  1. You can add assertions about the HTML that you get in pytest form.
  2. You can easily interactively test by doing make clean; make html from within the numpydoc/tests/tinybuild directory and looking at the rendered HTML.
  3. You can easily add more test cases by adding new things to be documented in the nd_test_mod module in tinybuild.

I've shown how to use this framework in some basic cases here. I've also added some assertions that are bad (marked by XXX) that #221 should fix (@thequackdaddy after this PR is merged and you rebase, you can change a couple of ins below to not ins to assert that the *args, **kwargs situation has been fixed).

This PR also attempts to enable codecov.

@larsoner
Copy link
Collaborator Author

larsoner commented Aug 2, 2019

Okay all green, ready for review/merge from my end. @thequackdaddy perhaps you could look?

@larsoner
Copy link
Collaborator Author

larsoner commented Aug 2, 2019

Looks like we are already at 93.4% coverage, not bad!

@larsoner
Copy link
Collaborator Author

larsoner commented Aug 7, 2019

@jnothman if you can take a look, it would be good to get in so that #221 can proceed.

@thequackdaddy
Copy link
Contributor

@larsoner Sorry for my non-response! Busy week.

I took a look and I think its a good start. I'll totally confess you are clearly more up-to-speed with some of this than I am. I wasn't thinking about adding in a mini-sphinx build to test against, so I think it looks good. That looks impressive and 93.4% coverage is excellent.

Obviously, the maintainers would be a better reviewer than me though.

Copy link
Member

@jnothman jnothman left a comment

Choose a reason for hiding this comment

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

I generally think this is an awesome improvement to the testing. Though I don't really like how the test fixtures, i.e. the numpydoc, is far from the assertions about it, and that at the same time the purpose of each test case is very mixed... I.e. the tests aren't very legible, which is not improved by the use of string matching over html.

I don't have any constructive suggestions at this point, and would be happy to see this merged.

from sphinx.util.docutils import docutils_namespace


# Test framework adapted from sphinx-gallery
Copy link
Member

Choose a reason for hiding this comment

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

No licensing requirements?

'sphinx.ext.intersphinx',
'numpydoc',
]
project = 'nd_test_mod'
Copy link
Member

Choose a reason for hiding this comment

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

It took me a while to work out what nd is. Maybe expand it?

@@ -0,0 +1,51 @@
"""Numpdoc test module.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"""Numpdoc test module.
"""Numpydoc test module.

assert 'stdtypes.html#dict' in html


def test_function(sphinx_app):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def test_function(sphinx_app):
def test_my_function(sphinx_app):

Otherwise this sounds too generic

return app


def test_class(sphinx_app):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def test_class(sphinx_app):
def test_MyClass(sphinx_app):

@larsoner
Copy link
Collaborator Author

larsoner commented Aug 8, 2019

Though I don't really like how the test fixtures, i.e. the numpydoc, is far from the assertions about it, and that at the same time the purpose of each test case is very mixed... I.e. the tests aren't very legible, which is not improved by the use of string matching over html.

Basically the idea is to test that the end output (HTML) is as it should be. I agree that this is not so great for inspecting where in numpydoc the failures arose, but it is good for ensuring that we don't introduce failures in the final output (what is correct). Ideally tests for new functionality are built at the fine-grained level as encapsulated as possible (like what we have at master) to help with the first case, and not just at the full-sphinx-build level. But sometimes this is difficult.

Another way to look at it is that at the end of the day we want numpydoc to give us correct HTML outputs, so it's reasonable to check that the HTML output looks correct.

@larsoner
Copy link
Collaborator Author

larsoner commented Aug 8, 2019

Pushed a commit with the suggested changes, plus a tiny bit more explanatory text comments for the HTML assertions

@larsoner
Copy link
Collaborator Author

larsoner commented Aug 9, 2019

Since comments were addressed, this does not change any functionality (just expands our testing capabilities a bit and enables codecov), and @jnothman was +1 for merge I'll go ahead and put this in. Hopefully it helps #221!

@larsoner larsoner merged commit aa290a1 into numpy:master Aug 9, 2019
@larsoner larsoner deleted the full branch August 9, 2019 13:00
@jnothman
Copy link
Member

jnothman commented Aug 11, 2019 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants