Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide CI testing support #8

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Conversation

mitchnegus
Copy link
Collaborator

This provides a starting point for converting our GitLab CI testing infrastructure to the GitHub Actions workflow.

In the process, this fixes a testing error identified by testing the FIREWHEEL deployment in an fresh installation (a hardcoded path in the test_cli_completion.py script.

To avoid duplication in the workflow scripts, I've created a new composite action to install the tox dependencies. We should hopefully be able to use these composite actions to dramatically simplify some of the CI scripts we had before.

@mitchnegus mitchnegus marked this pull request as draft November 15, 2024 23:38
@mitchnegus mitchnegus marked this pull request as ready for review November 15, 2024 23:39
@mitchnegus mitchnegus marked this pull request as draft November 15, 2024 23:39

runs:
using: "composite"
steps:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems reasonable to also require installing Python here. Are there compelling reasons why choosing a standard version (3.11) and using that for all tox jobs wouldn't work?

Copy link
Collaborator Author

@mitchnegus mitchnegus Nov 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I had not included that one was because I hadn't put thought into how the matrixing versions (in lint) and full inclusion of all versions (in the tests) should be reconciled.

When we run tox -e py39,py310,... it runs an individual tox job for tests for each. That means that the runner has to have all those versions of Python installed to start with, because tox will look for each Python as it runs each job. But for linting, our tox -e lint job will just use whichever Python is the default in our virtual environment (I think).

The setup-python action seems to allow you to specify the default Python version as the last version identified among multiple selected. I'd think if nothing else we might want to control which version is used for linting (e.g., 3.11 like you suggested).

Also, another concern that popped into my head might be that there could be a performance penalty for installing all versions of Python for the linting job. From what I can see on the pipelines right now, it's less than 1 second for just one version at a time and closer to 9 seconds to install all 6 (Python 3.8 through 3.13). That's not huge for a CI pipeline—especially if it's running concurrent with tests which will almost always take far longer—so I'm inclined to think it's not a big deal.

@mitchnegus mitchnegus force-pushed the tests branch 3 times, most recently from 5339623 to 0565cc7 Compare November 21, 2024 16:44
@mitchnegus mitchnegus force-pushed the tests branch 2 times, most recently from c210aa1 to 56e1493 Compare December 2, 2024 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants