Test docs

[tlambert03]

While going through the docs I found a few code blocks that seem to be outdated. This PR adds (just one idea for) a way to test that PRs don't break the docs, or at least that you get notified.

It's challenging, of course, since the docs repo is independent of this one, so it's hard to fix any failures here within the same PR... so that's a topic to discuss. This also, for now, clones the docs repo into the current directory, but that could be made a temp path if you prefer. But, this is just an idea

Issues this reveals:

• in get_started.md, chunked.md, and multiscale.md: AttributeError: 'builtins.StorageProperties' object has no attribute 'chunking'
• in compressed.md - KeyError: '0' when reading back the zarr data
• in configure.md - AttributeError: type object 'builtins.SampleType' has no attribute 'U32'
• in framedata.md - AttributeError: 'builtins.AvailableDataContext' object has no attribute 'frames'

[aliddell]

I like this idea, but I don't like having the docs repo as a submodule. I also think it makes more sense to test the docs in the docs repo, no? This could be a workflow in acquire-docs where you check out the latest acquire-python release (or better yet, install acquire from pip) and test the docs there.

[tlambert03]

but I don't like having the docs repo as a submodule

yep, it's not necessary though, and we could absolutely guard the clone command in the tests with if os.getenv('CI') ... I definitely agree it shouldn't be added as an actual submodule here.

I also think it makes more sense to test the docs in the docs repo, no?

that's fine too, as long as you have some mechanism to trigger those tests from here as well, since you're more likely to be breaking the tests from this repo than you are from that one (i.e. that one can go stale, as it has, and with the attention paid here, it might be useful to know ahead of time if a PR here mandates an update to the docs)

[tlambert03]

we could absolutely guard the clone command in the tests with if os.getenv('CI')

added here, so this won't affect anyone running locally

[aliddell]

I think this is a great addition, but I have a couple suggestions.

Sorry it took me so long to get back to this.

[aliddell]

I don't know if this is necessary. Nobody will be able to run pytest without specifying which tests to run, since we have a few tests that depend on hardware. But if we do go with this, we'd need to pair it with setting the appropriate environment variable in test_pr.yml or it won't run.

[aliddell]

Could make this a function, e.g.,

def docs_repo():
    skip = {
        "setup.md",  # has invalid syntax
        "trigger.md",  # has some non-existant paths
    }
    docs_repo = "https://github.com/acquire-project/acquire-docs"

    if not (docs_path := Path("acquire-docs", "docs")).exists():
        subprocess.check_call(["git", "clone", docs_repo])

    tuts = [docs_path / "get_started.md"]
    tuts.extend([fn for fn in docs_path.glob("tutorials/*.md") if fn.name not in skip])
    tuts.sort()

    return tuts

and call it in the decorator for test_tutorials(). I tried it out and it seems to work. Here's what my whole file looks like:

from pathlib import Path
import subprocess
import re
import logging
import pytest

logging.getLogger("acquire").setLevel(logging.CRITICAL)

code_block = re.compile(r"```python\n(.*?)```", re.DOTALL)


def docs_repo():
    print("calling docs_repo()")
    skip = {
        "setup.md",  # has invalid syntax
        "trigger.md",  # has some non-existant paths
    }
    docs_repo = "https://github.com/acquire-project/acquire-docs"

    if not (docs_path := Path("acquire-docs", "docs")).exists():
        subprocess.check_call(["git", "clone", docs_repo])

    tuts = [docs_path / "get_started.md"]
    tuts.extend([fn for fn in docs_path.glob("tutorials/*.md") if fn.name not in skip])
    tuts.sort()

    return tuts


@pytest.mark.parametrize("tutorial", docs_repo(), ids=lambda x: x.name)
def test_tutorials(tutorial: Path):
    for block in code_block.finditer(tutorial.read_text()):
        exec(block.group(1))

[aliddell]

It would be great if we could make it a fixture that would clean up the docs repo after yielding, but I don't know if that's possible, to call a fixture from the decorator like that.

[aliddell]

Failing that, an entry in the .gitignore for tests/acquire-docs. Would hate to accidentally commit stage it with a git add . and commit it before noticing.

[tlambert03]

yeah we can do that. this whole PR isn't really close to merge and final touches. it was opened more for discussion and a possible pattern. you mentioned elsewhere that you're note sure you even want it in this repo, which certainly makes sense too. Your call. If you do think you want to have it here in this repo, i can make the whole pattern more robust. but if you think you want to move it to another repo, there's probably no point here

[aliddell]

No, on further reflection we probably want tests in both repos.

[tlambert03]

ok, sounds good. I'm away teaching a course for another week, but will have another look at the patterns used here and update stuff in a bit

[aliddell]

@tlambert03 Were you ever able to take another look at this?

[tlambert03]

sorry, not yet. will have a look today.

a quick note on your suggestion above though: creating a function that gets called inside the call to parametrize (@pytest.mark.parametrize("tutorial", docs_repo(), ids=lambda x: x.name) is pretty much the same as defining the list returned by docs_repo at module level. It doesn't "delay" that until it's needed by the fixture. The primary challenge here is to both parametrize it on each tutorial and delay cloning and cleanup the docs repo as a regular fixture (because pytest needs params to be declared at test collection time, not at run time). the alternative is to just run a for loop inside of a single test... which would certainly make for a cleaner test directory pattern, but would prevent you from testing each tut individually

[tlambert03]

ok have another look. Tests will be failing due to inconsistencies with docs, but I won't have time to look into that part