Skip to content

test(export): clean temporary outputs on assertion failure - #1386

Merged
EtienneLescot merged 2 commits into
getopenscreen:mainfrom
songoao25:test/clean-export-fixtures
Oct 24, 2026
Merged

EtienneLescot merged 2 commits into
getopenscreen:mainfrom
songoao25:test/clean-export-fixtures

Conversation

@songoao25

@songoao25 songoao25 commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

Summary

If the export gap/origin tests panic, their explicit cleanup is skipped and generated files remain in the supplied fixture directory. Use an owned temporary-directory guard for the three tests added in #1379, including the frame-count probe. The guard removes its directory on normal return and unwinding. Add a regression that simulates a failed export assertion and verifies cleanup.

Test-only follow-up to the minor cleanup suggestion in #1379. No production code or dependencies change.

Related issue

Refs #1379, review #1379 (review)

Type of change

  • Refactor / maintenance

Release impact

  • No release note needed

Desktop impact

  • Not platform-specific

Testing

  • macOS 15.7.1 / Apple M2: OPENSCREEN_TEST_MEDIA=<synthetic-media> cargo test --manifest-path crates/Cargo.toml -p openscreen-compositor --test export_timing -- --test-threads=1 — 15 passed with all fixture media supplied, including the new unwinding regression.
  • git diff --check passed.
  • Clippy was attempted for this integration-test target; the library's 7 existing errors block it before the test target is checked. No lint rules were disabled.
  • Windows runtime was not exercised locally. Linux continues to exclude this existing integration-test file. Application code, native binaries and dependencies are unchanged, so desktop smoke tests and the JavaScript suite were not repeated for this test-only change.

Summary by CodeRabbit

  • Tests
    • Updated export timing tests to use uniquely named temporary output directories that are automatically cleaned up after normal completion or during panic unwinding. Cleanup failures are reported without causing a panic.

@coderabbitai

coderabbitai Bot commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: c0268ac6-9607-4c79-a849-72197dfcc9b3
Commits

Reviewing files that changed from the base of the PR and between 32f1093 and f2b23c0.

Files selected for processing: 1
  • crates/compositor/tests/export_timing.rs
Files skipped from review as they are similar to previous changes: 1
  • crates/compositor/tests/export_timing.rs

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 6 remain after this review.

Walkthrough

The export timing tests add a temporary output directory guard that removes its directory when it leaves scope. A new test checks cleanup during panic unwinding. Three existing tests use the guard for their output paths and cleanup.

Changes: Test output cleanup

Layer / File(s) Summary
Add temporary directory guard
crates/compositor/tests/export_timing.rs
Adds a uniquely named temporary directory helper and tests that it removes the directory during panic unwinding.
Use guard in export timing tests
crates/compositor/tests/export_timing.rs
Updates the gap, nonzero-timestamp, and frame-probe tests to use the helper for output paths and cleanup.

Priority: Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Other

Possibly related PRs

Merge Risk: Minimal · up to f2b23

The updated tests keep temporary outputs under cleanup ownership, including when a test panics. No cleanup issue needs resolution before merging.

Pre-merge checks: Passed 5
Check name Status Explanation
Title check Passed The title clearly and concisely describes the main change: cleaning temporary export outputs when assertions fail.
Description check Passed The description covers the change, related issue, change type, release impact, platform impact, and testing results. The omitted screenshots section is not relevant to this test-only change.
Docstring Coverage Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 1 files.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Warning

Some tools did not complete. Review the errors below.

🔧 Clippy (1.99.0)

Clippy execution failed

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@EtienneLescot
EtienneLescot merged commit d433e7c into getopenscreen:main Oct 24, 2026
21 checks passed
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