Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Migration: use tox to run unit tests and linting #650

Merged
merged 9 commits into from Oct 20, 2022
Merged

Migration: use tox to run unit tests and linting #650

Show file tree
Hide file tree
Changes from 6 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 Oct 17, 2022
43c30f2
Remove typing-related modules from library dependencies
jayaddison Oct 17, 2022
612651a
Cleanup: remove empty tox.ini section
jayaddison Oct 17, 2022
1092ee8
Fixup: yaml key name in GitHub Actions workflows (was:name, intended:…
jayaddison Oct 17, 2022
fe9bcc6
Relocate platform-specific binary installation of lxml from GitHub Ac…
jayaddison Oct 17, 2022
520b386
Nitpick: disambiguate between platform regex and tox environment name…
jayaddison Oct 17, 2022
5051258
Cleanup: remove package self-installation-dependency from dev-require…
jayaddison Oct 18, 2022
f48bfd5
Revert changes to requirements-dev.txt and README files
jayaddison Oct 20, 2022
58ad0d2
Merge branch 'main' into issue-617/tox-migration
jayaddison Oct 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions .flake8
View file
Open in desktop
Expand Up @@ -6,5 +6,6 @@ max-line-length = 88
max-complexity = 18
select = B,C,E,F,W,T4,B9
exclude =
./.tox/
./.venv/
./venv/
11 changes: 2 additions & 9 deletions .github/workflows/linters.yaml
View file
Open in desktop
Expand Up @@ -17,12 +17,5 @@ jobs:
uses: actions/setup-python@v4
with:
python-version: "3.x"
- name: Install dependencies
run: |
python -m pip install --upgrade pip
pip install -r requirements-dev.txt
- name: black and flake checks
run: |
black --check .
flake8 --count .
mypy recipe_scrapers tests
- run: pip install tox
- run: tox -e lint
19 changes: 6 additions & 13 deletions .github/workflows/unittests.yaml
View file
Open in desktop
Expand Up @@ -23,26 +23,19 @@ jobs:
python-version: "3.11.0-rc.1"
- os: windows-latest
python-version: "3.11.0-rc.1"
include:
- toxenv: py
- os: macos-latest
toxenv: py-darwin
runs-on: ${{ matrix.os }}
steps:
- uses: actions/checkout@v3
- name: Set up Python
uses: actions/setup-python@v4
with:
python-version: ${{ matrix.python-version }}
# 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
- name: Install lxml from wheel on MacOS
if: ${{ matrix.os == 'macos-latest' }}
run: pip install --only-binary=lxml lxml
- name: Install dependencies
run: |
python -m pip install --upgrade pip
pip install -r requirements-dev.txt
- name: Unittest and Coverage Report
run: |
python run_tests.py
- run: pip install tox
- run: tox -e ${{ matrix.toxenv }}
# Provide code coverage reports on Linux
- if: ${{ matrix.os == 'ubuntu-latest' }}
name: coveralls.io
Expand Down
2 changes: 1 addition & 1 deletion README.rst
View file
Open in desktop
Expand Up @@ -341,7 +341,7 @@ Assuming you have ``>=python3.7`` installed, navigate to the directory where you
source .venv/bin/activate &&
pip install -r requirements-dev.txt &&
pre-commit install &&
python run_tests.py
tox -e py

In case you want to run a single unittest for a newly developed scraper

Expand Down
9 changes: 1 addition & 8 deletions requirements-dev.txt
View file
Open in desktop
@@ -1,12 +1,5 @@
-e .
black>=22.3.0
coverage>=4.5.1
flake8>=3.8.3
flake8-printf-formatting>=1.1.0
pre-commit>=2.6.0
pytest>=6.1.1
responses>=0.21.0
unittest-parallel>=1.5.0
mypy>=0.971
tox>=3.26.0
# language-tags>=1.0.0
# tld>=0.12.3
2 changes: 0 additions & 2 deletions setup.py
View file
Open in desktop
Expand Up @@ -29,8 +29,6 @@
"extruct>=0.8.0",
"isodate>=0.6.1",
"requests>=2.19.1",
"types-beautifulsoup4>=4.11.6",
"types-requests>=2.28.10",
],
packages=find_packages(),
package_data={"": ["LICENSE", "py.typed"]},
Expand Down
29 changes: 29 additions & 0 deletions tox.ini
View file
Open in desktop
@@ -0,0 +1,29 @@
[testenv]
deps =
Copy link
Collaborator Author

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's deps directive supports requirements.txt-format files.

We may be able to update testenv and/or lint to read those from a shared location.

Copy link
Collaborator Author

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:

  1. GitHub Actions unit tests: run isolated tests using tox -- install package & dependencies from pyproject.toml, but without any other dependencies
  2. Run unit tests locally: with or without package installed via pyproject.toml -- run tests using unittest-parallel, or python -m unittest directly; requires package dependencies installed, preferably into a virtualenv
  3. Run GitHub Actions lint checks: run isolated linting using tox -- install lint tools
  4. Run pre-commit checks locally: ideally using an isolated virtualenv (confirm: does pre-commit do this by default?), run flake8, black, etc

It'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 perhaps tox should run/install pre-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.

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 .
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note: this is a like-for-like migration of the existing flake8 behaviour (count only). To help people out in future though, we could/should probably run it without arguments so that it produces output when it detects problems.

mypy recipe_scrapers tests