Fixes #141 - Use a hybrid of Github Actions and TravisCI for testing.

[freakboy3742]

The Python version build matrix is being provided by Github Actions; however, we need to use TravisCI to get testing on older macOS versions.

This is the same configuration as #142; however, it radically simplifies the Travis CI configuration to avoid the use of pyenv, instead using the native Python3 that is available on these images (3.6.5).

This speeds up the build considerably; however it means we need to drop El Capitan and Sierra testing, as those TravisCI images don't have Python3 installed.

[dgelessus]

I think the general approach - having GitHub Actions workflows for everything, plus a reduced Travis CI config for testing on older macOSes - is the best solution here. This way we can take full advantage of GitHub Actions, while keeping most of our existing test environments and improving their build times.

The review comments below are a mix of conceptual questions, comments about specifics in the CI configs, and nitpicks about the documentation. The complete review became a bit long - I apologize :)

[dgelessus]

Quoting the output of python setup.py test:

WARNING: Testing via this command is deprecated and will be removed in a future version. Users looking for a generic test entry point independent of test runner are encouraged to use tox.

So it's probably best to use tox to call the tests, rather than python setup.py test.

Our current tox configuration also uses python setup.py test, so we need to change that at some point too, but not necessarily in this PR.

[freakboy3742]

Yeah. I've been (slowly) coming around on pytest... so maybe this is the trigger to make that change.

[dgelessus]

I don't think bypassing tox is a good idea, even if we only test on a single Python version. Reasons include:

• python setup.py test is deprecated (see above)
• tox sets up a clean virtual environment and properly installs rubicon-objc into it, which ensures that our library can actually be installed
• tox ensures that the Python version is actually what we expect
• tox is the documented common entry point for rubicon-objc's test suite, the CI build shouldn't bypass it
• There's no reason not to use tox with the new Travis configuration - tox can be easily restricted to only test with a single Python version

[freakboy3742]

So - I'm not a huge fan of tox.

It played a really useful role when we were trying to build multiple Python versions in a single macOS VM - but outside of that need, I'm not sure I see the value.

• tox is itself calling python setup.py test, so that's not really an argument either way

• If the Python version being used is a concern, we can invoke using python3.5; but even then, I don't see how tox provides any more protection than pyenv already provides.

• If installability is a concern, then that should be added as part of the test script, regardless how it's being invoked.

The usually given advantages to tox are that it simplifies testing matrix builds. To that, I'd answer that should be the role of the CI system, not a tool you're running locally - if only because any build matrix that's on a single machine will, by definition, be missing a large part of the build matrix.

tox also obscures success/failure status of individual sub-builds. When you run tox inside a CI build, each tox build can itself have sub-failures, which aren't surfaced to the external build reporting. Or, if they are surfaced, it's only because you're using invoking tox in a "build one specific version" configuration - at which point, tox is little more than a configuration language for running your tests... which is what your CI configuration is meant to be.

So - it helps run a partial build matrix locally, obscures the collection of test results when used in CI, and adds Yet Another configuration file to your CI configuration.

