Skip to content

MNT: do not let one Python version cancel the other in the slow matrix - #1100

Merged
Gui-FernandesBR merged 3 commits into
RocketPy-Team:developfrom
thc1006:mnt/pytest-matrix-fail-fast
Aug 8, 2026
Merged

MNT: do not let one Python version cancel the other in the slow matrix#1100
Gui-FernandesBR merged 3 commits into
RocketPy-Team:developfrom
thc1006:mnt/pytest-matrix-fail-fast

Conversation

@thc1006

@thc1006 thc1006 commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Follows from #1078. Rescoped after #1084 landed, see below.

Pull request type

  • Code maintenance (refactoring, formatting, tests)

Current behavior

test-pytest-slow.yaml runs 3.10 and 3.14 as a matrix and sets no fail-fast, so the default of true applies and the first version to fail cancels the other. The cancelled job then reports as a failure without having finished.

New behavior

      fail-fast: false

3.10 failing tells you nothing about 3.14, so cancelling it costs a result and saves nothing worth having.

Breaking change

  • No

Runner time on a failing slow run goes from "stops at the first failure" to "both finish". The slow workflow is not on pull requests, so this is not in anyone's review loop.

What changed since the approval

@Gui-FernandesBR approved this when it also set fail-fast: false on the main Pytest matrix. #1084 merged in the meantime and added exactly that, so that half is now redundant and I have dropped it. What is left is three lines on the slow matrix.

I also dropped the comment I had put above the main matrix setting. It referenced #1078 as a live problem, which #1084 has since fixed, and annotating someone else's line from a different PR seemed like the wrong place for it.

So this is smaller than what you looked at, and entirely a subset of it. Re-approving or closing are both reasonable; I would not want it merged on the strength of a review of the larger version.

@thc1006

thc1006 commented Aug 8, 2026

Copy link
Copy Markdown
Contributor Author

This PR ran against itself, so here is the before and after on the same crash, an hour apart.

#1098, on develop's current settings:

macos-latest, 3.14    failure     the #1078 crash
macos-latest, 3.10    cancelled
ubuntu-latest, 3.10   cancelled
ubuntu-latest, 3.14   cancelled
windows-latest, 3.10  cancelled
windows-latest, 3.14  cancelled

This branch, same crash on the same job:

macos-latest, 3.14    failure     the #1078 crash
macos-latest, 3.10    success
ubuntu-latest, 3.14   success
windows-latest, 3.10  success
windows-latest, 3.14  success
ubuntu-latest, 3.10   still running at the time of writing

One more thing I had not noticed until I went looking at #1098 this morning. gh pr checks prints cancelled jobs as fail:

Pytest (ubuntu-latest, 3.10)   fail   2m49s

The API says cancelled and the annotation says "The strategy configuration was canceled because Pytest.macos-latest_3_14 failed", but neither is what you see first. I read that output and briefly thought I had broken something on six platforms, which is the failure mode this PR is about rather than an argument for it.

@Gui-FernandesBR Gui-FernandesBR left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

the fail fast exists to save machine time.

however, we had never readly faced a limit on our github actions. So I'm ok with proceeding with this PR, let's make it

@Gui-FernandesBR

Copy link
Copy Markdown
Member

Heads up on both red marks here — neither is anything you did, and the fix is a rebase.

The conflict. #1084 landed about forty minutes after this run started, and it already carries fail-fast: false for test_pytest.yaml. On current develop:

.github/workflows/test_pytest.yaml:20:      fail-fast: false
.github/workflows/test-pytest-slow.yaml:      (absent)

So half of this is already in, which is what made the PR go CONFLICTING. Resolving it means dropping the test_pytest.yaml hunk and keeping the test-pytest-slow.yaml one. The PR does not become empty — test-pytest-slow.yaml is the part #1084 missed, and your explanatory comments are worth keeping either way.

The CI failure. It is #1078, and the run predates the fix:

run created  2026-08-08T01:08:59Z
#1084 merged 2026-08-08T01:51:06Z
Pytest (macos-latest, 3.14) | Run Integration Tests
Fatal Python error: Segmentation fault
pytest tests/integration --cov=rocketpy --cov-append
##[error]Process completed with exit code 139

