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

CI:Pin pytest version to 7.0.1 on Azure #15783

Merged
merged 2 commits into from Mar 15, 2022
Merged

Conversation

ilayn
Copy link
Member

@ilayn ilayn commented Mar 14, 2022

Reference issue

Closes #15780

What does this implement/fix?

It seems that the conftest.py is only taken into account in meson builds (I think). Hence I am testing whether adding the markers explicitly to pytest.ini file would help.

@ilayn ilayn added the CI Items related to the CI tools such as CircleCI, GitHub Actions or Azure label Mar 14, 2022
@ilayn ilayn requested a review from larsoner as a code owner March 14, 2022 20:48
@ilayn
Copy link
Member Author

ilayn commented Mar 14, 2022

OK seems like we have unearthed some tests that were failing before but weren't being run apparently. Or xslow is not recognized yet. My bad there is a tab character leaked from copy/pasting.

But at least the pytest mark issue is resolved.

@ilayn
Copy link
Member Author

ilayn commented Mar 14, 2022

Well, after digging in a bit more I found #10178 so apparently pytest.ini is a no go. But then the question is why conftest.py is not being picked up suddenly.

@ilayn
Copy link
Member Author

ilayn commented Mar 14, 2022

The problem is that the new pytest version is not taking conftest.py into account. But I can't understand how we invoke pytest due to the hoop-jumping in runtest.py hence I'm not sure exactly where the tests are run and with which config.

Copy link
Contributor

@tylerjereddy tylerjereddy left a comment

Choose a reason for hiding this comment

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

But then the question is why conftest.py is not being picked up suddenly.

It may be because the runtests.py stuff is messing around with PYTHONPATH but the new version of pytest fixes a bug:

pytest-dev/pytest#9645: Fixed regression where --import-mode=importlib used together with PYTHONPATH{.interpreted-text role="envvar"} or pythonpath{.interpreted-text role="confval"} would cause import errors in test suites.

If I do cd /tmp, then pytest --pyargs scipy, it works with the new pytest + the installed version of SciPy. This approach is noted in doc/source/dev/contributor/meson.rst.

For the immediate Azure CI problem, the only thing I don't know how to do with pytest directly is --mode=$(TEST_MODE), but then I'm confused about pytest.ini not being available in that situation since we probe an installed version of SciPy?

@tylerjereddy
Copy link
Contributor

In fact, I can reproduce the missing marks error locally with i.e.,:

pytest --pyargs scipy -c ~/github_projects/scipy/pytest.ini

Can run that just fine with the older pytest 7.0.1, but errors with 7.1.0. That said, I've experienced the exact bug that was fixed in the pytest release notes with another project, and the new pytest did fix the issue over there, so I'm still a bit suspicious of our PYTHONPATH stuff..

@ilayn
Copy link
Member Author

ilayn commented Mar 15, 2022

You might be right. I looked into that possibility but clearly you had a better grasp of it. There are more people piling up in pytest-dev/pytest#9767 and it seems like it is a combination of different things for now.

It seems to me that our runtests.py is way too intertwined to dissect this issue. I think this is mostly due to historical reasons since we had our own decorators and test gathering before we switched to pytest.

We might want to streamline the test invocation a bit.

@tupui
Copy link
Member

tupui commented Mar 15, 2022

We might want to streamline the test invocation a bit.

Agreed! If we do this, I suppose we should update dev.py instead.

What about pinning the version of pytest for now while we investigate, otherwise we cannot rely on half of our CI.

@rgommers
Copy link
Member

Pinning pytest for now seems like the right thing to do. runtests.py has worked fine for ages, and I don't think there's much wrong there. Other projects are seeing issues too, so likely this will be fixed in pytest.

@ilayn
Copy link
Member Author

ilayn commented Mar 15, 2022

There is definitely nothing wrong with it but it is not necessarily the right way to collect tests anymore given the pytest automation level. It is probably won't be the case that we will ever switch to a less-able testing framework anyways hence we can actually gather the test options directly with a clean pytest invocation.

I'll pin the version for now since there is a consensus. It is only affecting windows workflows apparently given in the pytest issue linked above. I'll only change it in the azure CI template

@ilayn ilayn changed the title TST:Register marks explicitly in pytest.ini CI:Pin pytest version to 7.0.1 Mar 15, 2022
@ilayn ilayn changed the title CI:Pin pytest version to 7.0.1 CI:Pin pytest version to 7.0.1 on Azure Mar 15, 2022
@ilayn ilayn requested a review from andyfaff as a code owner March 15, 2022 10:50
@rgommers
Copy link
Member

