ENH: Add tester for pytest. #10827

charris · 2018-03-30T00:59:34Z

This PR does two things

Removes 'bench' from the testing packages __init__ files, it isn't used anymore
Updates the pytest.ini file.
Adds pytesttester.py that provides PytestTester for pytest testing.

Actual testing with pytest will wait on another PR, but the new pytesttester has been tested to work as well as can be expected without the full suite of changes to come.

This PR is for getting the code under review. It is possible to combine it with the final switch to pytest if desired, but this is an opportunity to examine the addition in isolation.

eric-wieser · 2018-03-30T01:13:31Z

numpy/testing/_private/pytesttester.py

I don't think this is reliable in the presence of .pth files.

Yeah, that whole function is ugly, and inherited from nosetester. When Scipy switched the code was gone and things are done like so:

from scipy._lib._testutils import PytestTester test = PytestTester(__name__) del PytestTester

Which is nicely explicit. Going that way is a possibility.

The main problem with doing it the scipy way is tracking the develop vs release switch. However, I think that can be internalized into the test runner.

I think one reason develop vs release wasn't internalized before was downstream packages were using it for testing. I think we should stop the practice of supporting downstream packages when we switch to pytest because it has caused back compatibility problems. We should be able to change numpy testing without worrying about downstream.

@njsmith @rgommers Thoughts?

Agreed that the new pytest based test command should be based on a function/class that's not publicly exposed; it's easy for other packages to provide their own (see SciPy's _testutils.py).

Would indeed like to keep develop vs release behavior, that is useful.

Although, the whole filtering business is kind of tricky to get right.... let me think some more

My conclusion: I think we will need those warning filters in SciPy and other projects, however not having them there now is not super critical, and they can easily be copied over. In the end we need to be free here to change this test runner for NumPy's needs without having to worry about backwards compat, so I agree with Chuck that its best to keep this class private.

eric-wieser · 2018-03-30T01:15:41Z

numpy/testing/_private/pytesttester.py

Why the conversion here? Perhaps just check instance(package, str), and throw an error of some kind?

I think that may have been the intent.

eric-wieser · 2018-03-30T01:16:21Z

numpy/testing/_private/pytesttester.py

I'd be tempted to combine this with the else above

We can cure that problem if we decide to simply remove the function.

eric-wieser · 2018-03-30T01:16:59Z

numpy/testing/_private/pytesttester.py

Same remark about casting verbose - why not just require an integer to be passed, rather than allowing floats and strings?

I think the reason is to insure it is an integer and get an error if not.

If that's the intent, operator.index is a better way to spell that. I'd probably leave it without the cast though

eric-wieser · 2018-03-30T01:17:51Z

numpy/testing/_private/pytesttester.py

I'm curious why we need this at all

May not any more. IIRC, it was because in Python 3 not all objects could be compared.

I'm just thinking that we now test against py3, so our testcase would fall afoul of this if we did it.

I suppose we might be testing for warnings in py2-only tests.

That and the two above were added in 82a1d81. @juliantaylor Do you recall what that was about?

NVM, that was when they were fixed up.

Python 2.7 run with -3 issues a lot of deprecation warnings, and we do test with that flag. After we drop 2.7 things will be easier.

rgommers · 2018-03-30T02:24:21Z

numpy/testing/_private/pytesttester.py

Pytest not Nose here

charris · 2018-03-30T02:56:00Z

numpy/testing/_private/pytesttester.py

These can go, scipy won't be using this.

charris · 2018-03-30T02:56:55Z

numpy/testing/_private/pytesttester.py

These were mostly from Cython. IIRC, there has been some change on the Cython end, but I don't recall what it was.

The warnings are still there I believe: https://github.com/cython/cython/blob/2032632293ccda9d3700f10c85c6be101eeb53f6/Cython/Utility/ImportExport.c#L390

They haven't shown up in the last SciPy release, but I suspect that is because we haven't changed dtype and ufunc sizes in a while. Anyone know a handy way to check that?

pv · 2018-03-30T08:11:29Z

Re: warning filters --- in pytest, the way to do this is to put this stuff into pytest.ini

pv · 2018-03-30T08:18:24Z

Also on "develop" and "release" --- "develop" is signaled by the presence of our customized pytest.ini, and "release" by someone running without our pytest.ini present. This is something I though about when doing the Scipy conversion --- you can try to fight the tool and try to force it to do what you want, or do things its way. I think here "our way" of doing things is not that superior, so it's probably not worth the hassle. So I would just dump the "raise_warnings" stuff here. The `check_fpu_mode` above is also not necessary (it's always enabled in conftest.py).

charris · 2018-03-30T14:30:05Z

numpy/testing/_private/pytesttester.py

Anyone know how to pull this off in a pytest.ini file?

charris · 2018-03-30T14:45:00Z

@pv Do you know if the pytest warnings filters solve the Python problems that suppress_warnings is designed to solve? In fact, I'm concerned about a possible bad interaction between that and pytest as suppress_warnings fools with the python internals. It might be best to keep these filters and run pytest with addopts = -p no:warnings.

