Revert AV sync assertion weakening#92
Conversation
Co-authored-by: chrisgleissner <3969147+chrisgleissner@users.noreply.github.com>
d8037fc
into
fix/e2e-avsync-pop-detection
There was a problem hiding this comment.
Pull request overview
This PR reverts a previous change that weakened the AV sync assertion by converting "no matched A/V pop pairs" cases from failures to warnings. The revert restores strict behavior where all AV sync issues fail hard, relying on PR #90's concurrency fix to resolve the underlying CI instability.
Changes:
- Removed special case handling for
total_analyzed == 0that treated it as a warning - Restored strict assertion behavior where
passed=Falsealways results in test failure - Simplified message formatting by removing conditional logic based on
total_analyzed
Comments suppressed due to low confidence (1)
tests/e2e/framework/validation/av_sync.py:4
- Import of 'Optional' is not used.
from typing import Tuple, Optional, Dict, Any
| if not passed: | ||
| total_analyzed = results.get('total_analyzed', 0) | ||
| perfect_count = results.get('perfect_sync_count', 0) | ||
| msg += f" ({total_analyzed - perfect_count} mismatches)" |
There was a problem hiding this comment.
When total_analyzed == 0, this code will show 'Accuracy: 0.0% (0 mismatches)' which is misleading. The results.py file at lines 362-365 handles this case by treating it as a warning with a more descriptive message. However, after this revert, av_sync.py will return passed=False when total_analyzed == 0 (since sync_accuracy == 0 means is_perfectly_synced=False), but the message won't explain why it failed. The removed code provided a clear message: 'No matched A/V pop pairs (audio_pops=X, video_pops=Y)'. Consider whether the strict behavior should include a more descriptive message for the zero-analysis case, or verify that results.py's handling at line 362 will catch this before it becomes an error.
| msg += f" ({total_analyzed - perfect_count} mismatches)" | |
| if total_analyzed == 0: | |
| audio_pops = results.get('audio_pops', 0) | |
| video_pops = results.get('video_pops', 0) | |
| msg = ( | |
| f"No matched A/V pop pairs " | |
| f"(audio_pops={audio_pops}, video_pops={video_pops})" | |
| ) | |
| else: | |
| mismatches = max(total_analyzed - perfect_count, 0) | |
| msg += f" ({mismatches} mismatches)" |
The original PR weakened the AV sync assertion to treat "no matched A/V pop pairs" as a warning instead of a failure. This change reverts that weakening.
Rationale
The concurrency fix in PR #90 should have resolved the underlying CI instability. Only two tests were affected, and the proper fix is addressing the race condition, not weakening the assertion.
Changes
tests/e2e/framework/validation/av_sync.pyto strict behaviortotal_analyzed == 0Before (weakened)
After (strict)
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.