Fix flaky StreamVideo tests#1118
Draft
ipavlidakis wants to merge 2 commits intodevelopfrom
Draft
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis pull request removes one test case and refactors multiple test files to replace fixed-delay waits with event-driven assertion patterns. Changes eliminate timing dependencies and improve test reliability through eventual assertions and async helpers across integration, unit, and WebRTC test suites. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
ipavlidakis
commented
Apr 16, 2026
| ) | ||
| } | ||
|
|
||
| // MARK: - Duration |
Contributor
Author
There was a problem hiding this comment.
That same behavior is already covered directly in CallState_Tests.swift (line 224), CallState_Tests.swift (line 237), and CallState_Tests.swift (line 278).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
StreamVideoTests/WebRTC/v2/StateMachine/Stages/WebRTCCoordinatorStateMachine_DisconnectedStageTests.swift (1)
396-409:⚠️ Potential issue | 🟠 MajorWrong-stage transitions are treated as success here.
Line 407 fulfills the non-inverted expectation after a mismatched target without recording a failure, and Line 396 disables over-fulfill checks. That means this helper can pass when the state machine lands on the wrong stage, or when it reaches the expected stage and then immediately transitions somewhere unexpected.
🛠️ Suggested fix
subject.transition = { target in Task { if target.id == expectedTarget { await validationHandler(target) transitionExpectation.fulfill() } else if let expectedTarget { - transitionExpectation - .expectationDescription = - "Expectation to land on id:\(expectedTarget) but instead landed on id:\(target.id)." + XCTFail( + "Expected transition to \(expectedTarget), but landed on \(target.id).", + file: file, + line: line + ) transitionExpectation.fulfill() } else if expectedTarget == nil { transitionExpectation.fulfill() } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@StreamVideoTests/WebRTC/v2/StateMachine/Stages/WebRTCCoordinatorStateMachine_DisconnectedStageTests.swift` around lines 396 - 409, The helper currently disables over-fulfill checks (transitionExpectation.assertForOverFulfill = false) and fulfills the non-inverted expectation even on mismatched targets inside subject.transition, allowing wrong-stage transitions to be treated as success; restore/assert over-fulfill behavior (remove or set assertForOverFulfill = true) and change the subject.transition closure so it only fulfills transitionExpectation when target.id == expectedTarget after running validationHandler, and on a mismatched target (else branch) record a test failure (e.g., XCTFail with a message using expectedTarget and target.id) or fulfill a separate inverted expectation instead of fulfilling the success expectation; keep the expectedTarget == nil branch behavior consistent (fulfill only when that case is genuinely expected).StreamVideoTests/WebRTC/v2/StateMachine/Stages/WebRTCCoordinatorStateMachine_FastReconnectingStageTests.swift (1)
288-301:⚠️ Potential issue | 🟠 MajorUnexpected transitions still satisfy the expectation.
Line 299 fulfills the non-inverted expectation even when
target.id != expectedTarget, and Line 288 suppresses over-fulfill failures. So this helper can now pass on the wrong transition instead of failing fast.🛠️ Suggested fix
subject.transition = { target in Task { if target.id == expectedTarget { await validationHandler(target) transitionExpectation.fulfill() } else if let expectedTarget { - transitionExpectation - .expectationDescription = - "Expectation to land on id:\(expectedTarget) but instead landed on id:\(target.id)." + XCTFail( + "Expected transition to \(expectedTarget), but landed on \(target.id).", + file: file, + line: line + ) transitionExpectation.fulfill() } else if expectedTarget == nil { transitionExpectation.fulfill() } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@StreamVideoTests/WebRTC/v2/StateMachine/Stages/WebRTCCoordinatorStateMachine_FastReconnectingStageTests.swift` around lines 288 - 301, The helper currently allows unexpected transitions to satisfy the expectation because transitionExpectation.assertForOverFulfill is disabled and the subject.transition closure fulfills the expectation even when target.id != expectedTarget; change it so only the matching target (when target.id == expectedTarget) calls transitionExpectation.fulfill and triggers validationHandler, while any non-matching transition should not fulfill the expectation but instead record a test failure (e.g., XCTFail with a descriptive message using expectedTarget and target.id) or immediately fail the test; also restore/assert assertForOverFulfill behavior (remove or set to true) so extra fulfills are flagged. Ensure you update the closure in subject.transition and references to expectedTarget/validationHandler accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@StreamVideoTests/Utils/Proximity/Policies/VideoProximityPolicy_Tests.swift`:
- Around line 141-151: The current assertState uses fulfilmentInMainActor which
returns as soon as the predicate is true, so zero-call expectations can pass
immediately and miss later changes; update assertState to handle the
expectedTimesCalledChangeVideoState == 0 case by first capturing the current
incomingVideoQualitySettings and timesCalledChangeVideoState (via
mockCall.state.incomingVideoQualitySettings and
mockCallController.timesCalled(.changeVideoState)), then wait a short grace
period (e.g. 100–200ms) on the main actor and assert that both values did not
change during that window; keep the existing behaviour for
expectedTimesCalledChangeVideoState > 0 (use fulfilmentInMainActor as before to
wait until the predicate becomes true).
---
Outside diff comments:
In
`@StreamVideoTests/WebRTC/v2/StateMachine/Stages/WebRTCCoordinatorStateMachine_DisconnectedStageTests.swift`:
- Around line 396-409: The helper currently disables over-fulfill checks
(transitionExpectation.assertForOverFulfill = false) and fulfills the
non-inverted expectation even on mismatched targets inside subject.transition,
allowing wrong-stage transitions to be treated as success; restore/assert
over-fulfill behavior (remove or set assertForOverFulfill = true) and change the
subject.transition closure so it only fulfills transitionExpectation when
target.id == expectedTarget after running validationHandler, and on a mismatched
target (else branch) record a test failure (e.g., XCTFail with a message using
expectedTarget and target.id) or fulfill a separate inverted expectation instead
of fulfilling the success expectation; keep the expectedTarget == nil branch
behavior consistent (fulfill only when that case is genuinely expected).
In
`@StreamVideoTests/WebRTC/v2/StateMachine/Stages/WebRTCCoordinatorStateMachine_FastReconnectingStageTests.swift`:
- Around line 288-301: The helper currently allows unexpected transitions to
satisfy the expectation because transitionExpectation.assertForOverFulfill is
disabled and the subject.transition closure fulfills the expectation even when
target.id != expectedTarget; change it so only the matching target (when
target.id == expectedTarget) calls transitionExpectation.fulfill and triggers
validationHandler, while any non-matching transition should not fulfill the
expectation but instead record a test failure (e.g., XCTFail with a descriptive
message using expectedTarget and target.id) or immediately fail the test; also
restore/assert assertForOverFulfill behavior (remove or set to true) so extra
fulfills are flagged. Ensure you update the closure in subject.transition and
references to expectedTarget/validationHandler accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ed514cbe-c5cd-4110-976a-2bb60e4760aa
📒 Files selected for processing (7)
StreamVideoTests/Call/Call_Tests.swiftStreamVideoTests/IntegrationTests/Call_IntegrationTests.swiftStreamVideoTests/TestUtils/XCTestCase+Wait.swiftStreamVideoTests/Utils/Proximity/Policies/VideoProximityPolicy_Tests.swiftStreamVideoTests/Utils/Timers/TimerPublisher_Tests.swiftStreamVideoTests/WebRTC/v2/StateMachine/Stages/WebRTCCoordinatorStateMachine_DisconnectedStageTests.swiftStreamVideoTests/WebRTC/v2/StateMachine/Stages/WebRTCCoordinatorStateMachine_FastReconnectingStageTests.swift
💤 Files with no reviewable changes (1)
- StreamVideoTests/Call/Call_Tests.swift
Comment on lines
+141
to
+151
| private func assertState( | ||
| incomingVideoQualitySettings expectedIncomingVideoQualitySettings: IncomingVideoQualitySettings, | ||
| timesCalledChangeVideoState expectedTimesCalledChangeVideoState: Int, | ||
| lastChangeVideoStateValue expectedLastChangeVideoStateValue: Bool? = nil, | ||
| file: StaticString = #filePath, | ||
| line: UInt = #line | ||
| ) async { | ||
| await fulfilmentInMainActor(timeout: 2, filePath: file, line: line) { | ||
| self.mockCall.state.incomingVideoQualitySettings == expectedIncomingVideoQualitySettings | ||
| && self.mockCallController.timesCalled(.changeVideoState) == expectedTimesCalledChangeVideoState | ||
| } |
There was a problem hiding this comment.
assertState no longer observes the “nothing happens” cases.
Line 148 uses fulfilmentInMainActor, which completes on the first successful predicate evaluation. For the zero-call scenarios in this file, that condition is often already true immediately, so the helper returns without any grace window and a delayed changeVideoState / state mutation would be missed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@StreamVideoTests/Utils/Proximity/Policies/VideoProximityPolicy_Tests.swift`
around lines 141 - 151, The current assertState uses fulfilmentInMainActor which
returns as soon as the predicate is true, so zero-call expectations can pass
immediately and miss later changes; update assertState to handle the
expectedTimesCalledChangeVideoState == 0 case by first capturing the current
incomingVideoQualitySettings and timesCalledChangeVideoState (via
mockCall.state.incomingVideoQualitySettings and
mockCallController.timesCalled(.changeVideoState)), then wait a short grace
period (e.g. 100–200ms) on the main actor and assert that both values did not
change during that window; keep the existing behaviour for
expectedTimesCalledChangeVideoState > 0 (use fulfilmentInMainActor as before to
wait until the predicate becomes true).
Generated by 🚫 Danger |
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Call_IntegrationTestsTask.sleepCall_TestsTesting
xcodebuild -project StreamVideo.xcodeproj -scheme StreamVideo -testPlan StreamVideo -destination 'platform=iOS Simulator,name=iPhone 17 Pro,OS=26.4' testxcodebuildSummary by CodeRabbit