MAINT Replace setup_module by pytest fixtures#28475
MAINT Replace setup_module by pytest fixtures#28475thomasjpfan merged 5 commits intoscikit-learn:mainfrom
Conversation
|
This looks uncontroversial to me. One thing to think about is https://docs.pytest.org/en/6.2.x/tmpdir.html instead of using |
|
Yeah I agree, that this was probably some old code that was inherited from the nose days and not touched in a while, I will look into using Pytest tmp_path_factory fixture and see if that simplifies the code even further. |
| data_dir = tmp_path_factory.mktemp("scikit_learn_empty_test") | ||
|
|
||
| global SCIKIT_LEARN_DATA, SCIKIT_LEARN_EMPTY_DATA, LFW_HOME | ||
| yield data_dir |
There was a problem hiding this comment.
I think you can now use return
There was a problem hiding this comment.
I can but, I'd rather always use yield so I don't have to think "when should I use yield, when should I use return" ...
FWIW Pytest doc also recommends using yield fixtures: https://docs.pytest.org/en/7.1.x/how-to/fixtures.html#yield-fixtures-recommended
|
@betatim are you happy with the latest changes ? |
Context #28461.
Using pytest fixtures avoids the Pytest 8.0.1 bug pytest-dev/pytest#12011 and is slightly more readable by not having to use module globals.
This is also recommended by Pytest in https://docs.pytest.org/en/stable/xunit_setup.html