it is not necessarily the right way to collect tests anymore given the pytest automation level. It is probably won't be the case that we will ever switch to a less-able testing framework anyways hence we can actually gather the test options directly with a clean pytest invocation.

pytest is actually quite poor at collecting tests from an installed module rather than a local checkout. We're also going to have to completely remove support for testing in a non-virtual/conda env, because without runtests.py/dev.py and its PYTHONPATH manipulation that just won't work.

You're welcome to try of course, but I'm not convinced it's a healthy idea yet.

@ilayn
Copy link
Member Author

ilayn commented Mar 15, 2022

Yes some PYTHONPATH modification is necessary but can be done in a much cleaner way.

Interesting that , we have the complete opposite experience with it but maybe I am not seeing completely what you have in mind. I am not using any virtual envs and I can test any installed package. But if you mean the scipy.test() mechanism that might be true I never used that one but then again I don't know its use case.

@tupui
Copy link
Member

tupui commented Mar 15, 2022

pytest is actually quite poor at collecting tests from an installed module rather than a local checkout. We're also going to have to completely remove support for testing in a non-virtual/conda env, because without runtests.py/dev.py and its PYTHONPATH manipulation that just won't work.

Not sure I understand what you mean by "poor at collecting tests".

I am actually never using runtests because most of the time I want to use the debugger of PyCharm. Aside from the not inplace build gymnastic (need to run the tests file which is in the build directory), everything is working just fine. And before this, there was just nothing to do to have full support from the IDE on the test side.

@ilayn
Copy link
Member Author

ilayn commented Mar 15, 2022

So apparently we are still using both azure templates in some jobs, I'll fix both.

Copy link
Member

@tupui tupui left a comment

Choose a reason for hiding this comment

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

Ok, aside from the usual timeout it's now green so let's get this in and keep the issue open to follow up. Thanks @ilayn

@tupui tupui merged commit b9c208b into scipy:main Mar 15, 2022
@ilayn ilayn deleted the pytest_mark_register branch March 15, 2022 13:59
@rgommers
Copy link
Member

Not sure I understand what you mean by "poor at collecting tests".

I mean the UX is bad. The equivalent of python dev.py -s spatial is:

pytest --pyargs scipy.spatial -m "not slow"

I had to look that up, because I can never remember. If pytest is not installed in the env that you built for but it's in your base env, it may also run the wrong test suite, so to guard against that the actual equivalent is:

python -m pytest --pyargs scipy.spatial -m "not slow"

You can save that as test command in an IDE, but it's far from ideal.

I am not using any virtual envs and I can test any installed package.

The point is that if you have for example packages from your package manager on Ubuntu, then there's nowhere good to install to. You may see a permissions error and be tempted to use sudo which is bad, or alternatively install to the --user location which is also bad as that's not isolated from other virtualenvs.

But if you mean the scipy.test() mechanism that might be true I never used that one but then again I don't know its use case.

That's indeed yet another thing you lose. It's quite handy in (for example) IPython if you want to check if there's an issue with your build. It works in notebooks too where you may not have a terminal at hand.

@ilayn
Copy link
Member Author

ilayn commented Mar 15, 2022

I think what @tupui and I am leaning towards is the collection of tests within runtests.py not the usage of runtests.py Using runtests.py is fine the CLI options are too but the code inside can have some ironing out.

I don't know about you but that code is seriously intertwined with names like test and tests as function names and a lot of difficult to follow things such that in the end pytest is invoked somewhere else and you don't know how it is connected or where they are. pytest has a very well established test gathering API in my opinion.

@rgommers
Copy link
Member

Isn't that just part of gh-15489? I agree it needs splitting up. The way scipy.test is called is just one of a bunch of problems with all the if-else's, and I don't think it is the cause of this pytest 7.1.0 regression.

@tupui
Copy link
Member

tupui commented Mar 15, 2022

Yes it is. @ilayn just noted here that it was difficult to debug due to the complexity of runtests. And we started to talk about this. It took me some time to find our custom pytest class too.

@ilayn
Copy link
Member Author

ilayn commented Mar 15, 2022

Isn't that just part of #15489?

Ah yes. That's it.

@tylerjereddy tylerjereddy added this to the 1.9.0 milestone Mar 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Items related to the CI tools such as CircleCI, GitHub Actions or Azure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: CI on Azure broken with PyTest 7.1
4 participants