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
Migration: use tox to run unit tests and linting #650
Merged
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
760f34e
Migrate unit testing and linting to use tox
jayaddison 43c30f2
Remove typing-related modules from library dependencies
jayaddison 612651a
Cleanup: remove empty tox.ini section
jayaddison 1092ee8
Fixup: yaml key name in GitHub Actions workflows (was:name, intended:…
jayaddison fe9bcc6
Relocate platform-specific binary installation of lxml from GitHub Ac…
jayaddison 520b386
Nitpick: disambiguate between platform regex and tox environment name…
jayaddison 5051258
Cleanup: remove package self-installation-dependency from dev-require…
jayaddison f48bfd5
Revert changes to requirements-dev.txt and README files
jayaddison 58ad0d2
Merge branch 'main' into issue-617/tox-migration
jayaddison File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,5 +6,6 @@ max-line-length = 88 | |
max-complexity = 18 | ||
select = B,C,E,F,W,T4,B9 | ||
exclude = | ||
./.tox/ | ||
./.venv/ | ||
./venv/ |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
[testenv] | ||
deps = | ||
coverage >= 4.5.1 | ||
responses >= 0.21.0 | ||
unittest-parallel >= 1.5.0 | ||
commands = | ||
unittest-parallel -t . -s tests --coverage --coverage-rcfile .coveragerc | ||
|
||
# The system-provided libxml2 on MacOS is typically outdated and this can lead to lxml parsing issues | ||
# Using PyPi-provided binary wheels instead resolves this | ||
# We are affected by https://bugs.launchpad.net/lxml/+bug/1949271 in test_wild_mode when using system-provided libxml2 on MacOS | ||
platform = | ||
py-darwin: darwin | ||
install_command = | ||
py-darwin: python -m pip install --only-binary=lxml {opts} {packages} | ||
|
||
[testenv:lint] | ||
skip_install = true | ||
deps = | ||
black >= 22.3.0 | ||
flake8 >= 3.8.3 | ||
flake8-printf-formatting >= 1.1.0 | ||
mypy >= 0.971 | ||
types-beautifulsoup4 >= 4.11.6 | ||
types-requests >= 2.28.10 | ||
commands = | ||
black --check . | ||
flake8 --count . | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note: this is a like-for-like migration of the existing |
||
mypy recipe_scrapers tests |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Note-to-self:
tox.ini
'sdeps
directive supportsrequirements.txt
-format files.We may be able to update
testenv
and/orlint
to read those from a shared location.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.
Use cases I'm aware of:
tox
-- install package & dependencies frompyproject.toml
, but without any other dependenciespyproject.toml
-- run tests usingunittest-parallel
, orpython -m unittest
directly; requires package dependencies installed, preferably into a virtualenvtox
-- install lint toolsflake8
,black
, etcIt's possible that items (3) and (4) can be consolidated.
Developers should be able to choose to run (2) and (4) if they like locally - (1) and (3) will help them to identify problems if they don't (it's personal - and sometimes situational - preference about whether to be strict locally and/or use cloud compute to detect problems, especially bearing in mind multi-platform support)
At the moment as of this branch at 58ad0d2, the linting dependency list is duplicated between
tox.ini
(tox lint),requirements-dev.txt
, and.pre-commit-config.yaml
- all slightly divergent.Could we remove that duplication? Yep: linting/checking behaviours should be isolated. And
pre-commit
seems designed for that purpose; so perhapstox
should run/installpre-commit
to achieve lint checks (both in continuous integration and locally).Regardless:
tox
itself seems like a unit testing improvement in terms of test isolation, so let's begin using it for continuous integration.