Skip to content

Fix #10193: harden SoftNI regression tests (review follow-up) - #13244

Merged
niksedk merged 1 commit into
SubtitleEdit:mainfrom
Ironship:followup/10193-test-hardening
Aug 5, 2026
Merged

Fix #10193: harden SoftNI regression tests (review follow-up)#13244
niksedk merged 1 commit into
SubtitleEdit:mainfrom
Ironship:followup/10193-test-hardening

Conversation

@Ironship

@Ironship Ironship commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Problem

Follow-up to merged PR #13241 (issue #10193, SoftNI sub export frame rate). Copilot review on #13241 noted that the regression tests index lines[timingIndex + 1] right after asserting the TIMING header exists, without checking that a following line actually exists — if the output is truncated or TIMING is the last line, the test would throw IndexOutOfRangeException instead of failing with a clear assertion message. The comment applies at two sites (line 31 and line 51 of tests/libse/SubtitleFormats/SoftNiSubTest.cs).

Fix

Added a bounds assertion before indexing in both SoftNI tests:
Assert.True(timingIndex + 1 < lines.Count, "Missing timing line after *TIMING* section");

Test-only change; no production code touched.

Verification

  • dotnet build tests/libse/LibSETests.csproj — EXIT:0, 0 errors
  • dotnet test tests/libse/LibSETests.csproj --filter FullyQualifiedName~SoftNiSubTest — Passed: 4, Failed: 0
  • Diff limited to 2 added assertion lines

Notes

Copilot AI lite review requested due to automatic review settings August 5, 2026 13:29
@Ironship

Ironship commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

/copilot-review

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Ready to approve

The change is a small, test-only hardening that prevents an out-of-range crash and aligns exactly with the stated follow-up goal.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

This PR hardens the SoftNI regression tests added for issue #10193 by ensuring the tests fail with a clear assertion message (instead of throwing IndexOutOfRangeException) if the exported output is truncated or *TIMING* is the last line.

Changes:

  • Add a bounds assertion (timingIndex + 1 < lines.Count) before indexing the timing line in SoftNiSub test.
  • Add the same bounds assertion in the SoftNiColonSub test.
File summaries
File Description
tests/libse/SubtitleFormats/SoftNiSubTest.cs Adds explicit bounds assertions before indexing the line following *TIMING*, improving test failure clarity and robustness.
Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 0
  • Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

@niksedk
niksedk merged commit 7a189c4 into SubtitleEdit:main Aug 5, 2026
1 check failed
@Ironship

Ironship commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

CI note: the test job failure is a pre-existing Avalonia headless flake, not a regression from this PR:

  • Failing test: UITests.Features.Shared.TextBoxTagTogglerTests.ToggleTag_SelectedItalicWord_RemovesItalic — a Test Case Cleanup Failure (System.InvalidOperationException: The calling thread cannot access this object because a different thread owns it), i.e. thread-affinity in teardown.
  • The test is unrelated to this PR (test-only hardening of SoftNiSubTest); LibSE tests all passed (859/859, incl. the 4 SoftNI tests).
  • The same flake class hits a different test on every full-suite run (locally: SubtitleGridScrollPerformanceTests.HomeAndEnd_RealizeOnlyAViewportOfRows failed in two consecutive full runs while passing in isolation; here CI failed TextBoxTagTogglerTests, which passes 5/5 in isolation on this exact branch).
  • Different test each run + teardown thread-affinity exception + passes in isolation = infrastructure flake, not a code issue.

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.

3 participants