Infra: add tests

[hugovk]

As suggested earlier, let's test our infra code at pep_sphinx_extensions/.

I mainly aimed at testing standalone functions, and skipped the harder to test things involving Sphinx. I think this is fine, it's more important to test the more logic-based things than the whole rendering process by Sphinx and distutils. And we can extract bits of logic to make them easier to test. Still, 55% coverage isn't bad.

Bugs

I found some bugs :)

1. pep_sphinx_extensions/pep_processor/transforms/pep_headers.py had an f-string missing the f prefix

2. pep_sphinx_extensions/pep_zero_generator/author.py had an AttributeError: can't set attribute on name_parts.surname = f"\\{name_parts.surname}"

3. Same file, I couldn't reach raise ValueError("Name is empty!") because that happens when name_parts == [""] (with length 1) not num_parts == 0

Maybe bugs

And some bits I didn't figure out how to hit, could be a bug, redundant code, or neither:

1. pep_sphinx_extensions/pep_zero_generator/writer.py:

        raise ValueError( 
            "some authors have more than one email address listed:\n" 
            + "\n".join(err_output) 

2. pep_sphinx_extensions/pep_zero_generator/parser.py:

            if not author.partition(" ")[1] and author.endswith("."): 
                prev_author = author_list.pop() 
                author = ", ".join([prev_author, author]) 

To test locally

make test
# or
python3 -m pip install tox
tox
# or
python3 -m pip install pytest pytest-cov
python -m pytest  # instead of `pytest to deal with paths: https://docs.pytest.org/en/6.2.x/usage.html

Coverage is visible at htmlcov/index.html

CI

This runs on GitHub Actions, but only when there's changes in pep_sphinx_extensions/**, .github/workflows/test.yml, or tox.ini, and sends coverage to Codecov (for example).

No need to run for normal PEP PRs.

[JelleZijlstra]

Thanks for putting this together!

[AA-Turner]

Thanks Hugo! Minor comments.

A

[AA-Turner]

Should we test on multiple OSes?

[hugovk]

We could probably just do Ubuntu?

[CAM-Gerlach]

It makes sense to either only test on one platform, or test on Ubuntu, macOS and Windows. I'd prefer the latter, since these checks only run when we make build system changes and we may as well make sure things aren't broken on a specific platform (though it seems relatively unlikely).

[hugovk]

Yes, let's add Windows.

[AA-Turner]

sends coverage to Codecov

Will this also enable the fairly large comments by codecov on all PRs? (& if it will, please can we make sure it doesn't!)

A

[hugovk]

sends coverage to Codecov

Will this also enable the fairly large comments by codecov on all PRs? (& if it will, please can we make sure it doesn't!)

A

Based on recent failed attempts to re-enable the comments on PRs in https://github.com/python/bedevere (e.g. python/bedevere#432), I think not!

Comments are disabled globally for https://github.com/python (see https://app.codecov.io/account/gh/python/yaml). If they start showing up here, we can easily explicitly disable with a config file in this repo.

[CAM-Gerlach]

Thanks for this, @hugovk ! This obviously was a lot of work, but good to have. Most of my comments were fairly minor and as suggestions, though I did have a few requests. Thanks!

[CAM-Gerlach]

It makes sense to either only test on one platform, or test on Ubuntu, macOS and Windows. I'd prefer the latter, since these checks only run when we make build system changes and we may as well make sure things aren't broken on a specific platform (though it seems relatively unlikely).

[CAM-Gerlach]

Why relay on make, another automation tool (and one that will not work cross-platform, so the tests cannot be run locally through tox on Windows), instead of just calling pytest directly? It will still automatically pick up the same args so long as they are properly configured in the pytest.ini as they should be, rather than stuck in a Makefile. This just all seems rather odd, at least to me...

Also, this means that like we already are in the build job, the dependencies are installed twice—once in the outer env, which are not used, and once in the inner env via make venv, which are (AFAIK).

[hugovk]

It was to handle installing requirements, avoid repeating the pytest command, and make sure the make command is tested. But good point about Windows! Will change it.

Requirements are only installed once in this workflow, currently via make test -> make venv.

[CAM-Gerlach]

It was to handle installing requirements, avoid repeating the pytest command, and make sure the make command is tested. But good point about Windows! Will change it.

Gotcha. The other benefit to changing it is that it makes it easier to add additional arguments to control the output of the CI job, which should be -v --cov-report term at least per the discussion above.

Ideally we'd fully replace make with drop-in equivalent tox tasks, but that's an issue for another day.

Requirements are only installed once in this workflow, currently via make test -> make venv.

Ah, that's only in the build job—I got mixed up, sorry.

[hugovk]

Added:

• Windows on CI
• pytest.ini

Removed:

• Double requirements install in render.yml

[CAM-Gerlach]

Just one minor change and one tiny textual tweak, otherwise LGTM—thanks @hugovk !

[AA-Turner]

Thanks Hugo!

A