Increase Scrcpy/Agent test coverage for previously untested integration and error branches#49
Open
Increase Scrcpy/Agent test coverage for previously untested integration and error branches#49
Conversation
3 tasks
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| Go |  |
Apr 16, 2026 7:53p.m. | Review ↗ |
| Code coverage |  |
Apr 16, 2026 7:53p.m. | Review ↗ |
| Secrets | |
Apr 16, 2026 7:53p.m. | Review ↗ |
Important
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
… paths Agent-Logs-Url: https://github.com/uelkerd/vis/sessions/cd6de7d6-2a8d-40ff-a4dd-1bc8fed7dbf0 Co-authored-by: uelkerd <[email protected]>
Agent-Logs-Url: https://github.com/uelkerd/vis/sessions/cd6de7d6-2a8d-40ff-a4dd-1bc8fed7dbf0 Co-authored-by: uelkerd <[email protected]>
Copilot
AI
changed the title
[WIP] Add tests for missing coverage in Scrcpy integration
Increase Scrcpy/Agent test coverage for previously untested integration and error branches
Apr 16, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Adds targeted unit tests to increase branch/edge-case coverage in the Scrcpy livefeed integration and agent multi-device execution paths.
Changes:
- Extend
ScrcpyClientcontrol-path tests to cover additional error branches (deadline-setting failure, mid-swipe write failure, first-write failure, and input-action settle timeout). - Add agent tests covering multi-device error handling (device listing failure) and
runMultiDeviceInternalresult population (agent creation error + history assignment). - Minor test cleanups/whitespace normalization in existing test files.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| internal/livefeed/scrcpy_control_test.go | Adds new coverage-focused tests and enhances mockConn to simulate additional failure modes. |
| internal/livefeed/scrcpy_capturer_test.go | Minor comment/whitespace cleanup (no behavioral change). |
| internal/agent/agent_test.go | Adds multi-device and internal runner coverage tests; minor formatting/whitespace cleanup. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Owner
|
/gemini review |
Contributor
Reviewer's GuideAdds branch-focused tests to improve coverage of scrcpy control-path error handling and agent multi-device orchestration paths, plus minor test refactors and mock enhancements, without changing production behavior. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Some of the new timeout-based tests (e.g.,
executeInputActionusing a 20ms context timeout) may be brittle under slow CI; consider using a longer timeout with explicit cancellation or a controllable clock/timer to avoid flakiness. - The expanded
mockConnuses overlappingfailWriteandfailAtWritecontrols; it would be clearer to consolidate the failure configuration into a single mechanism or document the precedence to avoid confusion in future tests.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Some of the new timeout-based tests (e.g., `executeInputAction` using a 20ms context timeout) may be brittle under slow CI; consider using a longer timeout with explicit cancellation or a controllable clock/timer to avoid flakiness.
- The expanded `mockConn` uses overlapping `failWrite` and `failAtWrite` controls; it would be clearer to consolidate the failure configuration into a single mechanism or document the precedence to avoid confusion in future tests.
## Individual Comments
### Comment 1
<location path="internal/livefeed/scrcpy_control_test.go" line_range="958" />
<code_context>
})
}
+
+func TestScrcpyClient_executeInputAction_ErrorPaths_Coverage(t *testing.T) {
+ client := &ScrcpyClient{}
+ action := types.Action{Action: "input"}
</code_context>
<issue_to_address>
**suggestion (testing):** `executeInputAction` tests cover empty input, focus-tap failure, and settle-timeout, but they don't exercise the InjectText failure path.
To fully cover the error paths, consider also testing the case where the focus tap succeeds but `InjectText` fails (e.g., via a `ScrcpyClient` using `mockConn{failWrite: true}` and a non-nil `Target.Bounds`). Then assert that the returned error clearly reflects the injection failure, completing coverage of `executeInputAction`’s control-path errors.
Suggested implementation:
```golang
func TestScrcpyClient_executeInputAction_ErrorPaths_Coverage(t *testing.T) {
client := &ScrcpyClient{}
action := types.Action{Action: "input"}
// Empty input value should fail early.
err := client.executeInputAction(context.Background(), action)
if err == nil || !strings.Contains(err.Error(), "empty input value") {
t.Fatalf("expected empty input error, got %v", err)
}
// Non-empty value with bounds but no control connection should fail with a
// clear "control socket not connected" error.
action.Value = "hello"
action.Target = &types.Target{
Bounds: &types.Bounds{
X: 10,
Y: 10,
Width: 20,
Height: 20,
},
}
err = client.executeInputAction(context.Background(), action)
if err == nil || !strings.Contains(err.Error(), "control socket not connected") {
t.Fatalf("expected control socket not connected error, got %v", err)
}
// Now simulate a connected control socket where InjectText fails due to a
// write error. The focus tap should succeed, but text injection should fail.
client.control = &mockConn{failWrite: true}
err = client.executeInputAction(context.Background(), action)
if err == nil || !strings.Contains(err.Error(), "failed to inject text") {
t.Fatalf("expected inject text error, got %v", err)
}
```
Because only part of the function was visible in the snippet, you should:
1. Ensure there is no leftover body from the old `TestScrcpyClient_executeInputAction_ErrorPaths_Coverage` after this replacement (i.e., the function should close with the final `}` shown in the REPLACE block).
2. Confirm the actual error string used in `executeInputAction` for the InjectText failure path. If it differs from `"failed to inject text"`, update the substring in the final `strings.Contains` check to match the real message.
3. Verify that `mockConn` is already defined in this test file with a `failWrite` field that triggers a write failure on the control socket, consistent with the existing InjectTap write-failure tests. If the field name or behavior differs, adjust `&mockConn{failWrite: true}` accordingly.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| }) | ||
| } | ||
|
|
||
| func TestScrcpyClient_executeInputAction_ErrorPaths_Coverage(t *testing.T) { |
Contributor
There was a problem hiding this comment.
suggestion (testing): executeInputAction tests cover empty input, focus-tap failure, and settle-timeout, but they don't exercise the InjectText failure path.
To fully cover the error paths, consider also testing the case where the focus tap succeeds but InjectText fails (e.g., via a ScrcpyClient using mockConn{failWrite: true} and a non-nil Target.Bounds). Then assert that the returned error clearly reflects the injection failure, completing coverage of executeInputAction’s control-path errors.
Suggested implementation:
func TestScrcpyClient_executeInputAction_ErrorPaths_Coverage(t *testing.T) {
client := &ScrcpyClient{}
action := types.Action{Action: "input"}
// Empty input value should fail early.
err := client.executeInputAction(context.Background(), action)
if err == nil || !strings.Contains(err.Error(), "empty input value") {
t.Fatalf("expected empty input error, got %v", err)
}
// Non-empty value with bounds but no control connection should fail with a
// clear "control socket not connected" error.
action.Value = "hello"
action.Target = &types.Target{
Bounds: &types.Bounds{
X: 10,
Y: 10,
Width: 20,
Height: 20,
},
}
err = client.executeInputAction(context.Background(), action)
if err == nil || !strings.Contains(err.Error(), "control socket not connected") {
t.Fatalf("expected control socket not connected error, got %v", err)
}
// Now simulate a connected control socket where InjectText fails due to a
// write error. The focus tap should succeed, but text injection should fail.
client.control = &mockConn{failWrite: true}
err = client.executeInputAction(context.Background(), action)
if err == nil || !strings.Contains(err.Error(), "failed to inject text") {
t.Fatalf("expected inject text error, got %v", err)
}Because only part of the function was visible in the snippet, you should:
- Ensure there is no leftover body from the old
TestScrcpyClient_executeInputAction_ErrorPaths_Coverageafter this replacement (i.e., the function should close with the final}shown in the REPLACE block). - Confirm the actual error string used in
executeInputActionfor the InjectText failure path. If it differs from"failed to inject text", update the substring in the finalstrings.Containscheck to match the real message. - Verify that
mockConnis already defined in this test file with afailWritefield that triggers a write failure on the control socket, consistent with the existing InjectTap write-failure tests. If the field name or behavior differs, adjust&mockConn{failWrite: true}accordingly.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request focuses on improving test coverage by adding new test cases for error paths in the agent and scrcpy control logic. It also includes widespread whitespace and formatting cleanup across several test files. The feedback suggests refining test assertions in the new coverage tests to provide clearer failure messages by separating result count checks from content validation.
…ailures Agent-Logs-Url: https://github.com/uelkerd/vis/sessions/a1052216-e58b-4f2c-b47c-ce288a949e81 Co-authored-by: uelkerd <[email protected]>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Agent-Logs-Url: https://github.com/uelkerd/vis/sessions/164a5522-8404-439d-82de-4521a64fc1a0 Co-authored-by: uelkerd <[email protected]>
Agent-Logs-Url: https://github.com/uelkerd/vis/sessions/604a3a94-529f-4368-a825-05b310c4b9c7 Co-authored-by: uelkerd <[email protected]>
Agent-Logs-Url: https://github.com/uelkerd/vis/sessions/604a3a94-529f-4368-a825-05b310c4b9c7 Co-authored-by: uelkerd <[email protected]>
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
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.
internal/livefeed/scrcpy_control_test.gowhile preserving the InjectText failure-path assertioninternal/livefeedtestsmake lint && make build && make testvalidationmockConn.failWriteAtcomment indexing per validation feedback and re-run targeted tests