-
Notifications
You must be signed in to change notification settings - Fork 4
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
Add xdoctest #141
Add xdoctest #141
Conversation
WalkthroughThis pull request introduces several changes across multiple files, enhancing unit testing capabilities, improving logging practices, modifying error handling, and updating project configuration. Key updates include the introduction of a Changes
Possibly related PRs
Suggested labels
Poem
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- .github/workflows/unit_tests.yml (1 hunks)
- ovos_dinkum_listener/service.py (6 hunks)
- ovos_dinkum_listener/transformers.py (1 hunks)
- ovos_dinkum_listener/voice_loop/hotwords.py (1 hunks)
- pyproject.toml (1 hunks)
- requirements/tests.txt (1 hunks)
- setup.py (0 hunks)
Files not reviewed due to no reviewable changes (1)
- setup.py
Files skipped from review due to trivial changes (3)
- ovos_dinkum_listener/service.py
- ovos_dinkum_listener/voice_loop/hotwords.py
- requirements/tests.txt
Additional comments not posted (7)
pyproject.toml (6)
1-3
: LGTM!The
[build-system]
section is correctly configured with the appropriaterequires
andbuild-backend
fields for a Python project usingsetuptools
.
5-6
: Approved with a note of caution.The
[tool.mypy]
section is correctly configured to ignore missing imports, which can help streamline development by reducing noise from unresolved dependencies.However, please be cautious when using this configuration as it may also mask potential issues with missing imports. Ensure that all required dependencies are properly installed and imported in the code.
8-11
: LGTM!The
[tool.pytest.ini_options]
section is correctly configured with appropriate options for testing and test discovery.
- The
addopts
field enables running doctests and ignores thesetup.py
file during test discovery.- The
norecursedirs
field excludes unnecessary directories from test discovery.- The
filterwarnings
field sets the default warning filter.These configurations enhance the testing process by focusing on relevant files and enabling doctests.
13-15
: LGTM!The
[tool.coverage.run]
section is correctly configured to collect meaningful coverage data for theovos_dinkum_listener
package.
- The
branch
field is set totrue
, ensuring that branch coverage is collected.- The
source
field specifies theovos_dinkum_listener
package as the source of coverage data, focusing the coverage analysis on the relevant code.These configurations contribute to effective coverage analysis and reporting.
17-35
: LGTM!The
[tool.coverage.report]
section is correctly configured to exclude irrelevant or intentionally uncovered lines from coverage reports.
- The
exclude_lines
field specifies a comprehensive list of regular expressions matching lines to exclude, such as those marked with specific comments, certain control flow statements, and other patterns.- The
omit
field excludes thesetup.py
file from coverage reports.These configurations ensure that the coverage metrics reflect meaningful code and provide a more accurate representation of the project's test coverage.
37-38
: LGTM!The
[tool.xdoctest]
section is present with an emptyoptions
field, indicating that the project is ready for documentation testing usingxdoctest
.Although no additional options are specified, the presence of this section suggests that
xdoctest
is properly integrated into the project's testing workflow..github/workflows/unit_tests.yml (1)
57-57
: Excellent improvements to the testing process!The updated
pytest
command introduces several beneficial changes:
- Specifying the coverage configuration file ensures consistent coverage settings.
- Generating an HTML coverage report provides a visual representation of test coverage, making it easier to identify areas that may require additional testing.
- Providing terminal output for coverage gives immediate feedback during test runs.
- Measuring test durations helps identify slow tests that may need optimization, contributing to faster CI/CD pipelines.
- Setting the coverage target to the
ovos_dinkum_listener
module ensures that the coverage metrics are focused on the relevant code.These enhancements will greatly improve the quality and maintainability of the codebase by providing more comprehensive and informative test results.
de27d33
to
d9eb3e6
Compare
@@ -54,4 +54,4 @@ jobs: | |||
pip install -r requirements/tests.txt | |||
- name: Run unittests | |||
run: | | |||
pytest test/unittests | |||
pytest --cov-config pyproject.toml --cov-report html --cov-report term --durations 5 --cov=ovos_dinkum_listener |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider a pytest.ini file, as this could start to get unwieldy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or add it to pyproject.toml, which I just noticed you added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are extra options on top of the ones in pyproject.toml. Adding coverage is not free, and often a single run of pytest
is enough for a developer, so I wanted to ensure the pyproject-level options enabled all test by default, but I only wanted the extra coverage reporting on the CI.
But this is a design decision, and making CI and the user the same by default is reasonable. Typically what I do is I add a "./run_tests.sh" script to my repo root, which does the tests with coverage, but leave pytest just run the tests and don't bother with coverage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually this is a better example of a more modern run_tests.py that I use:
https://gitlab.kitware.com/computer-vision/kwimage/-/blob/main/run_tests.py?ref_type=heads
This PR tweaks some of the CI to include xdoctest as a pytest plugin and run doctests on the repo. I've also added code coverage and one simple doctest as an example. I cleaned up one unused import in setup.py
Summary by CodeRabbit
New Features
CyclicAudioBuffer
class to improve documentation.Bug Fixes
Chores