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

[MRG] MAINT pytest test discovery #9839

Closed
wants to merge 7 commits into from
Closed

Conversation

rth
Copy link
Member

@rth rth commented Sep 27, 2017

Makes sure that py.test . works work scikit-learn, fixes #9699 .

@jnothman
Copy link
Member

jnothman commented Sep 27, 2017 via email

@rth
Copy link
Member Author

rth commented Sep 27, 2017

My initial assumption was that pytest . should behave like pytest sklearn/ (i.e. make test-code) but in the end it would probably make more sense if the docs were tested as well. So I made some small changes to make pytest collect everything that it can without failing. Now it, therefore, behaves like

make test  # i.e. make test-code test-doc test-sphinxext

Should we use pytest . in CI just to be sure?

I don't know how this would fit in. For instance, the doc is currently only tested in Travis CI,

if [[ "$USE_PYTEST" == "true" ]]; then
pytest $(find doc -name '*.rst' | sort)
else
# Makefile is using nose
make test-doc
fi

I don't know with pytest migration you intend to eventually replace the testing via make with pytest (cf #9840)? For instance, with the changes in this PR it could be possible to just run,

pytest doc/

Regarding the testing of pytest . I think it applies more for testing the local install during development in a single command without using make. I don't think it's worth adding e.g. another travis build just to
test this, and I'm not sure it's a good idea to change the current separate doc/code tests with a single one.

What do you think? cc @lesteve

@rth rth changed the title [MRG] MAINT py.test discovery in the root directory [MRG] MAINT pytest test discovery Sep 27, 2017
@rth
Copy link
Member Author

rth commented Sep 27, 2017

The problem with pytest doc/ is that currently, in order to discover tests, it is also going to run any python script in doc/ that is not explicitly ignored, which is not optimal...

@lesteve
Copy link
Member

lesteve commented Oct 12, 2017

Maybe norecursedirs is what we need? For example see sphinx-gallery setup.cfg

@rth
Copy link
Member Author

rth commented Mar 8, 2018

Maybe norecursedirs is what we need?

In the end, I'm not so keen on norecursedirs, and I went with similified --ignore flags. It may be a bit more verbose but at least you are sure it doesn't have unexpected side effects and you know where the excluded folders are.

Should we use pytest . in CI just to be sure?

I'm not sure, this would also increase the run time somewhat. I could also affect the coverage btw (in a good way).

Overall I think this is an improvement, but note that currently this wouldn't work in CI anyway, because e.g. Travis CI uses pytest's --pyargs option and that's not very compatible atm with the --ignore option (pytest-dev/pytest#3287)

@lesteve
Copy link
Member

lesteve commented Mar 9, 2018

I agree it would be convenient to be able to just do pytest and run all the tests including doctests inside doc .rst files. Also pytest doc is a lot nicer than pytest $(find doc -name '*.rst').

The current problems I spotted:

  • you are not running doc/tutorial/text_analytics/working_with_text_data.rst at the moment. I looked at the diff of pytest doc --collect-only | sort vs pytest $(find doc -name '*.rst') | sort.
  • it does not look like --ignore supports wildcards or regex so this is a bit of a pain to skip only python files inside doc, because you need to mention explicitly all of them. Maybe this could be done via pytest_ignore_collect in conftest.py? Have a look at https://docs.pytest.org/en/latest/_modules/_pytest/hookspec.html (edit: actually I forgot about that but sklearn/externals/conftest.py uses pytest_ignore_collect already so you could use something similar in doc/conftests.py)

@rth
Copy link
Member Author

rth commented Mar 9, 2018

Maybe this could be done via pytest_ignore_collect in conftest.py? [..] sklearn/externals/conftest.py uses pytest_ignore_collect already so you could use something similar in doc/conftests.py

Thanks for pointing that out! I agree that writing a conftest.py under doc/ might be the cleanest way to get this working.

@ogrisel
Copy link
Member

ogrisel commented Sep 5, 2018

Merged a more coarse-grained config from #12011.

@ogrisel ogrisel closed this Sep 5, 2018
@ogrisel
Copy link
Member

ogrisel commented Sep 5, 2018

If someone want to have pytest doc work by setting the right options in doc/conftest.py, I am not opposed to it, but that's a lot of work for little benefits IMHO :)

@rth rth deleted the pytest-ignore branch September 5, 2018 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

py.test test discovery in the root directory
4 participants