Merged
Conversation
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This adds a
tox.inifile so thattoxcan be run locally to test on 3.7, 3.8, 3.9, 3.10, 3.11, and 3.12, or whichever of them are available, and also run the lint, formatting, and style checks.Because CI also tests building the documentation, and it is convenient to be able to use
toxto check all the things CI checks (albeit on just one's local platform), I have added a tox environment for that too. But because it actually writes to the build directory, I have not listed it inenv_listat the top, so it doesn't run automatically--it can still be run withtox -e html. That could be improved in the future, but I think reasonable ways of doing so would involve modifyingdoc/Makefileto make it more configurable, and that seems outside the proper scope of this PR.This PR does not use
toxon CI. It is possible to usetoxto drive tests on CI, such as with thetox-gh-actionsplugin, but this does not do that, because:toxfor this project is not primarily for what it brings to CI nor even for making local and CI based testing more consistent.toxworkflow will sometimes make results harder to read, because they are no longer divided into top-level steps belonging to the test job and clearly shown as separate. This doesn't mean it shouldn't be done, and either manual customization or alternatives totox-gh-actionssuch astox-gh-matrixmight help.toxon CI are for when thetoxenvironments are more granular than the CI jobs. This happens in projects that need to parameterize their test runs not just on OS and Python version but also on dependency versions (while avoiding the proliferation of separate CI jobs), which I don't think is applicable here.So while I suspect it may eventually be a good idea to use
toxon CI, I don't think it should be done until it is clearly no worse than the current approach and other readily available approaches.My intention is also not for
toxto replace other ways of running tests when developing locally. During routine development,toxmay be too slow or its output too complicated, for regularly checking the status of some or all tests. Furthermore,pre-commitseems to be working great for the lint checks--and the lint tox environment I configured uses it, to as to pick up any new or changed linting tools automatically. (Also, not everyone likestox.)For these reasons, I have not modified the readme to replace anything with
tox-based instructions. However, the readme could be modified to also talk abouttox. I am not sure if that should be done either, because the users most interested in usingtoxare those already familiar with it, and they will notice thetox.ini. Furthermore, I think it may be best to see how it works out (for example, should it really have the environment for testing that we can build docs by actually building them?) before adding it to the readme or other project documentation. However, if you'd liketoxinstructions, or a mention of it, etc., added the readme, I'd be pleased to add it (as well as to make other requested changes).Some other considerations:
toxto run tests was being prompted to enter my passphrase for some tests. Because that completely breaks the useful automationtoxprovides, I allowedtoxto pass environment variables whose names start withSSH_through to its test environments. It would probably be sufficient to pass throughSSH_AGENT_PIDandSSH_AUTH_SOCK; I'd be pleased to change it to just that, if requested. (If other forms of authentication than SSH that I'm not using rely on environment variables being set, then that might still be broken.)toxenvironments for everything other than the tests are specified as 3.9, to support Cygwin and to allow results to be compared between Cygwin and other platforms. Also, because Cygwin doesn't have 3.10, 3.11, or 3.12, I have not specifiedskip_missing_interpreters = true, leaving it to the default offalse. Nonetheless, I do not claimtoxbehaves perfectly on Cygwin in this configuration. I sometimes have problems where it tries to use Python interpreters from my native system before eventually finding something it can really use.toxfromrequirements-dev.txt, and not added it totest-requirements.txtsince it should rarely be installed together with other project dependencies in the same environment. It's possible to installtoxin a virtual environment that one is using for the project, but this is not typically done, becausetoxcreates its own environments. It's best to installtoxwithpipxwhen possible. Furthermore, if any version oftoxis available, and that version is lower than the version specified intox.ini, thentoxwill bootstrap a virtual environment and install a newer version of itself in it. Of course, one can still runpip install toxin a virtual environment (or the global environment) if one wishes to gettoxthat way.toxconfiguration inpyproject.tomlto avoid adding yet another top-level file. But I decided against it. It forgoes the discoverability benefits noted above in my reasoning about why I haven't mentionedtoxin the readme. Also, it has to be given as one giant string, and editors (and GitHub) don't currently special-case that with useful syntax highlighting.