That is the un-isolated Run Integration Tests step, which #1084 replaced with a separate step for the three VTK animation tests. Once you rebase, this should go green — #1085 was in exactly the same position, had never had a green run, and went 6/6 immediately after picking up #1084.

Worth noting your own change did its job in that failing run: the other five legs ran to completion and reported real results instead of being cancelled. The PR that fixes cancellation got taken down by the very crash that motivated it, and only the crashing leg went red.

One thing from my #1084 review that is relevant to you, since you have the measurements: the retry there keys on exit code 138 only, which is SIGBUS on macOS. Your tally of 8 SIGSEGV to 2 SIGBUS means the common case (139) falls straight through it, and on Linux SIGBUS is 135 rather than 138. If you want to fold case "$status" in 135|138|139) into this PR while you are in these two files, that would close the gap — otherwise I am happy to take it separately.

RocketPy-Team#1084 added fail-fast: false to the main test matrix while this was open, so
the only half left is the slow one. Same reason: 3.10 failing says nothing
about 3.14, so cancelling it costs a result and saves nothing worth having.

Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
@thc1006
thc1006 force-pushed the mnt/pytest-matrix-fail-fast branch from 1bb8f23 to 806a0d7 Compare August 8, 2026 02:05
@thc1006 thc1006 changed the title MNT: do not let one platform cancel the rest of the test matrix MNT: do not let one Python version cancel the other in the slow matrix Aug 8, 2026
RocketPy-Team#1084 retries the animation tests when they die on 138, which is SIGBUS on
macOS. Counting the last 40 Tests runs, the crash was 139 eight times and 138
twice, so the common case fell straight through the retry. Linux SIGBUS is 135
rather than 138, so that missed as well.

Also sets fail-fast: false on the slow matrix, which RocketPy-Team#1084 left out. 3.10
failing says nothing about 3.14, so cancelling it costs a result and saves
nothing worth having.

Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
@thc1006

thc1006 commented Aug 8, 2026

Copy link
Copy Markdown
Contributor Author

Taken, thanks. Both in as 67c002d.

You are right about the exit codes and it is worse than a rounding error. Counting the last 40 Tests runs, the crash was 139 eight times and 138 twice, so keying on 138 alone let four out of five through. Linux SIGBUS is 135, so that missed too:

Linux   SIGBUS  = 7  -> 135
macOS   SIGBUS  = 10 -> 138
both    SIGSEGV = 11 -> 139

The condition is now a match against all three, in both workflow files:

if [[ ! "$status" =~ ^(135|138|139)$ || "$attempt" == "$attempts" ]]; then
  exit "$status"
fi

I checked the branch rather than the regex by eye, since an anchor slip here would silently retry everything:

status  first attempt   last attempt
135     retry           exit 135
138     retry           exit 138
139     retry           exit 139
1       exit 1          exit 1
1350    exit 1350       exit 1350

I had dropped the fail-fast comment on test_pytest.yaml before reading your note, on the reasoning that annotating a line from someone else's PR was not my place. Put back, since you would rather have it, and reworded to credit #1084 and say why it is written down.

So this is now three things: fail-fast: false on the slow matrix, the comment on the main one, and the retry fix across both.

One thing worth flagging before this goes in: what you approved was a different patch. At the time it set fail-fast: false on the main matrix, which #1084 has since done, and it did not touch the retry at all. So please give it another look rather than merging on the earlier approval.

@codecov

codecov Bot commented Aug 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.53%. Comparing base (e0ff281) to head (acf350b).
⚠️ Report is 20 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1100      +/-   ##
===========================================
+ Coverage    82.18%   82.53%   +0.34%     
===========================================
  Files          122      128       +6     
  Lines        16355    16556     +201     
===========================================
+ Hits         13441    13664     +223     
+ Misses        2914     2892      -22     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Gui-FernandesBR
Gui-FernandesBR merged commit 3656d2a into RocketPy-Team:develop Aug 8, 2026
8 checks passed
@thc1006
thc1006 deleted the mnt/pytest-matrix-fail-fast branch August 8, 2026 02:23
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