Skip to content

TST: harden flaky VTK flight animation tests (#1078) - #1127

Closed
thatrandomasiandev wants to merge 1 commit into
RocketPy-Team:developfrom
thatrandomasiandev:tst/1078-vtk-animation-flake
Closed

TST: harden flaky VTK flight animation tests (#1078)#1127
thatrandomasiandev wants to merge 1 commit into
RocketPy-Team:developfrom
thatrandomasiandev:tst/1078-vtk-animation-flake

Conversation

@thatrandomasiandev

Copy link
Copy Markdown

Summary

  • Mark the three PyVista/VTK flight animation tests with flaky_vtk and register the marker in pyproject.toml / tests/conftest.py.
  • Drive CI isolation with -m "not flaky_vtk" / -m flaky_vtk instead of hard-coded --deselect paths; bump native-crash retries from 3 to 5.
  • Soft-skip test_flight_animation_export_gif only on OSError when off-screen rendering is unavailable; assertion failures still fail the test.

develop already isolates these tests from the main integration job and retries SIGBUS/SIGSEGV (135/138/139). This PR makes that flake explicit via a marker and a slightly safer export path.

Fixes #1078

Test plan

  • PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 pytest tests/integration/test_plots.py -m flaky_vtk --collect-only collects exactly the three animation tests
  • -m "not flaky_vtk" deselects those three and keeps the compare tests
  • CI Pytest matrix: integration step skips flaky_vtk; VTK step retries up to 5 times on 135/138/139

@thatrandomasiandev
thatrandomasiandev requested a review from a team as a code owner August 11, 2026 01:36
@codecov

codecov Bot commented Aug 12, 2026

Copy link
Copy Markdown

Codecov Report

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

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1127      +/-   ##
===========================================
+ Coverage    82.18%   82.51%   +0.33%     
===========================================
  Files          122      128       +6     
  Lines        16355    16617     +262     
===========================================
+ Hits         13441    13712     +271     
+ Misses        2914     2905       -9     

☔ 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

Copy link
Copy Markdown
Member

Thanks for picking this up, @thatrandomasiandev — and sorry for the outcome, this one is timing rather than the work itself.

Closing as not needed: the crash #1078 describes is already fixed on develop, by two PRs that landed after the issue was opened.

@thc1006, who reported it, confirmed on 2026-08-08 that macOS 3.14 has been green since, on a leg that had been crashing all week.

Two things in the diff also argue against merging it as it stands.

The try/except OSError cannot address this flake, and it hides real failures. SIGBUS/SIGSEGV kills the interpreter, so no Python except ever runs — it does nothing for #1078. What it does do is turn genuine errors into green skips: _validate_animation_inputs raises FileNotFoundError (a subclass of OSError) at rocketpy/plots/flight_plots.py:179 when the 3D model file is missing, and that STL ships via [tool.setuptools.package-data] while CI runs pip install .. A packaging regression would skip quietly instead of failing the job — exactly what this integration test exists to catch. Same for imageio's open_gif and PIL's save on a write error.

The --deselect → marker move is only half applied. test-pytest-slow.yaml still carries the same hard-coded list of nodeids, so the repo would be left running both conventions at once.

That said, the marker itself is a genuine improvement and stands on its own merits. I checked the assumption behind it: --deselect with a nodeid that does not exist is silently ignored — pytest collects everything, with no warning at all. So renaming any of those three tests today would quietly put the flake back into the main integration job. If you would like to send that part as a standalone maintenance PR, it would be welcome:

  • the marker only, no change to the test body;
  • applied to both test_pytest.yaml and test-pytest-slow.yaml;
  • registered in one place, not in both pyproject.toml and tests/conftest.py (the latter already registers slow, so that entry would be duplicated);
  • attempts left at 3 — there is no evidence 3 was insufficient, and the data points the other way since CI: force software rendering to fix flaky VTK off-screen bus error (#1078) #1084;
  • pytest tests/integration/test_plots.py -m flaky_vtk rather than collecting the whole integration package to run three tests.

Framed as CI maintenance, not as a fix for #1078. Thanks again for the time you put into it.

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