Conversation
* Initial plan * Revert test.yml changes to keep only bumpversion.toml change Co-authored-by: HEROgold <21345384+HEROgold@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: HEROgold <21345384+HEROgold@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR attempts to fix the .bumpversion.toml configuration syntax and restructure the test workflow to use caching, but introduces critical issues in the workflow logic.
Changes:
- Fixed
.bumpversion.tomlsyntax from[tool.bumpversion.files]to[[tool.bumpversion.files]](array of tables) - Split the test workflow into two jobs:
check-cacheandtest, attempting to skip tests when cache is hit - Added Python 3.14 to the test matrix
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
.bumpversion.toml |
Correctly fixes TOML syntax from single table to array of tables notation |
.github/workflows/test.yml |
Restructures workflow with cache-checking job, but introduces broken logic and includes unreleased Python version |
| test: | ||
| runs-on: ubuntu-latest | ||
| needs: check-cache | ||
| if: needs.check-cache.outputs.cache-hit != 'true' |
There was a problem hiding this comment.
The condition needs.check-cache.outputs.cache-hit != 'true' will never work as expected because the check-cache job doesn't define any outputs. The cache step's output (steps.cache-pytest.outputs.cache-hit) is local to that job and cannot be accessed by other jobs via needs.check-cache.outputs.
To fix this, the check-cache job needs to explicitly define outputs that map to the step outputs. You should add an outputs: section to the check-cache job like:
check-cache:
runs-on: ubuntu-latest
outputs:
cache-hit: ${{ steps.cache-pytest.outputs.cache-hit }}However, there's a deeper architectural issue: GitHub Actions doesn't support matrix-based outputs in the way this workflow attempts to use them. When a job has a matrix strategy, you cannot reliably reference outputs from all matrix runs in a dependent job. Each matrix run creates a separate job instance, and only the outputs from one arbitrary instance would be available.
A better approach would be to keep the cache step within the test job itself, or remove the cache-checking logic entirely since the cache action already handles cache hits efficiently.
| test: | ||
| runs-on: ubuntu-latest | ||
| needs: check-cache | ||
| if: needs.check-cache.outputs.cache-hit != 'true' | ||
| strategy: | ||
| matrix: | ||
| python-version: ["3.10", "3.11", "3.12", "3.13", "3.14"] | ||
| fail-fast: false | ||
|
|
||
| steps: | ||
| - uses: actions/checkout@v4 | ||
|
|
||
| - name: Install uv | ||
| uses: astral-sh/setup-uv@v7 | ||
| with: |
There was a problem hiding this comment.
The workflow splits the original test job into two separate jobs (check-cache and test), but this creates several issues:
- The
check-cachejob runs a matrix strategy but doesn't do anything with the cache results - The approach of trying to skip the entire test job based on cache hits is problematic because:
- GitHub Actions cache is meant to speed up jobs, not skip them entirely
- The
.pytest_cachedirectory contains test metadata, not test results - Even with a cache hit, tests should still run to validate the current code
This architectural change appears to misunderstand the purpose of pytest caching. The .pytest_cache directory stores test collection and execution metadata to make subsequent test runs faster, but it doesn't mean tests can be safely skipped. Consider reverting to the original single-job structure where caching naturally speeds up test execution without skipping validation.
|
|
||
| jobs: | ||
| test: | ||
| check-cache: |
There was a problem hiding this comment.
The job name was changed from test to check-cache, but this may break dependent workflows. The coverage.yml workflow explicitly depends on a workflow named "Test" (line 5 of coverage.yml: workflows: ["Test"]).
While the workflow name remains "Test", downstream systems or other workflows might reference the job name test specifically. Additionally, the actual testing is now done in a job called test that may not run due to the problematic cache-hit condition, which could cause the coverage workflow to succeed even when tests haven't actually been executed.
No description provided.