Add FailureInjector for in-app SDK failure simulation (1/3)#1698
Open
rahul-lohra wants to merge 6 commits into
Open
Add FailureInjector for in-app SDK failure simulation (1/3)#1698rahul-lohra wants to merge 6 commits into
rahul-lohra wants to merge 6 commits into
Conversation
WalkthroughThis PR introduces a debug failure injection system to the Stream Video SDK. The core framework defines a ChangesFailure Injection Framework & Demo Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/failureinjector/FailureInjector.kt (1)
21-37: ⚡ Quick winAdd KDoc for this public failure-injection contract.
FailureInjectorexposes behavior that is easy to misapply (throwDebugFaultvssendFailResult). Please document expected semantics on the interface/methods.As per coding guidelines, "Use KDoc (
/** ... */) for public APIs and complex subsystems; link to Stream docs when relevant".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/failureinjector/FailureInjector.kt` around lines 21 - 37, Add KDoc to the public FailureInjector interface documenting its intended semantics and usage: describe the interface purpose (failure-injection for tests/debugging), clarify the difference between throwDebugFault(FailureKey) (throws an exception synchronously for debugging) and sendFailResult(FailureKey) (returns an io.getstream.result.Result.Failure to be used as a failure value), explain lifecycle methods enable/disable/setEnabled/isEnabled/clear and thread-safety expectations, and include a link to the Stream docs and any examples or recommended usage patterns; apply the KDoc to the FailureInjector interface declaration and each public method name (enable, disable, setEnabled, isEnabled, clear, throwDebugFault, sendFailResult) so callers can’t misapply the API.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@demo-app/src/main/kotlin/io/getstream/video/android/App.kt`:
- Around line 87-90: injectFault() currently calls StreamVideo.instanceOrNull()
and silently does nothing if the SDK instance is not yet created; change it to
ensure the FailureInjectorImpl is attached even after a cold start by: if
StreamVideo.instanceOrNull() != null attach the injector to
StreamVideo.instanceOrNull()!!.state.failureInjector, otherwise persist the
FailureInjectorImpl (e.g., an App-level property) and attach it from the code
path that initializes StreamVideo (or subscribe to an SDK-initialized callback)
so that when StreamVideo is created you set state.failureInjector =
persistedFailureInjector; reference injectFault(), StreamVideo.instanceOrNull(),
FailureInjectorImpl, and state.failureInjector.
In
`@demo-app/src/main/kotlin/io/getstream/video/android/ui/FailureInjectorImpl.kt`:
- Around line 26-46: FailureInjectorImpl's enabledFaults uses mutableMapOf which
is not thread-safe; replace it with a thread-safe backing store (e.g.,
java.util.concurrent.ConcurrentHashMap or Collections.synchronizedMap) and keep
all access via the existing methods (enable, disable, setEnabled, isEnabled,
clear) so concurrent SDK/UI reads/writes can't race; ensure you update the
declaration of enabledFaults and leave the method signatures intact (no
global-scope coroutines) to maintain structured concurrency.
- Around line 51-55: The Retrofit Response.error call in FailureInjectorImpl
(the branch creating HttpException for FailureKey.FAIL_LOCATION) is using an
illegal status code 100 which causes Response.error to throw
IllegalArgumentException; change the status code to a value >= 400 (for example
500) while keeping the existing okhttp3.ResponseBody.create(null, "") so
Response.error<String>(500, ...) successfully constructs the error response and
the HttpException is created.
In `@demo-app/src/main/kotlin/io/getstream/video/android/ui/FailureInjectorUi.kt`:
- Around line 79-109: Replace hardcoded UI strings in FailureInjectorUi
composable with localized resources: change the Text(...) call that sets text =
"Failure Injection", the Text(...) with text = "Clear all", and any hardcoded
contentDescription like "Close" (and the string at line ~174) to use
stringResource(R.string.<name>) instead; add corresponding entries (e.g.,
failure_injection_title, failure_injection_clear_all, failure_injection_close)
to strings.xml and reference them via stringResource in the composable so all UI
text is localized.
- Around line 54-68: The root Column in FailureInjectorUi ignores the passed-in
modifier because it starts its chain with Modifier instead of the function
parameter; fix by replacing the leading Modifier with the function parameter
`modifier` in the Column's modifier chain (i.e., use
`modifier.fillMaxSize().background(...)`), so parent-provided modifiers applied
to FailureInjectorUi will be honored.
In
`@demo-app/src/main/kotlin/io/getstream/video/android/ui/join/CallJoinScreen.kt`:
- Around line 216-217: Remove the empty conditional block "if
(renderNetworkSettingsUi) { }" in CallJoinScreen (or, if it's intended as a
placeholder, replace the empty body with a clear TODO comment such as "// TODO:
render network settings UI here" so the intent is explicit); ensure you touch
the occurrence of the variable renderNetworkSettingsUi in CallJoinScreen.kt and
do not leave a no-op if statement lingering in the UI code.
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/socket/sfu/SfuSocket.kt`:
- Around line 112-113: debugFailureInjectors can throw before the socket is
created so update connectUser to catch any exception thrown by
debugFailureInjectors and route it into the SfuSocket state machine (use the
existing sfuSocketStateService error/transition API, e.g. call the error
transition method with the caught exception) instead of letting it escape; after
sending the error transition return early to avoid continuing socket creation.
Apply the same try/catch+state-transition+return pattern to the other
debugFailureInjectors call site(s) in this class (the second block around the
streamWebSocket setup) so all injected faults consistently trigger
sfuSocketStateService error transitions rather than uncaught throws.
---
Nitpick comments:
In
`@stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/failureinjector/FailureInjector.kt`:
- Around line 21-37: Add KDoc to the public FailureInjector interface
documenting its intended semantics and usage: describe the interface purpose
(failure-injection for tests/debugging), clarify the difference between
throwDebugFault(FailureKey) (throws an exception synchronously for debugging)
and sendFailResult(FailureKey) (returns an io.getstream.result.Result.Failure to
be used as a failure value), explain lifecycle methods
enable/disable/setEnabled/isEnabled/clear and thread-safety expectations, and
include a link to the Stream docs and any examples or recommended usage
patterns; apply the KDoc to the FailureInjector interface declaration and each
public method name (enable, disable, setEnabled, isEnabled, clear,
throwDebugFault, sendFailResult) so callers can’t misapply the API.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1d8a3d82-1166-499a-8395-d0bbc668c3c5
📒 Files selected for processing (12)
demo-app/src/main/kotlin/io/getstream/video/android/App.ktdemo-app/src/main/kotlin/io/getstream/video/android/ui/FailureInjectorImpl.ktdemo-app/src/main/kotlin/io/getstream/video/android/ui/FailureInjectorUi.ktdemo-app/src/main/kotlin/io/getstream/video/android/ui/join/CallJoinScreen.ktdemo-app/src/main/res/values/strings.xmlstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/Call.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/ClientState.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/StreamVideoClient.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/failureinjector/FailureInjector.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/failureinjector/FailureKey.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/failureinjector/NoOpFailureInjector.ktstream-video-android-core/src/main/kotlin/io/getstream/video/android/core/socket/sfu/SfuSocket.kt
rahul-lohra
changed the title
rahul-lohra
changed the title
rahul-lohra
changed the title
Contributor
|
We need to revise this approach, I fear having a testing code in the SDK is not the best approach. We need to find alternatives. |
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
2 participants
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.
Goal
Introduces a
FailureInjectorsubsystem to the Stream Video Android SDK, allowing engineers to simulate specific SDK failure scenarios at runtime directly from the demo app — without network proxies or source code changes.Implementation
enable,disable,setEnabled,isEnabled,clear,throwDebugFault, andsendFailResultisEnabledalways returnsfalse, resulting in zero production overheadvar failureInjector: FailureInjector, defaulting toNoOpFailureInjectorjoin()withFAIL_JOIN_CALLFAIL_LOCATIONFAIL_WS_CONNECT,FAIL_FAST_RECONNECT,FAIL_FULL_REJOIN,FAIL_MIGRATE🎨 UI Changes
Testing
FAIL_JOIN_CALL— verifycall.join()returns an errorFAIL_LOCATION— verify location routing fails gracefullyFAIL_WS_CONNECT— verify WS connection error is handledFAIL_FAST_RECONNECT/FAIL_FULL_REJOIN/FAIL_MIGRATE—verify reconnect fallback behaviour
Summary by CodeRabbit