TST: Do not use astropy.test() in double run#17732
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 |
|
My tox-fu is weak. Is this equivalent as running |
|
We have to be sure the tests are run twice without invoking |
|
OK, found it: it is in #16208, you seem to need, |
|
Yes it's important the tests are run twice in the same interpreter |
Co-authored-by: Mridul Seth <mail@mriduls.com>
4a896dc to
dc8519a
Compare
|
Oh, thanks! I forgot about that PR. I copied Mridul's incantation over and credited him as a co-author. |
|
I think it works? But hard to tell because there is only one log of the run instead of two logs as before, but the number of tests look like they doubled.
|
|
I don't see |
| cov-!double: pytest --pyargs astropy {toxinidir}/docs {env:MPLFLAGS} --cov astropy --cov-config={toxinidir}/pyproject.toml --cov-report xml:{toxinidir}/coverage.xml {posargs} | ||
|
|
||
| double: python -c 'import sys; from astropy import test; test(); sys.exit(test())' | ||
| double: pytest --keep-duplicates --pyargs astropy astropy |
There was a problem hiding this comment.
Do I have to do something like this?
| double: pytest --keep-duplicates --pyargs astropy astropy | |
| double: pytest --keep-duplicates --pyargs astropy astropy {toxinidir}/docs {toxinidir}/docs {env:MPLFLAGS} {posargs} |
There was a problem hiding this comment.
Yes, I think so, and probably do astropy {doxinidir}/docs astropy {toxinidir}/docs
There was a problem hiding this comment.
I think we can skip {env:MPLFLAGS} since, if I understand correctly, it'll always be empty in this context.
Also: the current implementation for double tests doesn't include doctests for docs/. We could add it later but that seems out of scope here.
Otherwise: neat ! I had no idea --keep-duplicates existed or that --pyargs
- could be used with more than one package name
- still falls back to interpreting arguments as fs-paths when they don't match an installed package name
There was a problem hiding this comment.
astropy.test() not able to collect docs was a bug, not a feature. So if we can address that here also, would be nice.
There was a problem hiding this comment.
As for the other inputs, I just want to be consistent with single run commands.
There was a problem hiding this comment.
Actually I think we need posargs...
astropy/.github/workflows/ci_workflows.yml
Lines 98 to 101 in 93b1abc
Anyways trying it out at #17743
If you look at the log, you see it has the normal ordering and then, after |
There was a problem hiding this comment.
This seems like the correct, simple solution. Very cool !
The one critical (and theoretically hard to check) question to answer for this to work should be "does it fail where the old implementation did ?".
... I'm happy to report that double test is apparently broken on my machine (yay !) so I was able to check that I get the exact same tests to fail with both invocations1. I'm now pretty confident they're indeed equivalent.
I'll approve as is as I don't think suggested additions are needed !
Thank you !
Footnotes
-
Naturally, I intend to report (and possibly fix) them, though I'm short on time now so it'll have to wait for Monday. ↩
| cov-!double: pytest --pyargs astropy {toxinidir}/docs {env:MPLFLAGS} --cov astropy --cov-config={toxinidir}/pyproject.toml --cov-report xml:{toxinidir}/coverage.xml {posargs} | ||
|
|
||
| double: python -c 'import sys; from astropy import test; test(); sys.exit(test())' | ||
| double: pytest --keep-duplicates --pyargs astropy astropy |
There was a problem hiding this comment.
I think we can skip {env:MPLFLAGS} since, if I understand correctly, it'll always be empty in this context.
Also: the current implementation for double tests doesn't include doctests for docs/. We could add it later but that seems out of scope here.
Otherwise: neat ! I had no idea --keep-duplicates existed or that --pyargs
- could be used with more than one package name
- still falls back to interpreting arguments as fs-paths when they don't match an installed package name
|
Well, since Clement did prove that the change as-is is exactly equivalent replacement, maybe I can merge this first as-is. And then open follow-up PR to expand it. Thanks, all! |
…732-on-v7.0.x Backport PR #17732 on branch v7.0.x (TST: Do not use astropy.test() in double run)
Description
Had some problem with warnings being in consistent in #17570 . And given #16177 , let's see if this works.
Update: Copied incantation from #16208 but without the controversial stuff.
Blocks