charris · 2018-03-30T14:57:57Z

Also on "develop" and "release" --- "develop" is signaled by the
presence of our customized pytest.ini, and "release" by someone running
without our pytest.ini present.

How does that work? Looks like SciPy includes pytest.ini in source releases, but not in wheels?

pv · 2018-03-30T15:07:56Z

As I understand, suppress_warnings is only needed if you want to ignore warnings in some tests but need to catch them in others. However, here the whole testsuite is contained in the same suppress_warnings context, and it's not clear any of these warnings is something that needs to be caught in any test. Regarding py3kwarning --- these do not appear important for numpy development, and they can be ignored unconditionally in the ini file. Note that the situation here is different from nosetester --- that's used also by 3rd party projects, but this class is specific to numpy. Yes, for scipy pytest.ini is only in the source directory. In particular, pytest does not look for configuration files inside the package (also, I was not able to figure out a non-horrible way to force it to).

pv · 2018-03-30T15:10:57Z

suppress_warnings has been in use in scipy, and I don't remember any problems with it and what pytest does.

charris · 2018-03-30T15:21:15Z

suppress_warnings was only needed when filtering with "ignore" locally, so globally might be OK. Although running tests in a session, and then continuing in the same session to do other things might be problematic. @seberg Thoughts?

pv · 2018-03-30T15:21:48Z

Another reason to avoid putting anything into the PytestTester class is that at least for me it was desirable to minimize the differences between `pytest --pyarg numpy` and `numpy.test()`. The pytest people seem to prefer the former for running tests.

charris · 2018-03-30T15:22:17Z

We do test in python 2.7 with -3. One option is to not do that.

pv · 2018-03-30T15:30:50Z

The other option for `-3` is to ignore those warnings unconditionally -- they are presumably not emitted when not running with -3, so ignoring them always does no harm.

charris · 2018-03-30T15:39:43Z

pytest --pyarg numpy and numpy.test(). The pytest people
seem to prefer the former for running tests.

Hmm, currently our ci tests work by running numpy.test(), I wonder if we shouldn't change that to simply do pytest ... --pyargs numpy. Do we even need to keep the *.test() methods? I can currently test installed numpy from the command line with pytest, although not from the numpy directory.

rgommers · 2018-03-30T17:21:29Z

Do we even need to keep the *.test() methods?

Yes definitely keep. There are many scenarios where this works better, e.g. on Windows most command lines will not have scripts on the path but do have a separate menu entry for starting an IPython Qt console or Jupyter notebook (default for Anaconda I believe). Also passing in non-default commands is easier by inspecting test() and having sane keywords (coverage=, doctests= etc.).

charris · 2018-03-30T20:03:36Z

@rgommers One of the reasons I am asking is that pytest.ini needs to be in the directory from which tests are run if we want to test as development. This is true of runtests.py -- which runs numpy installed elsewhere in testenv -- and also for the ci testing. Or so I understand at this point ;) I wonder if that also works for IPython and Jupyter notebook. Although I don't know if either is used for development. This is if we assume pytest.ini present means development and pytest.ini absent means release.

rgommers · 2018-03-30T23:06:40Z

I'm missing some context, why would we need a pytest.ini file at all? Nose doesn't need one, nor does SciPy with pytest. The dev vs release can be done based on version number, like it is now.

pv · 2018-03-30T23:55:28Z

For scipy, pytest.ini is used e.g. for turning warnings into errors --- the release vs. dev is not based on version number. It also avoids magic in scipy.test() --- the customization is done in the way the developers of pytest appear to intend it to be done. Moreover, I would like to have close equivalence between pytest --pyarg numpy and numpy.test().

rgommers · 2018-03-31T00:08:08Z

Hmm, somehow I had missed that file completely.

the release vs. dev is not based on version number.

It's not done at all it seems? Shouldn't the removal of filterwarnings = error be part of the release process now?

charris · 2018-03-31T00:28:03Z

Is there a 16 character length limit for messages in pytest.ini warnings filters?

    ignore:sys.exc_clear() :DeprecationWarning

works

    ignore:sys.exc_clear() n:DeprecationWarning

does not. Anyone know of a workaround?

charris · 2018-03-31T00:45:37Z

@rgommers If you look at the top of the output from runtests.py, you see the file

platform linux2 -- Python 2.7.14, pytest-3.1.2, py-1.4.34, pluggy-0.4.0
rootdir: /home/charris/Workspace/scipy.git, inifile: pytest.ini

pv · 2018-03-31T14:01:52Z