My personal preference would be to remove tox (and it's configuration).

[freakboy3742]

Oh - and the documentation currently doesn't mention tox at all - it's only referenced in the Travis CI configuation. The contribution docs reference python setup.py test, not tox as an entry point.

[dgelessus]

OK, it seems that I've misunderstood tox's intended role in the rubicon-objc tests. Was tox supposed to be just a support tool for the Travis-based CI build and not meant to be used elsewhere? My impression was that tox was intended to be the main entry point for the test suite, for both developers and CI. This also explains:

the documentation currently doesn't mention tox at all

You're right - the current documentation doesn't mention tox. I was accidentally looking at the docs from my big reference documentation PR (#118), where I changed the unit test running documentation to use tox, because I thought that was the intended entry point.

I agree that tox's Python version matrix testing is less important if our GitHub Actions build already handles that externally. I care mostly about the other features that tox provides - a central configuration file where you specify how all test/check tools are installed and called, and a single entry point (tox) that invokes all of the configured tools.

• tox is itself calling python setup.py test, so that's not really an argument either way

Ah, I should have been clearer with that point. You're right, the current tox.ini still calls python setup.py test. But if everything else used tox to run the tests, we would only have to update the single call in the tox.ini to whatever new unit testing tool we decide on, rather than every place in the docs and CI config.

• If installability is a concern, then that should be added as part of the test script, regardless how it's being invoked.

The problem is that we don't really have a test script other than tox.ini. We have a CI configuration, but you can't realistically run that locally as a developer. We have a set of unit tests, but those aren't the right place to test installation of the library (rather, the unit tests are run on the already installed library). We have python setup.py test, but aside from being deprecated, it just calls the standard unittest runner (as I undestand it). Besides, if tox already tests installation into a clean environment by default, I don't see a good reason to reimplement that ourselves.

To be clear, I'm not attached to tox in particular - if you would prefer another tool for configuring/running tests and checkers, I'm open to suggestions. The tox homepage suggests Invoke and nox as alternatives.

[freakboy3742]

It looks like the tox thing is a weird artefact of history. I added it way back in 2014 - as I recall, I was trying it out as a new tool I'd just heard of, and Rubicon's need to test multiple Python versions seemed like a good use case.

On reflection, I'm not a fan, and I'd honestly rather remove it. I haven't added a tox configuration to any recent project I've built - simply because I don't see that it adds anything.

I'm honestly not sure I see what the need for a "test script" is. The test script (at present) is python setup.py test. That will probably end up being pytest in the not-too-distant future. There is a testing prerequisite that you run make and set DYLD_LIBRARY_PATH - but those can be captured in documentation (especially since they don't need to be run every time you run the test suite during local development). On almost every other project in BeeWare, you only need the single invocation line. It isn't hard to document, though; and honestly, I'd rate the risk of needing to change documentation because we're changing test configuration tool as greater than the risk of needing to change because of the way tests are invoked.

There's also the broader interpretation of "testing" that encompasses linting, checking packaging, checking installation and so on. For me, CI is the ultimate arbiter of all this, because it's the only place where you can actually run everything in one hit, in a clean environment that hasn't been affected by the previous test run, etc. Adding a tool that wraps all these features means adding the complexity of yet another thing to configure, where the only real benefit is having a single point of entry to a test run - an entry point that, I might add, needs to be documented because no two Python projects use the same entry point.

And even if we do add a single entry point, CI won't be using it anyway, because then you lose the fidelity of reporting that a CI system can provide (i.e., telling you exactly what is broken, not just "CI Failed"). So, the "single entry point tool" is reduced to "a complex way to describe what list of commands need to be invoked". For my money, this is much easier to achieve with documentation, backed up with an actual implementation in CI.

So, in summary: I think we should remove the tox configuration; and, to be completely honest, I think we should replace it with "nothing". It's one more thing to maintain that doesn't actually help us guarantee code quality (that's the job of CI), and doesn't provide any significant benefit to onboarding new contributors (not that we're flooded with those anyway).

[dgelessus]

The test script (at present) is python setup.py test. That will probably end up being pytest in the not-too-distant future.

Hm, I'm not a huge fan of replacing python setup.py test (a generic, test-framework-agnostic entry point) with pytest (a specific test runner)... Sadly there is no proper replacement for what python setup.py test did (i. e. install the test runner used by the project and use it to run the tests).

The deprecation message from setup.py test recommends tox as a replacement, but that recommendation isn't universally agreed upon (see pypa/setuptools#931). I think the general arguments from that issue are similar to some of what you're saying - cross-version testing should be done by CI, and the test runner shouldn't set up its own virtual environments. (And in that context I agree - python setup.py test did not set up environments on its own, so its replacement shouldn't either. But that only disqualifies tox as a direct replacement for setup.py test - otherwise I think tox is a fine tool and has its uses.)

There's also the broader interpretation of "testing" that encompasses linting, checking packaging, checking installation and so on. For me, CI is the ultimate arbiter of all this, because it's the only place where you can actually run everything in one hit, in a clean environment that hasn't been affected by the previous test run, etc.

I agree that CI should officially decide whether code is working/acceptable or not, but I think as a dev it should still be possible to easily run tests, linters, etc. locally, without having to commit and push to trigger a CI build. For rubicon-objc my usual development workflow is that while developing I run the unit tests for just one Python version, and once I'm finished and ready to commit/push I run the full tox suite with all supported Python versions and the flake8 check. This way I can catch minor Python version compatibility and formatting problems more quickly than via CI, and it reduces the number of redundant commits.

This is of course still possible without tox - it's just less convenient, as I have to run each step manually (or set my IDE up to do it).

Adding a tool that wraps all these features means adding the complexity of yet another thing to configure, where the only real benefit is having a single point of entry to a test run - an entry point that, I might add, needs to be documented because no two Python projects use the same entry point.

In general that's true, but tox seems to be fairly widely used - looking at some random high-profile Python projects, mypy, pip, setuptools, numpy and requests all have a tox.ini in their repos. It's fairly likely that people will already know what tox is, and if not, the knowledge will be useful elsewhere.

And even if we do add a single entry point, CI won't be using it anyway, because then you lose the fidelity of reporting that a CI system can provide (i.e., telling you exactly what is broken, not just "CI Failed").

If you mean that the CI system can no longer differentiate between (for example) the test suite failing and flake8 failing, that could be fixed by invoking the tox environments one at a time (tox -e py in one step, tox -e flake8 in another one, etc.). mypy's Travis build does exactly this - each Travis environment runs one tox environment (possibly with extra arguments). The main advantage of this is that it's relatively easy to replicate each Travis environment locally or on another CI system (except for OS differences obviously) by running tox with the same arguments that were used on Travis.

[freakboy3742]

I can understand the desire to have a single common entry point for pre-commit tests; and I agree tox is a relatively common way to do this in the Python ecosystem.

The root of my concern is when tox doesn't meet that requirement. For example, tox doesn't currently handle invoking make; so that means we need to communicate that as an additional build requirement. So, we've adopted a tool to provide a single testing entry point... and it doesn't serve as a single testing entry point. I suspect this might be something we can fix with some more tox configuration, but it definitely isn't working like this now.

There are also complications with steps like beefore, which need to be invoked in different ways when they're in CI. Beefore will add comments to the PR to point out problems, but that means it needs to know about the PR environment. That said, this is possibly an argument to drop beefore and move back to a simpler "just invoke flake" configuration for these checks

I guess what I really want is the ability to run Github Actions configurations locally... but I suspect that's a yak I really don't want to start shaving...

[dgelessus]

For example, tox doesn't currently handle invoking make [...] So, we've adopted a tool to provide a single testing entry point... and it doesn't serve as a single testing entry point. I suspect this might be something we can fix with some more tox configuration, but it definitely isn't working like this now.

Yes, that's not the intent and should be fixed. This should be easy to do, we just need to add the make call above python setup.py test in the tox.ini.

There are also complications with steps like beefore, which need to be invoked in different ways when they're in CI. Beefore will add comments to the PR to point out problems, but that means it needs to know about the PR environment.

tox lets you pass command-line arguments into an environment (they are available as {posargs} in tox.ini), so --pull-request and such can be passed in that way. For the auth token: you can selectively pass additional envvars from the calling process (by default tox only passes through a few known ones, like PATH, and drops everything else to ensure a clean environment).

I guess what I really want is the ability to run Github Actions configurations locally...

That's sort of the point of tox, except that it's not specific to any CI system. The more configuration you put into tox.ini rather than CI-specific config files, the closer your local tox environment will be to the actual CI environment.

There are limits to this obviously. All of the context information (pull request, etc.) will be missing, but that would always be the case when running locally. You also need to simulate everything by hand that happens before tox is called, but that's mostly environment setup (Git checkout, Python and tox installation) which you usually don't want to replicate exactly (because you already have a fully set up environment). Also you can't call any GitHub Actions actions (is that what they are called?) from tox, but perhaps that's a good thing - it helps minimize vendor lock-in :)

[dgelessus]

The command "DYLD_LIBRARY_PATH=tests/objc "$(pyenv which python)" setup.py test" exited with 0.

YES

[dgelessus]

By the way, I would vote for squashing this PR once we merge it. I'm normally against squash-merging PRs because it obscures the PR's internal commit history, but in this case most of the 50 commits are going back and forth between different CI configs to find one that works properly, so I think a single squashed commit would be cleaner. (Anyone who wants to see the full details of our Travis debugging efforts can still look at the PR.)

[freakboy3742]

Agreed - I'll do a squash merge.

[freakboy3742]

Ok - Time for a (final?) review.

[dgelessus]

The PR looks good to me now, except for a few minor things that I've commented on.

I think the change from setup.py test to another test entry point doesn't have to be done in this PR - for now setup.py test is still working, it's "just" deprecated. (We should still replace it soon, but that can be done in a separate PR.)

Regarding the tox discussion, I wouldn't consider that a blocking issue for this PR either. In our case the difference between "tox" and "no tox" isn't that significant, and our build process is simple enough that you can reasonably run it by hand, so either way will work fine until we decide what general testing setup we want to use in the future.

[freakboy3742]

@dgelessus Time for another review, I guess.

I've also done a bit more poking on tox, and I think it might be possible to make tox a genuine consistent entry point (including make as a tox pre-command, and dropping beefore from our CI environment). If that makes sense as an approach - would you rather see that as a separate PR, or merged in as part of this one? I don't mind either way...

[dgelessus]

It seems I forgot to reply here... I just went over the changes again, and everything looks good to me. I'll try to fix the merge conflict that appeared in the meantime; once that's done I think this is ready to be merged.

[dgelessus]

The odd CI failure with Python 3.7 will be fixed by #157.