Migration: use tox to run unit tests and linting

[jayaddison]

The motivation behind this is primarily that it would have caught issue #641 (a bad import hidden by the installation of our dev-dependencies). It's also kinda relevant to #617.

I'm not 100% sure whether removing the typing-related dependencies from the distributed package is fine. I think it is - as far as I know, they're only required when we run mypy checks, not when the library itself is installed / run as a dependency.

[jayaddison]

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.

[jayaddison]

Todo: figure out how to combine this neatly with poetry. I think it should be possible in a fairly clean configuration nowadays with widespread support for PEP-517 in pip. We'll see..

[bfcarpio]

I'll admit, I'm not sure what value tox adds here. Is it the ability to run a cross platform command that runs all the checks we have or is it something else?

How exactly does it help prevent #641?

[jayaddison]

Without tox, the unit tests run in a Python environment that contains all of the app's dependencies (extruct, etc) and all of the development dependencies (black, flake8, ...)

So without tox, the library could import a dependency of flake8, and unit tests would run fine.

When running with tox, the tests run the package in an environment that only has the application dependencies installed.

(#641 was a case where the attr module was imported by a test dependency (pytest) -- and so unit tests passed without tox. if we'd run with tox, the tests would have failed with an ImportError)

[jayaddison]

Some thoughts at the moment:

• I like pytest - and the idea of using pytest-randomly to randomize unit test ordering appeals to me - but we don't really use it for anything (other than reading the online test setting -- which can go in an env variable instead), and there are probably ways to achieve random test order that don't require yet more dependencies.. so I'm planning to remove pytest soon

• Of the two pyproject.toml alternatives - I'm leaning towards poetry at the moment, mainly because it was straightforward to set up, seems relatively stable, and natively provides a lockfile format (so that we can easily follow dependency introductions/removals). It is heavier-weight than setuptools though, so I'm going to let the choice sit for a while longer.

• Still feeling fairly confident that tox is the way to go testing-wise

[jayaddison]

Ah, and it looks like I totally broke the online testing mode when introducing expected_requests anyway (tl;dr - stopped using canonical_url), so... yep, that's another vote in favour of removing pytest.

[bfcarpio]

After your description I'm on board with using tox for testing. I don't think random order testing matters too much as each scraper should be independent of one another and common code has it's own tests.

[jayaddison]

Thanks @bfcarpio. I do believe that this will be an improvement for our test infrastructure.

That said, after further consideration, I think it's possible to do this without requiring all of our developers to use tox; a few more changes coming up in this branch soon.

[jayaddison]

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.

[jayaddison]

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.

[jayaddison]

Moving ahead with this change; figuring out how to do linting sensibly without duplication seems most likely to require a bit more exploration/research.