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

TST: Register markers in conftest.py. #10178

Merged
merged 1 commit into from May 14, 2019

Conversation

charris
Copy link
Member

@charris charris commented May 13, 2019

Register the markers 'slow' and 'xslow' in the conftest.py file
instead of pytest.ini as the latter file is not always present.

Note that pytest >= 4.5.0 raises a warning if it finds unregistered markers.

Register the markers 'slow' and 'xslow' in the `conftest.py` file
instead of `pytest.ini` as the latter file is not always present.
@charris charris added the Build issues Issues with building from source, including different choices of architecture, compilers and OS label May 13, 2019
@larsoner
Copy link
Member

Register the markers 'slow' and 'xslow' in the conftest.py file
instead of pytest.ini as the latter file is not always present.

When is it not present?

It seems like we'll need to port all config options, including warning skips/ignores, if this is the case.

@charris
Copy link
Member Author

charris commented May 13, 2019

pytest.ini not present in wheels and installs, which is OK if it is in the directory from whence the tests are run, which is why runtests.py works, but that isn't the case for most users.

@charris
Copy link
Member Author

charris commented May 13, 2019

It seems like we'll need to port all config options, including warning skips/ignores,

NumPy has the ignore options duplicated in the test runner, but it would probably be a good idea to move them also, but I don't know how to do that yet :)

@larsoner
Copy link
Member

I think charris#1 should do it, feel free to merge/cherry-pick/whatever if you agree.

@charris
Copy link
Member Author

charris commented May 14, 2019

@larsoner The absence of pytest.ini is used to indicate releases. For instance, raising errors on deprecation warnings is done for development, but not for releases. What made the markers different was that pytest now raises warnings for unregistered markers and we do not want that to happen in releases, hence the move. For the rest, I would prefer the active scipy developers to make the decision as to what to do.

@charris
Copy link
Member Author

charris commented May 14, 2019

@tylerjereddy This might be something to backport if it goes in, I'll leave that up to you.

@larsoner
Copy link
Member

The absence of pytest.ini is used to indicate releases...
For the rest, I would prefer the active scipy developers to make the decision as to what to do.

It would probably be better to use something seemingly more standard like the __version__ having .devN at the end or not, but I agree we should wait to use larsoner@8a7706e or something similar until this is sorted out.

+1 for merge and backport from me

@tylerjereddy tylerjereddy added the backport-candidate This fix should be ported by a maintainer to previous SciPy versions. label May 14, 2019
@tylerjereddy
Copy link
Contributor

Alright, I'll put this in based on core dev +1.

Have to admit I still find this stuff confusing, even after discussing with Chuck in person on Saturday.

I'll put a tentative backport label on so I don't forget about this when I (soon) start preparing the final SciPy 1.3.0 release.

@rgommers quick thought on risk / reward for backporting this with no rc3?

@tylerjereddy tylerjereddy merged commit 002043c into scipy:master May 14, 2019
@tylerjereddy
Copy link
Contributor

Thanks Chuck

@rgommers
Copy link
Member

It's just a single warning that is being silenced for now, so I'm not sure I would take the risk. Probably okay, but if you do backport then I'd do some very careful testing.

@tylerjereddy tylerjereddy removed the backport-candidate This fix should be ported by a maintainer to previous SciPy versions. label May 14, 2019
@rgommers
Copy link
Member

@charris @tylerjereddy did anyone open a pytest issue or discuss with the pytest devs? This error is popping up in other places too (just noticed it on the PyWavelets repo) and it's extremely annoying. Most projects will be using @slow I think. A revert would be great.

@charris
Copy link
Member Author

charris commented May 15, 2019

@rgommers I did open an issue, pytest-dev/pytest#5255, and they gave me this workaround. I then closed the issue. I figured they would get a bunch more complaints, you may want to comment on the issue or open a new one.

@charris
Copy link
Member Author

charris commented May 15, 2019

I think the easy universal fix would be for pytest to add slow to the builtin marks.

EDIT: But that wouldn't fix the problem for other unregistered marks.

@rgommers
Copy link
Member

Thanks Chuck. I'll comment on that issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build issues Issues with building from source, including different choices of architecture, compilers and OS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants