Skip to content

test(ci): check that a finished meeting keeps its recording with retention off - #977

Open
pasrom wants to merge 1 commit into
mainfrom
test/e2e-retention-default-keeps-audio
Open

pasrom wants to merge 1 commit into
mainfrom
test/e2e-retention-default-keeps-audio

Conversation

@pasrom

@pasrom pasrom commented Nov 12, 2026

Copy link
Copy Markdown
Owner

What changed and why

Follow-up to #960 (recording retention, issue #723). Unit tests pin the default (off) and the policy, but nothing checked the production chain: the retention pass that runs when a job reaches done, against the app's real settings and output folder.

  • Every lane that goes through run_one_meeting (default, --two-meetings, --title-source, --mic-device-change) now checks that the job's relocated recordings/<stem>_mix.wav is there right after done and still there 5 s later. The second look is needed because the pass runs on its own just after the state change, and with the switch off it logs nothing the lane could wait for.
  • Under the default period of 30 days a recording minutes old is never due, so that check alone would stay green with the switch ignored. In CI the app is therefore launched with -recordingRetentionDays <integer>0</integer> as a launch argument, so nothing is stored or has to be restored. The switch is then all that keeps the recording.
  • The 0-day period is applied only in CI ($GITHUB_ACTIONS) and only when the host has not switched retention on. With 0 days a broken switch would delete every listed recording in the host's output folder, and on a developer's Mac those are their own recordings.
  • A host with retention switched on skips the check and logs that it did. "On" follows UserDefaults.flag: yes or true in any case, or any integer but 0.
  • The verdict is retention_default_verdict in scripts/lib/e2e-helpers.sh, pinned by scripts/tests/test_retention_default_verdict.sh (runs in about a second in the existing script-test step).

Tests

  • Script test, 18 checks, run locally and in the existing CI step. Removing the second look (always reporting kept) makes deleted_after_done_is_caught fail.
  • bash -n and shellcheck -S warning are clean on the changed files, and every scripts/tests/test_*.sh passes locally.
  • The lane itself runs on this PR through the run-e2e label. What to look for in the e2e-app log: CI: recording retention period 0 days for this run and recording still in ... 5s after done.
  • Not checked: that the lane goes red with the switch broken. That would need a deliberately broken build on the runner. The unit test for the switch covers that direction.

Known limits, left as they are:

  • A pass that starts more than 5 s after done goes unseen.
  • The mix path comes from the transcript's stem. The lanes keep the default file names, so this holds for them.
  • The --mic-stall lane does not go through run_one_meeting and is not covered.

AI assistance

  • Written with: Claude (Opus) in Claude Code.
  • Reviewed with: Claude (Opus), one cleanup pass and one correctness pass. The correctness pass found that the first version could not fail under the 30-day default, which led to the 0-day period in CI.

Checklist

  • Rebased on main, no merge commits
  • Script tests pass, shellcheck clean on the changed files
  • Conventional Commits, one logical change per commit

…ntion off

Recording retention (issue #723) deletes relocated audio only once the user
switches it on. Unit tests pin the default and the policy, but nothing checked
the production chain: the retention pass that runs when a job reaches done,
against the app's real settings and output folder. Every lane that goes
through run_one_meeting now looks at the job's relocated _mix.wav right after
done and again five seconds later. The pass runs on its own just after the
state change and, switched off, logs nothing to wait for, so a single look
could race it.

Under the default period of 30 days a recording minutes old is never due, so
that check alone would stay green with the switch ignored. In CI the app is
therefore launched with a period of 0 days, as a launch argument so nothing is
stored: the switch is then all that keeps the recording. Only in CI and only
with the switch not on, because with 0 days a broken switch would delete every
listed recording in the host's output folder, which on a developer's Mac is
their own.

A host that has switched retention on skips the check and says so instead of
failing. "On" follows the app's own reading: yes or true in any case, or any
integer but 0.

The verdict lives in e2e-helpers.sh so a script test pins it in both
directions; dropping the second look turns the deleted case green and the
test catches that.
@pasrom pasrom added the run-e2e Run E2E lanes (fixture + live Mac mini) on this PR label Nov 12, 2026
@github-actions github-actions Bot added the chore Maintenance or non-functional changes label Nov 12, 2026
@codecov

codecov Bot commented Nov 12, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.1%. Comparing base (f5ba602) to head (ed934d2).

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff           @@
##            main    #977    +/-   ##
======================================
  Coverage   92.1%   92.1%            
======================================
  Files        322     322            
  Lines      24795   25374   +579     
  Branches       6       6            
======================================
+ Hits       22838   23385   +547     
- Misses      1953    1985    +32     
  Partials       4       4            
Flag Coverage Δ
appstore 92.3% <ø> (-0.2%) ⬇️
audiotap 92.5% <ø> (+0.4%) ⬆️
homebrew 92.1% <ø> (+<0.1%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 56 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

chore Maintenance or non-functional changes run-e2e Run E2E lanes (fixture + live Mac mini) on this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant