Upload videos of instrumented tests in CI#218
Open
JordanLongstaff wants to merge 51 commits intomainfrom
Open
Conversation
Contributor
Code Coverage (app)
|
Contributor
Code Coverage (IAN/udp)
|
Contributor
Code Coverage (IAN/vesseldata)
|
Contributor
Code Coverage (IAN)
|
Contributor
Contributor
Instrumented Test Results 27 files 27 suites 9h 47m 7s ⏱️ For more details on these failures, see this check. Results for commit ab570b2. ♻️ This comment has been updated with latest results. |
Contributor
Code Coverage (IAN/listener)
|
Contributor
Code Coverage (IAN/util)
|
Contributor
Contributor
Code Coverage (IAN/packets)
|
Contributor
Code Coverage (IAN/world)
|
Contributor
Code Coverage (IAN/enums)
|
Contributor
Total Code Coverage
|
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files
|
# Conflicts: # .github/workflows/instrumented-tests.yml
# Conflicts: # app/build.gradle.kts # app/src/main/kotlin/artemis/agent/UpdateCheck.kt
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| uses: gradle/actions/setup-gradle@ed408507eac070d1f99cc633dbcf757c94c7933a # v4 | ||
|
|
||
| - name: Setup FFmpeg | ||
| uses: AnimMouse/setup-ffmpeg@v1 |
Check warning
Code scanning / CodeQL
Unpinned tag for a non-immutable Action in workflow Medium
Collaborator
Author
|
Generated by 🚫 Danger |
Contributor
|
No dependency violations found. 👍 |
Contributor
Code Coverage (IAN/grid)
|
📝 WalkthroughWalkthroughA new shell script automates Android instrumented test execution with concurrent screen recording via FFmpeg. The CI workflow updated to invoke this recording script and upload MP4 artifacts. Test files enhanced with network connectivity enable/disable steps and assertion refactoring to support network-related test scenarios. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 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: 2
🧹 Nitpick comments (3)
.github/workflows/instrumented-tests.yml (2)
104-109: Inconsistent action pinning: use commit hash instead of@v4.Line 98 pins
actions/upload-artifactto commit hash@bbbca2ddaa5d8feaa63e36b76fdaad77386f024f # v7.0.0, but line 105 uses@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4. This appears to be an older version (v4 vs v7) and the comment format differs. Consider using the same version consistently across both upload steps.♻️ Proposed fix
- name: Upload Test Recordings - uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4 + uses: actions/upload-artifact@bbbca2ddaa5d8feaa63e36b76fdaad77386f024f # v7.0.0 if: always() with: name: "Instrumented Test Recording (API ${{ matrix.api-level }}, ${{ matrix.orientation.name }})" path: testRecording-${{ matrix.api-level }}-${{ matrix.orientation.rotation }}.mp4🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/instrumented-tests.yml around lines 104 - 109, The "Upload Test Recordings" workflow step inconsistently pins actions/upload-artifact to `@ea165f8`... (# v4); update that step (the one named "Upload Test Recordings" and the uses: line referencing actions/upload-artifact) to use the same pin style and commit hash as the other upload step (replace `@ea165f8`... with `@bbbca2ddaa5d8feaa63e36b76fdaad77386f024f` and update the inline comment to match # v7.0.0) so both upload-artifact usages are consistently pinned to the same version.
83-84: Consider setting execute permission in the repository instead of at runtime.The script should be committed with executable permission (
git update-index --chmod=+x). This avoids the extra step and ensures the script is always executable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/instrumented-tests.yml around lines 83 - 84, Remove the runtime chmod step in the "Enable script execution" job and instead commit the script with executable permission; run git update-index --chmod=+x .github/workflows/instrumented-test-recording.sh locally (or mark it executable in your git client) so the script .github/workflows/instrumented-test-recording.sh is stored executable in the repo and you can delete the `run: chmod +x .github/workflows/instrumented-test-recording.sh` step from the workflow..github/workflows/instrumented-test-recording.sh (1)
1-1: Consider using#!/bin/bashfor better portability of process handling.The script uses
#!/bin/sh, but relies on behavior ($!,kill, process management) that is more predictable in bash. Additionally, if you need to use arrays or other bash-specific features in the future, this will be limiting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/instrumented-test-recording.sh at line 1, The script uses a POSIX sh shebang but relies on process-handling behavior better provided by bash; update the shebang from "#!/bin/sh" to "#!/bin/bash" at the top of the script to ensure predictable handling of background PIDs ($!), kill, and future bash-specific features (and confirm the script remains executable).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/instrumented-test-recording.sh:
- Around line 9-19: Move the PID capture for the Gradle test runner so TEST_PID
is assigned immediately after backgrounding ./gradlew connectedCheck (&) to
ensure it refers to that process; then start the adb exec-out | ffmpeg pipeline
in the background and capture its PID (the $! returned right after that
backgrounding) into a RECORDING_PID variable; after wait $TEST_PID completes,
send a graceful termination to the recording process (use a signal that lets
ffmpeg finalize the MP4, e.g., kill -INT $RECORDING_PID or kill -TERM then wait)
and then wait for $RECORDING_PID to exit so the recording is finalized and
resources are cleaned up.
In `@app/src/androidTest/kotlin/artemis/agent/setup/ConnectFragmentTest.kt`:
- Around line 201-234: The noNetworkTest disables networking via
device.network.disable() but never restores it; update the noNetworkTest (inside
mainScreenTest and around the device.network.disable() call) to ensure
device.network.enable() is always called (e.g., wrap the disable + assertions in
a try/finally or call enable at the end), so network is re-enabled for
subsequent tests while preserving existing steps like toggleShowingInfo() and
ConnectPageScreen checks.
---
Nitpick comments:
In @.github/workflows/instrumented-test-recording.sh:
- Line 1: The script uses a POSIX sh shebang but relies on process-handling
behavior better provided by bash; update the shebang from "#!/bin/sh" to
"#!/bin/bash" at the top of the script to ensure predictable handling of
background PIDs ($!), kill, and future bash-specific features (and confirm the
script remains executable).
In @.github/workflows/instrumented-tests.yml:
- Around line 104-109: The "Upload Test Recordings" workflow step inconsistently
pins actions/upload-artifact to `@ea165f8`... (# v4); update that step (the one
named "Upload Test Recordings" and the uses: line referencing
actions/upload-artifact) to use the same pin style and commit hash as the other
upload step (replace `@ea165f8`... with `@bbbca2ddaa5d8feaa63e36b76fdaad77386f024f`
and update the inline comment to match # v7.0.0) so both upload-artifact usages
are consistently pinned to the same version.
- Around line 83-84: Remove the runtime chmod step in the "Enable script
execution" job and instead commit the script with executable permission; run git
update-index --chmod=+x .github/workflows/instrumented-test-recording.sh locally
(or mark it executable in your git client) so the script
.github/workflows/instrumented-test-recording.sh is stored executable in the
repo and you can delete the `run: chmod +x
.github/workflows/instrumented-test-recording.sh` step from the workflow.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 760e2d51-7e0c-401f-a348-d2934f079031
📒 Files selected for processing (5)
.github/workflows/instrumented-test-recording.sh.github/workflows/instrumented-tests.ymlapp/src/androidTest/kotlin/artemis/agent/scenario/ConnectScenario.ktapp/src/androidTest/kotlin/artemis/agent/screens/ConnectPageScreen.ktapp/src/androidTest/kotlin/artemis/agent/setup/ConnectFragmentTest.kt
Comment on lines
+9
to
+19
| ./gradlew connectedCheck & | ||
| sleep 10 | ||
| TEST_PID=$! | ||
| echo "Starting the screen recording..." | ||
| adb exec-out "while true; do screenrecord --bugreport --output-format=h264 -; done" | ffmpeg -i - testRecording-$API_LEVEL-$ORIENTATION.mp4 & | ||
| sleep 1 | ||
| echo "Waiting for instrumented tests to finish..." | ||
| wait $TEST_PID | ||
| TEST_STATUS=$? | ||
| # Wait for the screen recording process to exit | ||
| sleep 1 |
There was a problem hiding this comment.
Critical bugs: PID captured from wrong process and recording process never terminated.
Two issues here:
-
Wrong PID captured (Line 11):
$!captures the PID of the most recently backgrounded command. Sincesleep 10runs after./gradlew connectedCheck &, by line 11 the$!still refers to the gradle process—but placing the capture after the sleep is fragile and confusing. Move it immediately after backgrounding gradle. -
Recording process never killed (Lines 13-19): The
adb exec-out+ffmpegpipeline runs an infinitewhile trueloop that never terminates. Afterwait $TEST_PIDcompletes, the script exits but leaves the recording process orphaned. This can corrupt the MP4 (no proper finalization) and leak resources across matrix jobs.
🐛 Proposed fix
set -x
set +e
echo "Starting instrumented tests..."
./gradlew connectedCheck &
+TEST_PID=$!
sleep 10
-TEST_PID=$!
echo "Starting the screen recording..."
adb exec-out "while true; do screenrecord --bugreport --output-format=h264 -; done" | ffmpeg -i - testRecording-$API_LEVEL-$ORIENTATION.mp4 &
+RECORD_PID=$!
sleep 1
echo "Waiting for instrumented tests to finish..."
wait $TEST_PID
TEST_STATUS=$?
-# Wait for the screen recording process to exit
-sleep 1
+# Terminate the screen recording process
+kill $RECORD_PID 2>/dev/null || true
+wait $RECORD_PID 2>/dev/null || true
exit $TEST_STATUS📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ./gradlew connectedCheck & | |
| sleep 10 | |
| TEST_PID=$! | |
| echo "Starting the screen recording..." | |
| adb exec-out "while true; do screenrecord --bugreport --output-format=h264 -; done" | ffmpeg -i - testRecording-$API_LEVEL-$ORIENTATION.mp4 & | |
| sleep 1 | |
| echo "Waiting for instrumented tests to finish..." | |
| wait $TEST_PID | |
| TEST_STATUS=$? | |
| # Wait for the screen recording process to exit | |
| sleep 1 | |
| ./gradlew connectedCheck & | |
| TEST_PID=$! | |
| sleep 10 | |
| echo "Starting the screen recording..." | |
| adb exec-out "while true; do screenrecord --bugreport --output-format=h264 -; done" | ffmpeg -i - testRecording-$API_LEVEL-$ORIENTATION.mp4 & | |
| RECORD_PID=$! | |
| sleep 1 | |
| echo "Waiting for instrumented tests to finish..." | |
| wait $TEST_PID | |
| TEST_STATUS=$? | |
| # Terminate the screen recording process | |
| kill $RECORD_PID 2>/dev/null || true | |
| wait $RECORD_PID 2>/dev/null || true | |
| exit $TEST_STATUS |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/instrumented-test-recording.sh around lines 9 - 19, Move
the PID capture for the Gradle test runner so TEST_PID is assigned immediately
after backgrounding ./gradlew connectedCheck (&) to ensure it refers to that
process; then start the adb exec-out | ffmpeg pipeline in the background and
capture its PID (the $! returned right after that backgrounding) into a
RECORDING_PID variable; after wait $TEST_PID completes, send a graceful
termination to the recording process (use a signal that lets ffmpeg finalize the
MP4, e.g., kill -INT $RECORDING_PID or kill -TERM then wait) and then wait for
$RECORDING_PID to exit so the recording is finalized and resources are cleaned
up.
Comment on lines
+201
to
+234
| @Test | ||
| fun noNetworkTest() { | ||
| run { | ||
| mainScreenTest { | ||
| step("Disable network connections") { device.network.disable() } | ||
|
|
||
| val showingInfo = AtomicBoolean() | ||
| step("Fetch showing network info setting") { | ||
| activityScenarioRule.scenario.onActivity { activity -> | ||
| showingInfo.lazySet( | ||
| activity.viewModels<AgentViewModel>().value.showingNetworkInfo | ||
| ) | ||
| } | ||
| } | ||
| val settingValue = showingInfo.get() | ||
|
|
||
| if (!settingValue) { | ||
| toggleShowingInfo() | ||
| } | ||
|
|
||
| ConnectPageScreen { | ||
| step("Check \"Network not found\" text") { | ||
| networkTypeLabel.isDisplayedWithText(R.string.network_not_found) | ||
| } | ||
|
|
||
| step("Check for no address") { addressLabel.isNotDisplayed() } | ||
| } | ||
|
|
||
| if (!settingValue) { | ||
| toggleShowingInfo() | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Test does not re-enable network after disabling it.
The test disables network at line 205 but never re-enables it. If tests share process state or run in a specific order, this could cause subsequent tests (e.g., scanTest, showNetworkInfoTest) to fail due to disabled network. Consider re-enabling network in a finally block or at the end of the test.
🛡️ Proposed fix
`@Test`
fun noNetworkTest() {
run {
mainScreenTest {
step("Disable network connections") { device.network.disable() }
val showingInfo = AtomicBoolean()
step("Fetch showing network info setting") {
activityScenarioRule.scenario.onActivity { activity ->
showingInfo.lazySet(
activity.viewModels<AgentViewModel>().value.showingNetworkInfo
)
}
}
val settingValue = showingInfo.get()
if (!settingValue) {
toggleShowingInfo()
}
ConnectPageScreen {
step("Check \"Network not found\" text") {
networkTypeLabel.isDisplayedWithText(R.string.network_not_found)
}
step("Check for no address") { addressLabel.isNotDisplayed() }
}
if (!settingValue) {
toggleShowingInfo()
}
+
+ step("Re-enable network connections") { device.network.enable() }
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @Test | |
| fun noNetworkTest() { | |
| run { | |
| mainScreenTest { | |
| step("Disable network connections") { device.network.disable() } | |
| val showingInfo = AtomicBoolean() | |
| step("Fetch showing network info setting") { | |
| activityScenarioRule.scenario.onActivity { activity -> | |
| showingInfo.lazySet( | |
| activity.viewModels<AgentViewModel>().value.showingNetworkInfo | |
| ) | |
| } | |
| } | |
| val settingValue = showingInfo.get() | |
| if (!settingValue) { | |
| toggleShowingInfo() | |
| } | |
| ConnectPageScreen { | |
| step("Check \"Network not found\" text") { | |
| networkTypeLabel.isDisplayedWithText(R.string.network_not_found) | |
| } | |
| step("Check for no address") { addressLabel.isNotDisplayed() } | |
| } | |
| if (!settingValue) { | |
| toggleShowingInfo() | |
| } | |
| } | |
| } | |
| } | |
| `@Test` | |
| fun noNetworkTest() { | |
| run { | |
| mainScreenTest { | |
| step("Disable network connections") { device.network.disable() } | |
| val showingInfo = AtomicBoolean() | |
| step("Fetch showing network info setting") { | |
| activityScenarioRule.scenario.onActivity { activity -> | |
| showingInfo.lazySet( | |
| activity.viewModels<AgentViewModel>().value.showingNetworkInfo | |
| ) | |
| } | |
| } | |
| val settingValue = showingInfo.get() | |
| if (!settingValue) { | |
| toggleShowingInfo() | |
| } | |
| ConnectPageScreen { | |
| step("Check \"Network not found\" text") { | |
| networkTypeLabel.isDisplayedWithText(R.string.network_not_found) | |
| } | |
| step("Check for no address") { addressLabel.isNotDisplayed() } | |
| } | |
| if (!settingValue) { | |
| toggleShowingInfo() | |
| } | |
| step("Re-enable network connections") { device.network.enable() } | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/androidTest/kotlin/artemis/agent/setup/ConnectFragmentTest.kt` around
lines 201 - 234, The noNetworkTest disables networking via
device.network.disable() but never restores it; update the noNetworkTest (inside
mainScreenTest and around the device.network.disable() call) to ensure
device.network.enable() is always called (e.g., wrap the disable + assertions in
a try/finally or call enable at the end), so network is re-enabled for
subsequent tests while preserving existing steps like toggleShowingInfo() and
ConnectPageScreen checks.
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 by CodeRabbit
Tests
Chores