Skip to content

Test docs #175

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Binary file added .coverage.M3-Macbook.37567.XbVcFOqx
Binary file not shown.
1 change: 1 addition & 0 deletions .github/workflows/test_pr.yml
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ jobs:
run: |
python -m pytest -k test_basic
python -m pytest -k test_zarr
python -m pytest -k test_docs

dcam:
name: Python ${{ matrix.python }} (DCAM)
Expand Down
1 change: 1 addition & 0 deletions acquire-docs
Submodule acquire-docs added at d260e5
34 changes: 34 additions & 0 deletions tests/test_docs.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
from pathlib import Path
import subprocess
import re
import logging
import pytest
import os

if not os.getenv("CI") or os.getenv("SKIP_DOCS_TEST", False):
pytest.skip("Skipping docs test", allow_module_level=True)
Copy link
Member

Choose a reason for hiding this comment

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

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.



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

DOCS_REPO = "https://github.com/acquire-project/acquire-docs"
CODE_BLOCK = re.compile(r"```python\n(.*?)```", re.DOTALL)
SKIP = {
"setup.md", # has invalid syntax
"trigger.md", # has some non-existant paths
}

# NOTE: this clones the repo on import... not the best practice, could be improved
if not (DOCS_PATH := Path("acquire-docs", "docs")).exists():
subprocess.check_call(["git", "clone", DOCS_REPO])
Copy link
Member

Choose a reason for hiding this comment

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

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))

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

@aliddell aliddell Apr 11, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@tlambert03 tlambert03 May 28, 2024

Choose a reason for hiding this comment

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

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



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()


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