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.

Sign up for GitHub

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

Provide CI testing support #8

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
47a21a8
Remove hardcoded path in CLI completion test
mitchnegus Nov 15, 2024
6bea74e
Use a composite action to prepare tox
mitchnegus Nov 15, 2024
7dd21a7
Update workflow comment headers
mitchnegus Nov 15, 2024
94b170f
Add unit tests to GitHub workflows
mitchnegus Nov 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions .github/actions/prepare-tox/action.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
# A composite action to prepare tox-based workflows

name: "Prepare tox"

description: "Prepare tox-based workflows"

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
Member Author

@mitchnegus mitchnegus Nov 18, 2024 •
edited
Loading

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I've thought more about this, and I think adding Python here would make things difficult. Based on the comment above, in these tests we need to do three distinct things:

  1. Unit test – tox run/tox -e py39,py310...: Run unit tests with tox, with tox running tests for each version of Python. This requires that all tested versions of Python be installed on the runner for this one job.
  2. Documentation – `tox -e docs: Build documentation with just any old Python version that is installed (only needs to be done once).
  3. Lint – tox -e lint: Run linting with tox, where tox will run the linting job with whatever Python version is the default. Hence, we use the version matrixing so that only the Python version we want to use for linting is installed for a given workflow, and tox uses that (e.g., job 1 installs 3.9 and lints with 3.9, before job 2 installs 3.10 and lints with 3.10, etc.).

That all means that (1) needs all versions installed, (2) only needs one version installed, but if there are multiple versions it should be fixed as some mid-lifecycle default (e.g., 3.11), and (3) only needs one version installed at a time, but must be run multiple times with the majority of those runs being something other than the default.

Reconciling these conditions, especially if we install all the versions but then need to explicitly call tox with one version of Python or the other, is a more difficult means of accomplishing the same effect as installing only the versions we need for each job but then performing the rest of the tox setup in a common manner.

(I'm also still quite green in my understanding of GitHub workflows/actions, so if you think this misses the mark I'm open to other suggestions.)

- name: "Ensure upgraded pip"
run: pip install --upgrade pip
shell: bash
- name: "Install tox"
run: pip install tox
shell: bash
9 changes: 3 additions & 6 deletions .github/workflows/documentation.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
# This workflow will install Python dependencies, run tests and lint with a variety of Python versions
# For more information see: https://docs.github.com/en/actions/automating-builds-and-tests/building-and-testing-python
# This workflow will build and deploy the FIREWHEEL documentation

name: Documentation

Expand Down Expand Up @@ -38,9 +37,7 @@ jobs:
with:
python-version: '3.x'
- name: Install dependencies
run: |
python -m pip install --upgrade pip
python -m pip install tox
uses: ./.github/actions/prepare-tox
- name: Build Documentation
run: |
tox -e dependencies,docs
Expand All @@ -60,4 +57,4 @@ jobs:
steps:
- name: Deploy to GitHub Pages
id: deployment
uses: actions/deploy-pages@v4
uses: actions/deploy-pages@v4
6 changes: 2 additions & 4 deletions .github/workflows/linting.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# This workflow will install Python dependencies, run tests and lint with a variety of Python versions
# This workflow will install Python dependencies and lint with a variety of Python versions
# For more information see: https://docs.github.com/en/actions/automating-builds-and-tests/building-and-testing-python

name: Linting
Expand Down Expand Up @@ -27,9 +27,7 @@ jobs:
with:
python-version: ${{ matrix.python-version }}
- name: Install dependencies
run: |
python -m pip install --upgrade pip
python -m pip install tox
uses: ./.github/actions/prepare-tox
- name: Lint code
run: |
tox -e lint
Expand Down
31 changes: 31 additions & 0 deletions .github/workflows/testing.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
# This workflow will install Python dependencies and run tests with a variety of Python versions
# For more information see: https://docs.github.com/en/actions/automating-builds-and-tests/building-and-testing-python

name: Testing

on:
push:
branches: [ "*" ]
pull_request:
branches: [ "main" ]

jobs:
test:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- name: Set up Python
uses: actions/setup-python@v5
with:
python-version: |
3.8
3.9
3.10
3.11
3.12
3.13
- name: Install dependencies
uses: ./.github/actions/prepare-tox
- name: Run unit tests
run: |
tox -e py38,py39,py310,py311,py312,py313
5 changes: 3 additions & 2 deletions src/firewheel/tests/unit/cli/test_cli_completion.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
from pathlib import Path
from unittest.mock import Mock, patch, mock_open

from firewheel.config import config
from firewheel.cli.completion import COMPLETION_SCRIPT_PATH
from firewheel.cli.completion.actions import (
_keyboard_interruptable,
Expand Down Expand Up @@ -95,8 +96,8 @@ def test_populate_template(self):
mock_handle.write.assert_called_once()
script_content = mock_handle.write.call_args.args[0]
filled_placeholders = [
'fw_venv="/opt/firewheel/fwpy"',
'python_bin="python3"',
f"fw_venv=\"{config['python']['venv']}\"",
f"python_bin=\"{config['python']['bin']}\"",
]
assert all(_ in script_content for _ in filled_placeholders)

Expand Down
Loading