@rgommers: pytest does not look for the ini file inside the installed package (and it's not shipped in the first place). I think it works fine as it's now -- error on warning is enabled if you run the tests inside the source tree (or explicitly specify the config file).

@charris: the second field I think is a regexp, so do () need to be quoted?

The "bench" testing with the old bench files is no longer supported. These days we use `runtests.py` and `asv`.

Include relevant suppression of various warnings.

[ci skip] This is not used yet, but has been tested with temporary changes and works.

charris · 2018-03-31T17:23:12Z

Updated.

seberg · 2018-04-01T10:35:06Z

@charris, filtering globally is only fine if you never want to test that warning anywhere. Which is probably OK for things like those funny cython warnings, but not OK for anything that numpy warns itself.

Otherwise "once" filters/default is always problematic (if you may want to test the warning). "ignore" filters should be fine once we drop 2.7 probably (since I guess the older py3s are also gone then).

Maybe just to note suppress_warnings has the slight advantage of allowing nesting. catch_warnings (used for a simple ignore), will also catch all the other warnings that might pop up in the future (within the block) and they will be unnoticed.

seberg · 2018-04-01T10:43:37Z

Re: pytest + the context, I do not see how it can do real issues. It really is just a catch_warnings on steroids, and does not do more then catch_warnings when it comes to messing with python internals (except with the module filter, which we do not really use, and even that should be harmless).

I agree with @pv, as also said above, the most outer level of suppress_warnings is largely unnecessary. Though there are probably a few warnings we are almost always suppressing (except in an isolated test). The py3 warnings that numpy raises are for example probably tested somewhere. There was also the thing that the tester was being used by scipy, so it seemed best to try to not do unnecessary "ignore" filters.

charris · 2018-04-01T20:05:55Z

@seberg OK, I think we are good, with only ignore filters up top, and most of those will go away with Python 2.7. I don't recall exactly why we are testing the -3 option anyway.

charris · 2018-04-01T20:07:16Z

I'm going to put this in as it has no effect on current NumPy. Thanks all.

rgommers · 2018-04-01T20:26:24Z

pytest does not look for the ini file inside the installed package

Okay, I see how it works. Looks good, the only thing to keep in mind is that we shouldn't put warning filters in pytest.ini, because those filters then also won't be shipped. The only thing that should go in is the error and always lines (and the once: ... after to undo the always is fine too).

seberg · 2018-04-02T08:16:05Z

We actually have a deprecation warning on integer divisions with the -3 flag set. It is a deprecation warnig "numpy: classic int division". It is tested in the deprecation tests module, it seems some other tests are skipped/filtered for the same reason. (See also gh-8083)

So, it does not matter much anyway, the only thing that might make a bit sense is to say that "numpy: classical int division" should not be ignored, but the test will fail if it is, so...

charris added 01 - Enhancement component: numpy.testing labels Mar 30, 2018

charris added this to the 1.15.0 release milestone Mar 30, 2018

eric-wieser reviewed Mar 30, 2018

View reviewed changes

rgommers reviewed Mar 30, 2018

View reviewed changes

numpy/testing/_private/pytesttester.py Outdated

Copy link

Member

rgommers Mar 30, 2018

Choose a reason for hiding this comment

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

Pytest not Nose here

charris commented Mar 30, 2018

View reviewed changes

numpy/testing/_private/pytesttester.py Outdated

Copy link

Member Author

charris Mar 30, 2018

Choose a reason for hiding this comment

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

These can go, scipy won't be using this.

charris commented Mar 30, 2018

View reviewed changes

numpy/testing/_private/pytesttester.py Outdated

Copy link

Member Author

charris Mar 30, 2018

Choose a reason for hiding this comment

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

Anyone know how to pull this off in a pytest.ini file?

charris added 3 commits March 31, 2018 11:08

MAINT: Remove "bench" from testing modules __init__s.

6126789

The "bench" testing with the old bench files is no longer supported. These days we use `runtests.py` and `asv`.

MAINT: Update pytest.ini.

6d7e702

Include relevant suppression of various warnings.

ENH: Add tester for pytest.

fddaa7c

[ci skip] This is not used yet, but has been tested with temporary changes and works.

charris force-pushed the create-pytesttester branch from 59c2bd9 to fddaa7c Compare March 31, 2018 17:10

charris merged commit bbd72df into numpy:master Apr 1, 2018

charris deleted the create-pytesttester branch September 19, 2018 14:33

This was referenced May 30, 2020

switch old numpy nosetester to pytest call python-control/Slycot#136

Merged

remove unused patch; update test command and requirements conda-forge/slycot-feedstock#26

Merged

Uh oh!

ENH: Add tester for pytest. #10827

ENH: Add tester for pytest. #10827

Uh oh!

Conversation

charris commented Mar 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

charris Mar 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

charris Mar 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eric-wieser Mar 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

charris Mar 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pv commented Mar 30, 2018 via email

Uh oh!

pv commented Mar 30, 2018 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

charris commented Mar 30, 2018

Uh oh!

charris commented Mar 30, 2018

Uh oh!

pv commented Mar 30, 2018 via email

Uh oh!

pv commented Mar 30, 2018 via email

Uh oh!

charris commented Mar 30, 2018

Uh oh!

charris commented Mar 30, 2018 •

edited

Loading

charris Mar 30, 2018 •

edited

Loading

charris Mar 30, 2018 •

edited

Loading

eric-wieser Mar 30, 2018 •

edited

Loading

charris Mar 30, 2018 •

edited

Loading

pv commented Mar 31, 2018 •

edited

Loading

seberg commented Apr 1, 2018 •

edited

Loading