TST: migrate from nosetest to pytest #238
Conversation
neutrinoceros
force-pushed
the
drop_nose
branch
3 times, most recently
from
7e731ba to
51339ba
Compare
yt_astro_analysis/radmc3d_export/tests/test_radmc3d_exporter.py
Outdated
Show resolved
Hide resolved
neutrinoceros
force-pushed
the
drop_nose
branch
from
51339ba to
ba384ae
Compare
brittonsmith
left a comment
brittonsmith
left a comment
There was a problem hiding this comment.
This PR has some fairly significant changes. It would be useful to have them outlined in the description so that I don't have to suss them out from the diff. As far as I can see, you are removing entirely the ability to save answers and test against them. I recognize that we were testing against the same changeset, but after this we will not be able to return to maintaining a gold standard of results. Adding answers inline is also going to limit the answers we are able to test against. Some discussion of why this needs to happen would be good, but minimally, this deserves a more detailed record of what's being changed.
| return yt.load(path, *args, **kwargs) | ||
|
|
||
|
|
||
| def requires_sim(sim_fn, sim_type, file_check=False): |
There was a problem hiding this comment.
If this is being deprecated, what functionality is replacing it? Same for can_run_sim
There was a problem hiding this comment.
None. The idea is that if anyone is using those functions downstream, they should be able to vendor them because they're very simple.
|
I updated the PR description. You are right that, I'm effectively diminishing the flexibility of what tests can be implemented because I am not replacing the answer test framework by anything equivalent. To be clear, replacing this framework is a non-goal (to me). I'm just observing that
My personal opinion is that it justifies dropping nose as a dependency, which would simplify maintenance. Porting the framework to pytest is a substantial undertaking that I think should happen upstream (in yt itself), and that I have no desire to champion. This by no means implies that I want to impose this proposal as the de-facto way forward, but I want to make it clear what my personal limits are early in the discussion :-) |
|
@brittonsmith I should disclose that I do not see myself returning to this in the foreseeable future. I understand your objections but don't have the bandwidth to address them. Migrating from nose to pytest is a long-due necessity (here as well as in yt), but maybe this isn't the right approach for the project. Anyway, I will keep this open for another month or two in case anyone wants to continue it, and if nothing happens I'll just close it. Thanks for understanding. |
|
@neutrinoceros sorry for being silent on this for so long. I've just been overwhelmed with teaching since last fall. I've discussed this with @cphyc and we're willing to take over responsibility for it. What we're probably going to do is simply write new tests for this package to use pytest straight away rather than trying to port what we have now. We're going to punt on this until the summer, but have committed to returning to it then. Let's leave this open for now and we will take over. |
for more information, see https://pre-commit.ci
|
@brittonsmith I'm very surprised you pinned setuptools as a runtime dependency. Did you mean to edit the build-system section instead ? What problem are you trying to solve ? |
pyproject.toml
Outdated
| "yt>=4.0.1", | ||
| "numpy>=1.19.3, <3", | ||
| "packaging>=20.9", | ||
| "setuptools>=61.2, <81" |
There was a problem hiding this comment.
this should not be needed; either it's a mistake, or there's a deeper problem that we're hiding with it, and I'd like to know which one.
pyproject.toml
Outdated
| [build-system] | ||
| requires = [ | ||
| "setuptools>=61.2", | ||
| "setuptools>=61.2, <81", |
There was a problem hiding this comment.
please document why the upper bound is needed. This seems fishy but I'd like to know why this was needed
|
I strongly suspect, from the commit description, that the root issue is with girder-client, not yt_astro_analysis itself, so I updated the PR again and proposed a patch upstream: girder/girder#3717 |
neutrinoceros
force-pushed
the
drop_nose
branch
from
9055b44 to
29d21e8
Compare
…on and document it
|
Update: turns out my patch isn't the solution to this problem, but the correct solution was already made a couple months ago. |
cphyc
left a comment
cphyc
left a comment
There was a problem hiding this comment.
The version-fiddling looks like black magic to me, but if it works, I'm happy.
3 participants
this is a subset of #229, focusing on migrating out of nosetest + yt's answer testing framework by rewriting tests in a simpler fashion with pytest
This PR removes nosetest as a requirement for tests. Namely, answer tests are converted to simple pytest functions (reference results are inlined). As a result, imports from
yt.testing.answer_testing.frameworkare also dropped.