Skip to content

Run doctests in pytest. Add linters to pre-commit. Add a pre-commit workflow.#96

Merged
aliddell merged 28 commits intoacquire-project:mainfrom
aliddell:92-run-tests-on-documented-examples
Dec 13, 2023
Merged

Run doctests in pytest. Add linters to pre-commit. Add a pre-commit workflow.#96
aliddell merged 28 commits intoacquire-project:mainfrom
aliddell:92-run-tests-on-documented-examples

Conversation

@aliddell
Copy link
Member

@aliddell aliddell commented Oct 6, 2023

  • Adds doctests for pyi and md files.
  • Pulls pytest arguments used in build / test workflows into pyproject.toml.
  • Adds ruff and rustfmt checks to pre-commit.
  • Adds a pre-commit check workflow that runs on PRs into main.

@aliddell aliddell requested review from andy-sweet and nclack October 6, 2023 20:48
@aliddell aliddell force-pushed the 92-run-tests-on-documented-examples branch from a48ca40 to 03245c8 Compare October 6, 2023 21:44
Copy link
Member

@nclack nclack left a comment

Choose a reason for hiding this comment

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

Looks good to me.

I know you've got a question for @andy-sweet and I wonder about other things to get into the pre commit (e.g. ruff and cargo fmt). So feel free to hold off on merging till you're comfortable. But I think this can go in as is.

Copy link
Contributor

@andy-sweet andy-sweet left a comment

Choose a reason for hiding this comment

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

Running doctests as part of pre-commit feels a bit heavy to me (e.g. compared to linting), especially when those tests depend on locally connected hardware.

I think that pytest should just pick up on any doctests in Python modules by default and we can modify the glob pattern to also look at the Markdown files (and Rust files if needed). In that case, we don't need to add any new workflows either (though adding a remote pre-commit might be a good idea).

@aliddell
Copy link
Member Author

I added a check on the .pyi files to test_docs.py and removed the pre-commit check, opting to just call pytest -k test_docs from the usual tests workflow.

@aliddell aliddell requested a review from andy-sweet November 15, 2023 19:48
Copy link
Contributor

@andy-sweet andy-sweet left a comment

Choose a reason for hiding this comment

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

Mostly looks good, but I don't think we need to explicitly collect doctests with test_docs.py, in which case we can and should make a few simplifications.

@aliddell aliddell requested a review from andy-sweet December 12, 2023 16:39
Copy link
Contributor

@andy-sweet andy-sweet left a comment

Choose a reason for hiding this comment

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

This PR is mostly about adding code format checkers to pre-commit (and the resulting changes that causes) than it is about running the tests on documented examples.

I'm fine with that (i.e. no need to split the PR), but at least update the title and description to match that.

Comment on lines +67 to +74
exclude = [
"__init__.py"
]

ignore = [
"F403",
"F405",
] No newline at end of file
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm OK to start with these excludes and ignores, though I'd prefer to have none of them in the long term.

@aliddell aliddell changed the title Run tests on documented examples Run doctests in pytest. Add linters to pre-commit. Add a pre-commit workflow. Dec 13, 2023
@aliddell aliddell merged commit c04f714 into acquire-project:main Dec 13, 2023
@aliddell aliddell deleted the 92-run-tests-on-documented-examples branch December 13, 2023 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants