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

MAINT: remove the usage of the astropy TestRunner #606

Merged
merged 4 commits into from
Oct 2, 2024

Conversation

bsipocz
Copy link
Member

@bsipocz bsipocz commented Oct 1, 2024

This is to address #604 as well as the upstream plans of actually deprecating the TestRunner.

We haven't really advertised the pyvo.test() functionality anywhere, so I would be OK with including this in a bugfix release in the coming few weeks, if that is something you would find helpful @ManonMarchand

Copy link
Member

@pllim pllim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Nice to not have to keep a duplicate conftest.py any longer. Thanks!

Copy link

codecov bot commented Oct 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.70%. Comparing base (cc17425) to head (029980e).
Report is 29 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #606      +/-   ##
==========================================
+ Coverage   81.68%   81.70%   +0.01%     
==========================================
  Files          69       69              
  Lines        7089     7093       +4     
==========================================
+ Hits         5791     5795       +4     
  Misses       1298     1298              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bsipocz
Copy link
Member Author

bsipocz commented Oct 1, 2024

Nice to not have to keep a duplicate conftest.py any longer

Yeap, those got me confused several times, and there are still some inconsistencies. E.g Larry kept the one in the photutils directory, though the one there seems to have the comments that it was required by the TestRunner and not by pytest.

Anyway, this layout seems to work both locally with pytest -P as well as in tox, so I would just keep it.

@pllim
Copy link
Member

pllim commented Oct 1, 2024

Not sure if "online tests" job failures are related.

Also, now you don't need the test runner, you can even move all the tests out of package dir if you want.

@bsipocz
Copy link
Member Author

bsipocz commented Oct 1, 2024

Test failures are unrelated, something is off at CADC atm.

Also, now you don't need the test runner, you can even move all the tests out of package dir if you want.

No, I really don't want to move them out and really don't want to move to a src/ directory structure, I think there is value keeping tests close to the code. I would only consider changing this if @ManonMarchand browser work explicitly require it.

@ManonMarchand
Copy link
Member

ManonMarchand commented Oct 2, 2024

It works on my side, and if you think that this won't affect the users, getting it in a bugfix release would be welcome

PS: these browser-running python instances are awesome for workshops, maybe I can show you at the IVOA if you're interested? We already have a cds jupyterlite instance with working astroquery examples here (see notebook 1, even if it needs way more text). It'd be nice to also be able to showcase pyvo

PPS: thank you so much for the quick PR !!

@ManonMarchand ManonMarchand linked an issue Oct 2, 2024 that may be closed by this pull request
@ManonMarchand ManonMarchand linked an issue Oct 2, 2024 that may be closed by this pull request
@bsipocz
Copy link
Member Author

bsipocz commented Oct 2, 2024

Yes, I don't expect any user level issues as we haven't advertized it, and anyway, that usage is underused in the wider astropy ecosystem, too.

PS: I'm very much interested in your browser experience! I'm familiar with the interactive documentation approaches that are happening in the wider scientific python ecosystem, and we are thinking about interactive tutorials, too, but none of those got to the point of applicability on the astroquery/pyvo/navo levels.

@bsipocz bsipocz modified the milestones: v1.6, v1.5.3 Oct 2, 2024
@bsipocz bsipocz merged commit 130d08d into astropy:main Oct 2, 2024
12 of 13 checks passed
@bsipocz bsipocz deleted the MAINT_remove_testrunner branch October 2, 2024 19:02
bsipocz added a commit that referenced this pull request Oct 14, 2024
MAINT: remove the usage of the astropy TestRunner
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

astropy test dependencies are mandatory for pyvo MAINT: cleanup identical conftest files
3 participants