Switching to pytest for unit testing framework; Use doctest more extensively

[jthielen]

Would there be support for moving to pytest as the unit testing framework for Pint over unittest? I think this would be a worthwhile move for a couple reasons:

• pytest's unittest.TestCase support would make it an easy transition
• Parameterized tests in pytest's style would be very useful for some current/upcoming PRs (see WIP: floordiv, mod, divmod #943 #946 (comment), Add tests to ensure internal type is not mutated except in inplace operations #925, Insuring interoperability of pint with other (non-unit) array-like types #845 (comment))
• There's a plugin for matplotlib tests, which would aid Implement tests for matplotlib functionality #897
• It's widely used elsewhere (e.g., NumPy, Dask, xarray, Sparse, MetPy)

@hgrecco What are your thoughts on this?

[hgrecco]

I am +1 on this.

[hgrecco]

I would also like to migrate to Numpy docs and use doctest more extensively to make sure that the documentation is always up to date.

[jthielen]

In #954, I'm working on switching Travis CI to use pytest instead of unittest. However, I think this issue should remain open for the reminder on migrating to NumPy docs and using doctest more extensively?

[hgrecco]

However, I think this issue should remain open for the reminder on migrating to NumPy docs and using doctest more extensively?

I agree

[hgrecco]

I am trying pyment to migrate the docs

[hgrecco]

While this is partly complete, I am phasing this to 0.11 because I think we should make a more in depth change before closing this issue.

[clarkgwillison]

@hgrecco What's the recommended way of running the doctests? (Or even the whole test suite in general?)

If I run naked pytest on the current master (cda2a40) I get 1808 tests collected and no failures, but if I run

$ python -c 'from pint.testsuite import test_docs; test_docs()'

I get 18 tests and 11 failures, and the output looks like legitimate (albeit mostly minor) failures, which makes me wonder if doctests are being skipped in CI.

None of the following invocations replicates that behavior, so I'm at a bit of a loss:

$ pytest pint/testsuite/__init__.py::test_docs     # ERROR: not found
$ pytest pint/testsuite/__init__.py                # doesn't seem to collect doctests
$ pytest -k 'test_docs'                            # collects 1808, deselects all 1808

(Happy to submit a PR updating developer docs with the recommended local environment setup and test suite invocations)

[keewis]

I would use

python -m pytest --doctest-modules --ignore=pint/testsuite

which I believe will run the doctests it can find (remove the --ignore if you want to run the testsuite, too). To get it to work, the inconsistent whitespace in pint.registry.ContextRegistry.with_context has to be fixed. After that, I only get 4 (functions with) doctests...

[clarkgwillison]

Hmm, that works to run the tests in the module docstrings, but not for any of the .. doctest:: callouts in the Sphinx documentation. Poking around, it looks like the latter is best run with:

$ cd docs
$ make doctest

Which, as of f8ec2ca is giving me:

[.... lots of useful output]

Doctest summary
===============
  314 tests
   92 failures in tests
    0 failures in setup code
    0 failures in cleanup code
build finished with problems, 3 warnings.
make: *** [doctest] Error 1

Which are more believable numbers given there's 209 blocks marked with .. doctest:: in the docs.

If that is indeed the correct way of running the Sphinx doctests, it looks like we need to:

• clean up those 92 failures
• update the developer reference to help people run them locally, and
• add $ make doctest to the Travis configuration

[hgrecco]

It would also be good to see double check that all the docstrings are writen using the Numpy style, and the API docs are nicely generated.

[clarkgwillison]

It looks like the package flake8-docstrings might help there, though we get a very large number of errors. I ran it just now and got the following:

$ pip install flake8-docstrings
$ flake8 --docstring-convention numpy | egrep -o "\W([D]\d{3}.*$)" | sort | uniq -c | sort -r
 716  D102 Missing docstring in public method
 123  D105 Missing docstring in magic method
  92  D400 First line should end with a period
  72  D101 Missing docstring in public class
  65  D103 Missing docstring in public function
  62  D205 1 blank line required between summary line and description
  52  D414 Section has no content
  45  D202 No blank lines allowed after function docstring
  30  D200 One-line docstring should fit on one line with quotes
  25  D100 Missing docstring in public module
  24  D401 First line should be in imperative mood
  17  D208 Docstring is over-indented
   9  D401 First line should be in imperative mood; try rephrasing
   4  D403 First word of the first line should be properly capitalized
   3  D210 No whitespaces allowed surrounding docstring text
   2  D404 First word of the docstring should not be `This`
   2  D300 Use """triple double quotes"""
   1  D412 No blank lines allowed between a section header and its content
   1  D209 Multi-line docstring closing quotes should be on a separate line
   1  D104 Missing docstring in public package

Perhaps if we suppressed D102 and D105 the rest would be manageable?
Related: #972

[hgrecco]

@clarkgwillison I was not aware of this tool. Thanks for the pointing this out.

I was surprised by the result because I knew that most of the code is documented. Then I realized that the test folder was counted. Ignoring the testsuite folder provides a more accurate result:

flake8 --docstring-convention numpy --exclude testsuite | egrep -o "\W([D]\d{3}.*$)" | sort | uniq -c | sort -r
 123  D105 Missing docstring in magic method
  80  D400 First line should end with a period
  78  D102 Missing docstring in public method
  59  D205 1 blank line required between summary line and description
  45  D202 No blank lines allowed after function docstring
  28  D414 Section has no content
  28  D200 One-line docstring should fit on one line with quotes
  27  D103 Missing docstring in public function
  21  D401 First line should be in imperative mood
  17  D208 Docstring is over-indented
   9  D401 First line should be in imperative mood; try rephrasing
   8  D101 Missing docstring in public class
   5  D100 Missing docstring in public module
   4  D403 First word of the first line should be properly capitalized
   3  D210 No whitespaces allowed surrounding docstring text
   2  D404 First word of the docstring should not be `This`
   2  D300 Use """triple double quotes"""
   1  D412 No blank lines allowed between a section header and its content

Having said this, I would agree to ignore D105.

[hgrecco]

or better:
flake8 --docstring-convention numpy --exclude testsuite,bench,docs

[clarkgwillison]

Nice, that does make the numbers look a lot better.

Given the amount of work to get to zero here, it might make sense to go for a longer term goal (like getting to coverage 100%)

Is there a way to implement "check the # errors didn't increase" rather than "check that # errors is zero" in Travis?

[hgrecco]

I don't think that is possible. My suggestion is that you add this test and label it as expected failure. Having that reporting this would be enough for now.

[cpascual]

Is there a way to implement "check the # errors didn't increase" rather than "check that # errors is zero" in Travis?

Maybe it can be done with a bit of customization, by telling flake8 to check only the diffs. We do something like that (for flake8, not flake8-docstings) here

[hgrecco]

I have just pushed a new branch _pytest_migration in which unittest is replaces by pytest. While there is still room for improvement, all tests are passing, and it is 25% faster.

If there are now complains, I will rebase it on top of master and merge it on Jan 17.

Just to summarize what can be improved:

• This way of combining marks does not work:

  pint/pint/testsuite/helpers.py

  Line 169 in 659f630

  def multi_mark(*funcs):

• pytest-subtests has been used a lot but parametrize should be better in multiple cases.
• Properly scoped fixtures should be used to speed up testing
• Some classes could be removed and use plain functions.