TST/DEP: drop pytest-astropy in favor of corresponding, direct test dependencies#18784
TST/DEP: drop pytest-astropy in favor of corresponding, direct test dependencies#18784neutrinoceros wants to merge 1 commit intoastropy:mainfrom
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 |
|
Interesting. Looks like the |
|
of course, it's from |
|
Haha yes... It also has |
|
So yeah I am not sure if this is feasible without refactoring pytest-astropy itself... Maybe open discussion on astropy-dev if you still want to pursue? |
|
I do want to continue experimenting with this, but I don't think the problems I'm after can be addressed from |
|
(and with the impeding feature freeze, I'll have to postpone this work until at least next week) |
2e23c32 to
ca53c65
Compare
|
Turns out vendoring the contents of that plugin within our root |
ca53c65 to
3924e50
Compare
|
I'm seeing a couple unrelated failures caused by #18880 |
|
other failures I'm seeing are already handled in existing PRs |
|
The challenge isn't code, but people. We wanted the plugin stuff in pytest-astropy so other packages could also use it without requiring astropy, but they were so trivial that we did not want to create a whole new package just for them. Now, they been upstream long enough that we probably have a number of packages using them, and have to keep maintaining them. So copying them over is just duplicating code. This definitely need more discussion. |
|
we could easily make these markers into their own plugin if duplication must be avoided, but for what it's worth:
|
|
Moving them anywhere now would create churn. We have to find and inform all the affected downstream packages. And then go through deprecation period and face some resistance. And then still keep them going while deprecated. And then finally we have to remove them. |
why ? couldn't we just keep pytest-astropy as the "batteries included" option, even if we stop dog-fooding it in astropy itself ? I glanced over it and it didn't seem too burdensome to maintain (no release for 2 years and it's still working for everyone, as far as I can tell). Am I missing anything ? |
|
So you are saying we create a new package for the plugin and then include that in pytest-astropy, instead of moving it here? |
|
Not really. I would prefer that the code we use to test the core library live in the same repo, which is what the current state of the branch does, and leave pytest-astropy live its life as the provider of these functionality for everyone else that choose to adopt it. But if we must avoid code duplication at all cost, there are other avenues. Though, it seems we agree that they are highly disruptive and undesirable. |
664945e to
b6673f9
Compare
|
That said, it would make sense to isolate the plugin into its own package and make pytest-astropy (and maybe astropy itself) depend on it. It shouldn't have user-facing implications, would avoid duplication entirely, and would allow anyone who needs the plugin to drop other pytest plugins from pytest-astropy they don't need. |
|
As mentionned in #18882, I think my port of the |
|
Got it. What's broken is specifically runnings test with |
13390f7 to
9568d7b
Compare
|
I rebased this on top of #18890 to prevent further avoidable mistakes with marker registration, and I moved the plugin parts from pytest-astropy to our other, embedded |
|
Welp, that didn't work. Let's take this outside. astropy/pytest-astropy#73 |
|
Since the markers must be provided by a pytest plugin, let's try using a patched version of pytest-astropy (from my fork) that doesn't include any dependency besides pytest. |
b05a1e2 to
121b8b1
Compare
121b8b1 to
731e492
Compare
|
I'll remove the "extra CI" label for now. I agree it should run when this is stable enough, but it doesn't seem worth the extra CPU time at the current proof-of-concept stage. |
|
Can the markers be defined in |
neither will be discovered by pytest when running from an arbitrary location with |

Description
Based of #18780fix #18783