TST: use --strict-markers when running pytest with --pyargs#18890
Conversation
|
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
|
👋 Thank you for your draft pull request! Do you know that you can use |
|
This isn't critical yet, but I suggest backporting it any way. Any difference that this could make would be an error we want to know about. |
|
also note that |
|
I also needed to replace |
db76e36 to
89b3bbf
Compare
|
failures are unrelated and already handled in dedicated PRs. However, I would feel much more confortable undrafting this with green CI (or at least a single unrelated failure mode), so I'll wait for other fixes to get merged first. |
56cdbf2 to
c3e209d
Compare
|
On the other hand, we're having so many issues at the same time that I fear creating a deadlock instead of a queue, so let's undraft this now since none of the failures appear relevant. |
7055592 to
a5c6b90
Compare
| upload_to_anaconda: false | ||
| test_extras: test | ||
| test_command: pytest -p no:warnings --astropy-header -m "not hypothesis" -k "not test_data_out_of_range and not test_set_locale and not TestQuantityTyping" --pyargs astropy | ||
| test_command: pytest -Wdefault --astropy-header -m "not hypothesis" -k "not test_data_out_of_range and not test_set_locale and not TestQuantityTyping" --strict-markers --pyargs astropy |
There was a problem hiding this comment.
Is -Wdefault exactly the same as -p no:warnings?
There was a problem hiding this comment.
no. If it was, I wouldn't need to change the existing code.
-p no:warnings completely disables warning control from pytest, including making filterwarnings decorators noop (undefined). I assume the real intention is instead to override our settings, which notably include filterwarnings: error, and this is what-Wdefault does.
There was a problem hiding this comment.
But where is the "default" defined? I just want to make sure we won't get sudden failures in the jobs we thought we're ignoring warnings at future release time.
There was a problem hiding this comment.
It's actually a flag from the python interpreter, conveniently re-exposed in pytest. The definition is at https://docs.python.org/3/library/warnings.html#the-warnings-filter
There was a problem hiding this comment.
the default behavior is to let warnings be warnings instead of errors. They'll show up, but won't be blocking. In contrast, -p no:warnings hides them completely, which could be argued isn't desirable in CI
There was a problem hiding this comment.
Depends... when you look at build logs, you just want to know wheels were made and tested. For those cases, printing out warnings will just clutter the logs. But let's see what @astrofrog says.
There was a problem hiding this comment.
If we really want to ignore them, -Wignore would be the closest, strict mode friendly, equivalent. It's actually what I used originally before it occurred to me that warnings are valuable data...
There was a problem hiding this comment.
Well... in our case, they are all handled in normal settings, so when we pass alternate flag, it means we don't care. 🤷♀️
There was a problem hiding this comment.
it could still be valuable to see warnings that pop specifically in cibuildwheel. If any, these would likely be warnings from pytest itself, and it'd be helpful to see them to debug anything they would be warning about
There was a problem hiding this comment.
My assumption is that these logs are rarely consulted, but if I were to need them, I wouldn't want warnings to disappear (or have to remember that they are silenced, that alone could waste me hours of scratching my head in the long run)
astrofrog
left a comment
There was a problem hiding this comment.
Looks good but one small comment below
docs/io/fits/appendix/history.rst
Outdated
| line syntax: | ||
|
|
||
| python -Wignore yourscript.py | ||
| python -Wdefault yourscript.py |
There was a problem hiding this comment.
I think we should revert the changes to this file, it's a historical document and doesn't need to be up to date
There was a problem hiding this comment.
Oh, good catch. It wasn't intended at all.
a5c6b90 to
2fac1a4
Compare
…ytest with `--pyargs`
…ytest with `--pyargs`
…ytest with `--pyargs`
Description
extracted from #18784.
Enabling this option makes a tremendous difference in debugging the kind of problems I'm hitting in that PR.
In brief,
--pyargs astropyis used to test an installed package in place, so any test marker defined in the repo-levelconftest.pyis undefined. By default, pytest warns about such markers, but doesn't raise. The--strict-markersflag requires that all markers are defined.