astropy / pyvo

An Astropy affiliated package providing access to remote data and services of the Virtual Observatory (VO) using Python.
https://pyvo.readthedocs.io/en/latest
BSD 3-Clause "New" or "Revised" License
75 stars 52 forks source link

Speed up registry doctest #567

Closed msdemlei closed 3 months ago

msdemlei commented 3 months ago

This is using other constraints and services in order to transfer less data and perhaps have faster queries.

bsipocz commented 3 months ago

while checking this locally, I discovered some weird behaviour/bug of the tests, I'll open a separate issue for that as the behaviour is present on main, too.

msdemlei commented 3 months ago

On Fri, Jun 28, 2024 at 09:58:34AM -0700, Brigitta Sipőcz wrote:

@bsipocz approved this pull request.

Minor comment, otherwise looks good.

Thanks!

+ +.. doctest::

no need for this directive, code with >>> are picked up for doctesting

How bad is the spurious doctest::? I'm asking because to simplify the logic of my doctest profiler, https://blog.g-vo.org/watch-sphinx-doctests.html, I'm only picking up python lines from RST blocks with doctest directives.

I can loosen this up, but if the occasional explicit but unnecessary directive doesn't bother anyone badly, I'd like it better if were were explicit.

[And yes, it would be preferable if I used whatever logic sphinx uses to pick up the tests for the profiler, but I figured I'd spend a lot more time on locating that than I wanted to spend on this tooling-for-tooling thing].

bsipocz commented 3 months ago

We don't use sphinx for picking up doctests and its plumbing is somewhat different than the pytest ecosystem and plugins we use (e.g. some sphinx doctest directives are known to not work with pytest/pytest-doctestplus and similarly, some doctestplus things that we rely on are known not working with sphinx.). So, while most things may work without issues, I wouldn't be surprised if you see doctest failures in CI even if they pass with the sphinx system locally.

So, having this one .. doctest:: is simply superfluous but doesn't break anything, so can stay, but you will need to keep using >>> for the code snippets otherwise they won't be picked up by the system we use, even though sphinx doctest may be happy with just using the doctest directive.