Initial attempt at a GHA test workflow

[altendky]

Draft for:

• Working
• Self review

[altendky]

I have a lot of CI setups to build from but this happens to be based on https://github.com/twisted/towncrier/blob/2841c12f2e7ebfc40d9cc3228ca468d339225549/.github/workflows/ci.yml. It's a bit on the verbose side compared to what is needed here, but I have a tendency to try to be more consistent rather than most minimal. If you prefer minimal, I can cut it down.

One key bit of structure is that the artifacts, .whl and .tar.gz get built first and then tested in each case. In towncrier, the exact same files also get uploaded to PyPI when tagged for release. I see you already have a publish workflow. It could optionally be added to the end of this to retain that flow of publishing the exact files that were tested in CI.

Another specific point is that first the sdist is built, then the wheel is built from that. There's some discussion of this over at twisted/towncrier#323. The point is to make sure that the sdist itself is valid as well as the wheel without matrixing across testing them each separately.

[altendky]

It looks like there is config for isort and mypy and... maybe it would make sense to tackle those separately instead of exploding this single PR.

[altendky]

Ah yes, the All job is there as compensation for GHA not having a feature to require that all jobs succeed. With the All job in place you can select just a single required check in the GitHub branch protection configuration. You then do not have to updated those checks as you add and remove various matrix cells. Just keep the workflow itself correct.

[mristin]

LGTM! Thanks :-)!

Could you please also delete .travis.yml and update the badge in the Readme?

[mristin]

Will this also run precommit.py?

[altendky]

We could setup whatever to run, sure. I personally haven't gotten up to working on precommit stuff, just hasn't bubbled up the list... but yeah. Maybe make a ticket and list out all the bits you want. As mentioned, I think it's worth getting the basics in and then filling out the rest.

[altendky]

I am correct that from the options of Linux, macOS, and Windows... this is only expected to work in Linux, right? Windows is... not nix'y. And macOS uses otool or something instead of ldd.

[altendky]

Ready to merge as far as I'm concerned. Lots of work can be done, but having the tests covering the Python versions is a good place to start.

[mristin]

Sorry, I noticed this only here. Why do you want to include tests in the distribution? Would this mean that the tests are also shipped to pypi.org?

[altendky]

I personally put my tests inside the actual package and distribute them everywhere the code goes. But sure, lots of people don't. I'll look into this because perhaps the tests are getting installed even when you just pip install ..

[altendky]

pylddwrap/setup.py

Line 46 in 60b8b05

packages=find_packages(exclude=['tests']),

Ok, so you do exclude the tests from installation. Do you feel it is bad to include the tests in the sdist? It seems fairly reasonable to me to have them available for people that get grabbing the source.

To directly respond to your questions, it is useful to have tests available wherever you have the code so that no matter how you have the code you can run the tests. Sure, most people won't leverage this. They pay the price of 36K or whatever of disk space used. A bare venv with nothing extra installed is 15MB so... Again, sure, lots of packages don't do this. I don't know where I would even go for an authoritative should or shouldn't on this but the age old distutils says it defaults to including them.

https://docs.python.org/3/distutils/sourcedist.html#specifying-the-files-to-distribute

anything that looks like a test script: test/test*.py (currently, the Distutils don’t do anything with test scripts except include them in source distributions, but in the future there will be a standard for testing Python module distributions)

As to them being shipped to PyPI. Yes, if they are either in the sdist or the wheel then the expectation would be that they end up on PyPI.

[mristin]

Please correct me if I'm wrong -- I'm not familiar with testing Python packages this way.

Wouldn't it be simpler to just install dependencies and run the pre-commit script:

- name: Install dependencies
  run: pip3 install .[dev]

- name: Run checks
  run: python precommit.py

? This simplification would also allow you to dispense of MANIFEST.in?

[mristin]

Let's see how to fix this in a separate pull request.

[altendky]

Just installing from the source doesn't check to see if the built sdist and wheel work. Usually it doesn't matter. Sometimes you get into situations where you end up missing files in the sdist. The purpose of this overall structure is to 1) create the artifacts you are going to publish (sdist and wheel) then 2) test that single wheel file in all places it might be used (that you cover in CI) and 3) publish those same exact artifacts that were built in the first step. Then the thing to be published is the thing that gets tested. Rather than doing several different installations from several different source checkouts and testing that followed by creating an sdist and wheel to upload that never get tested at all.

But, I'm going to unresolve this conversation and take a look at the precommit before moving on.

[altendky]

I am used to tox being the central location for activity descriptions. It looks like you are using precommit for that role. If using precommit.py, I think it may make sense to add some more configurability (arguments) for it to be used here. I'm used to linting activities (isort, mypy, yapf, etc) being run on the vcs copy and tests with coverage being run on the installed package.

MANIFEST.in is just about describing what should and shouldn't be in your sdist. I almost certainly don't have the MANIFEST.in correct, but I don't think precommit is an alternative.

I'm going to phone a friend on this one since I don't have experience with precommit and what sort of setup works out well for all the use cases. @graingert, if you have a couple minutes to help get me up to speed... thanks.

[mristin]

Thanks for the clarification re tests! I haven't spent too much thought on it -- it actually does make sense to include the tests so that users can test for the "dependency hell" and that everything works with their virtual environment which might have certain dependencies with different versions already installed.

We used precommit.py just because it was simple and convenient. If you are familiar with tox, please feel free to move the code into tox. In some packages I wanted to add more complex logic into precommits, so tox falls short in that regard. For this particular package, though, tox should be just fine.

[mristin]

Should I merge the pull request as it is or you'd like to integrate first the pre-commit (tox or otherwise)?

[altendky]

I guess go ahead and merge it. As is, it is better than the present state of not having CI. Maybe some of the other bits will be able to be tackled in focused PRs. I'm certainly game for adding more tox, but I don't want to remove the existing precommit functionality that I presume you like. I need to figure out how they play together without too much repetition either of code or of env management.

[mristin]

Thanks, merged! You can just split up precommit.py into tox.ini as you see fit.

[mristin]

Thanks again a lot, @altendky ! Could you please have another look at the latest comment? Otherwise, it's ready to merge from my side as well.

[altendky]

@mristin, thanks. If you want to have these be required to pass you can setup branch protections and check the All check (corresponds to the All job in the yaml). It is optional whether the branch protection applies to admins so I think you could leave that unset if you like to commit directly to master. I haven't looked over your development practices.

https://docs.github.com/en/github/administering-a-repository/managing-a-branch-protection-rule