chore: Tox configuration scripts #718
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Type: Enhancement
PR Summary: This pull request introduces a significant enhancement to the project's development workflow by transitioning from Makefiles to tox configuration scripts. The change aims to leverage tox's capabilities for virtual environment management and test automation, facilitating a more standardized and efficient CI workflow. The PR includes modifications to the project's documentation, deletion of previous workflow scripts, and the addition of new scripts tailored for tox, covering installation, linting, and test coverage.
Decision: Comment
📝 Type: 'Enhancement' - not supported yet.
- Sourcery currently only approves 'Typo fix' PRs.
✅ Issue addressed: this change correctly addresses the issue or implements the desired feature.
No details provided.
📝 Complexity: the changes are too large or complex for Sourcery to approve.
- Unsupported files: the diff contains files that Sourcery does not currently support during reviews.
- Files deleted: Sourcery does not currently approve diffs with deleted files.
General suggestions:
- Ensure that the new email format in the 'Signed-off-by' lines in the CHANGELOG.md is intentional and adheres to standard conventions.
- Consider adding unit tests for the newly introduced automation scripts, particularly focusing on the
Command.runmethod to ensure its reliability across various scenarios. - It would be beneficial to include tests that specifically cover the new tox configuration scripts, ensuring they work as expected across the different Python environments specified.
- Documenting the expected test coverage threshold within the project's contribution guidelines or directly within the scripts could help maintain or improve test coverage as the project evolves.
Thanks for using Sourcery. We offer it for free for open source projects and would be very grateful if you could help us grow. If you like it, would you consider sharing Sourcery on your favourite social media? ✨
|
|
||
|
|
||
| def coverage(): | ||
| return Command.run("poetry run pytest --cov=./ --cov-report=html -vv") |
There was a problem hiding this comment.
suggestion (llm): While the coverage command is a good addition, it's crucial to ensure that the test suite includes tests that cover the new tox configuration scripts. This includes testing for different Python environments as specified in the tox.ini file. If such tests are not present, I recommend adding them to verify that the tox configurations work as expected across the specified Python versions.
| @@ -0,0 +1,19 @@ | |||
| import subprocess | |||
There was a problem hiding this comment.
suggestion (llm): It's great to see automation scripts being added to enhance the development workflow. However, it would be beneficial to include unit tests for these scripts themselves, especially for the Command.run method, to ensure its reliability and handle potential exceptions gracefully. Testing the scripts will help in maintaining the robustness of the development workflow.
|
|
||
|
|
||
| def coverage(): | ||
| return Command.run("poetry run pytest --cov=./ --cov-report=html -vv") |
There was a problem hiding this comment.
suggestion (llm): The addition of a coverage command is commendable as it emphasizes the importance of test coverage. However, it would be beneficial to document the expected coverage threshold within the project's contribution guidelines or as a comment within this script. This ensures that contributors are aware of the coverage expectations and maintain or improve the test coverage with their contributions.
|
Sorry for the noise, CI finally looks green ✅ |
|
Hey @tinvaan, I'll have a deeper look soon and get back |
Changes generated by: mdformat-gfm
|
Resolved merge conflicts & painted CI green again. /cc @J0 |
|
Thanks for the PR! I think the maintaining team will need to look through this before we can decide whether to move forward as it affects the development flow. It may take a while. |
|
Closing this PR for the reasons given on #696 (comment). |
What kind of change does this PR introduce?
Project maintenance, dev workflow enhancement.
What is the current behavior?
#696
What is the new behavior?
Move away from
Makefilestotoxconfiguration scripts.toxis tailor made for python projects compared toMakefilesand allows for venv management and test automation + standardization in CI workflows.For eg, the below config will run our tests against multiple python environments, to help guard against any breaking changes we introduce and thus ensuring backward compatibility.
Additional context
Tests against all supported versions can be launched using a single
toxcommand and optionally running tests against specific versions is also supported using thetox -eargument.