Upload videos of instrumented tests in CI#218
Open
JordanLongstaff wants to merge 57 commits into
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 3 files 3 suites 9m 30s ⏱️ For more details on these failures, see this check. Results for commit 72d0e53. ♻️ 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)
|
📝 WalkthroughWalkthroughAdds a shell script to run Android instrumented tests while recording device screens, updates CI to run that script and upload MP4 artifacts, and modifies Android instrumented tests to explicitly enable/disable network and refactors a settings toggle helper. Changes
Sequence DiagramsequenceDiagram
participant CI as CI Workflow
participant Script as Recording Script
participant Gradle as Gradle (connectedCheck)
participant Device as Android Device (adb)
participant FFmpeg as FFmpeg
participant File as MP4 Output
CI->>Script: invoke with API_LEVEL, ORIENTATION
Script->>Gradle: start ./gradlew connectedCheck & (capture PID)
Script->>Script: sleep briefly
Script->>Device: adb exec-out screenrecord --output-format=h264 & (capture PID)
Device->>FFmpeg: pipe stream into ffmpeg
FFmpeg->>File: write testRecording-API-ORIENTATION.mp4
Gradle-->>Script: tests finish (exit code)
Script->>Device: kill recorder PID (suppress errors)
Device->>FFmpeg: stream closed
FFmpeg->>File: finalize file
Script->>CI: exit with test status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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
+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.
# Conflicts: # app/src/androidTest/kotlin/artemis/agent/screens/ConnectPageScreen.kt # app/src/androidTest/kotlin/artemis/agent/setup/ConnectFragmentTest.kt
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/instrumented-tests.yml (1)
8-34:⚠️ Potential issue | 🟡 MinorInclude the new recording script in the workflow path filters.
Future changes to
.github/workflows/instrumented-test-recording.shalone will not trigger this workflow, even though Line 95 now depends on that script.🛠️ Proposed fix
paths: - .github/workflows/instrumented-tests.yml + - .github/workflows/instrumented-test-recording.sh - app/src/androidTest/** - app/src/main/** @@ paths: - .github/workflows/instrumented-tests.yml + - .github/workflows/instrumented-test-recording.sh - app/src/androidTest/** - app/src/main/**🤖 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 8 - 34, The workflow's path filters currently omit the new recording script, so changes to .github/workflows/instrumented-test-recording.sh won't trigger the instrumented-tests workflow; update both the push and pull_request "paths" arrays in .github/workflows/instrumented-tests.yml to include the new script path ".github/workflows/instrumented-test-recording.sh" (ensure the exact filename matches the script referenced on Line 95) so edits to that script will cause the workflow to run.
♻️ Duplicate comments (2)
.github/workflows/instrumented-tests.yml (1)
80-81:⚠️ Potential issue | 🟠 MajorPin the FFmpeg setup action to an immutable commit.
This new third-party action uses the mutable
v1tag while the rest of the workflow mostly pins actions by SHA. Please resolvev1to a commit and pin that SHA.#!/bin/bash set -euo pipefail # Description: Resolve AnimMouse/setup-ffmpeg@v1 to the immutable commit SHA to pin in the workflow. ref_json="$(gh api repos/AnimMouse/setup-ffmpeg/git/ref/tags/v1)" type="$(jq -r '.object.type' <<< "$ref_json")" sha="$(jq -r '.object.sha' <<< "$ref_json")" if [ "$type" = "tag" ]; then gh api "repos/AnimMouse/setup-ffmpeg/git/tags/$sha" --jq '.object.sha' else printf '%s\n' "$sha" fi🤖 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 80 - 81, The workflow is using the mutable tag "AnimMouse/setup-ffmpeg@v1" for the "Setup FFmpeg" action; resolve that tag to its commit SHA and replace the uses: entry (the string "AnimMouse/setup-ffmpeg@v1") with the corresponding immutable commit SHA (e.g., "AnimMouse/setup-ffmpeg@<sha>") so the action is pinned to a specific commit; use the provided script or gh API to obtain the correct commit SHA and update the uses value accordingly..github/workflows/instrumented-test-recording.sh (1)
13-20:⚠️ Potential issue | 🔴 CriticalCapture
RECORD_PIDbefore trying to stop the recorder.
RECORD_PIDis never assigned, so Line 19 kills nothing and Line 20 can degrade intowaitwith no PID, potentially hanging forever on the still-running recording pipeline. This also risks leaving the MP4 unfinalized.🐛 Proposed fix
-adb exec-out "while true; do screenrecord --bugreport --output-format=h264 -; done" | ffmpeg -i - testRecording-$API_LEVEL-$ORIENTATION.mp4 & +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 +wait "$TEST_PID" TEST_STATUS=$? # Terminate the screen recording process -kill $RECORD_PID 2>/dev/null || true -wait $RECORD_PID 2>/dev/null || true +kill -INT "$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 13 - 20, The screen-recording background pipeline is started without saving its PID, so RECORD_PID is unset when kill/wait run; after launching the pipeline line that ends with & (adb exec-out ... | ffmpeg ... &), capture its PID by assigning RECORD_PID=$! immediately on the next token, then use that RECORD_PID in the existing kill $RECORD_PID and wait $RECORD_PID lines (ensure RECORD_PID is exported or in scope if used in subshells); reference the background pipeline launch and the RECORD_PID variable to locate where to add the assignment.
🧹 Nitpick comments (1)
.github/workflows/instrumented-tests.yml (1)
104-109: Consider setting retention for video artifacts.This matrix can upload many MP4s per run. Adding a short
retention-dayskeeps artifact storage predictable.🧹 Proposed adjustment
with: name: "Instrumented Test Recording (API ${{ matrix.api-level }}, ${{ matrix.orientation.name }})" path: testRecording-${{ matrix.api-level }}-${{ matrix.orientation.rotation }}.mp4 + retention-days: 7🤖 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 step "Upload Test Recordings" that uses actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 should include a retention-days setting to limit how long the MP4 artifacts are kept; edit the step with name "Upload Test Recordings" and add a with key "retention-days" (e.g., a small integer like 3 or 7) alongside the existing with keys "name" and "path" so uploaded artifacts from the matrix runs are automatically expired.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In @.github/workflows/instrumented-tests.yml:
- Around line 8-34: The workflow's path filters currently omit the new recording
script, so changes to .github/workflows/instrumented-test-recording.sh won't
trigger the instrumented-tests workflow; update both the push and pull_request
"paths" arrays in .github/workflows/instrumented-tests.yml to include the new
script path ".github/workflows/instrumented-test-recording.sh" (ensure the exact
filename matches the script referenced on Line 95) so edits to that script will
cause the workflow to run.
---
Duplicate comments:
In @.github/workflows/instrumented-test-recording.sh:
- Around line 13-20: The screen-recording background pipeline is started without
saving its PID, so RECORD_PID is unset when kill/wait run; after launching the
pipeline line that ends with & (adb exec-out ... | ffmpeg ... &), capture its
PID by assigning RECORD_PID=$! immediately on the next token, then use that
RECORD_PID in the existing kill $RECORD_PID and wait $RECORD_PID lines (ensure
RECORD_PID is exported or in scope if used in subshells); reference the
background pipeline launch and the RECORD_PID variable to locate where to add
the assignment.
In @.github/workflows/instrumented-tests.yml:
- Around line 80-81: The workflow is using the mutable tag
"AnimMouse/setup-ffmpeg@v1" for the "Setup FFmpeg" action; resolve that tag to
its commit SHA and replace the uses: entry (the string
"AnimMouse/setup-ffmpeg@v1") with the corresponding immutable commit SHA (e.g.,
"AnimMouse/setup-ffmpeg@<sha>") so the action is pinned to a specific commit;
use the provided script or gh API to obtain the correct commit SHA and update
the uses value accordingly.
---
Nitpick comments:
In @.github/workflows/instrumented-tests.yml:
- Around line 104-109: The upload step "Upload Test Recordings" that uses
actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 should include
a retention-days setting to limit how long the MP4 artifacts are kept; edit the
step with name "Upload Test Recordings" and add a with key "retention-days"
(e.g., a small integer like 3 or 7) alongside the existing with keys "name" and
"path" so uploaded artifacts from the matrix runs are automatically expired.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0e1f8543-4213-4192-963b-33bd9db0a019
📒 Files selected for processing (3)
.github/workflows/instrumented-test-recording.sh.github/workflows/instrumented-tests.ymlapp/src/androidTest/kotlin/artemis/agent/setup/ConnectFragmentTest.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/androidTest/kotlin/artemis/agent/setup/ConnectFragmentTest.kt
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 8-21: Start the adb/ffmpeg screen recorder before launching the
tests so no early output is missed: start the adb exec-out ... | ffmpeg ...
background pipeline first, capture its PID into RECORD_PID, then start ./gradlew
connectedCheck in background and capture TEST_PID; remove the 10s blind sleep.
Also quote the output filename variable in the ffmpeg command (use
"testRecording-$API_LEVEL-$ORIENTATION.mp4") to avoid word-splitting, and quote
PID variables when terminating/waiting (use "$RECORD_PID" and "$TEST_PID") to be
safe and consistent.
🪄 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: 3e481ea3-8293-44be-9fb4-4a63099ddada
📒 Files selected for processing (2)
.github/workflows/gradle.yml.github/workflows/instrumented-test-recording.sh
Comment on lines
+8
to
+21
| echo "Starting instrumented tests..." | ||
| ./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 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && find . -name "instrumented-test-recording.sh" 2>/dev/null | head -5Repository: JPL-Software-Solutions/ArtemisAgent
Length of output: 61
🏁 Script executed:
git ls-files | grep -i "instrumented-test-recording"Repository: JPL-Software-Solutions/ArtemisAgent
Length of output: 127
🏁 Script executed:
cat -n .github/workflows/instrumented-test-recording.sh 2>/dev/null || echo "File not found at root level"Repository: JPL-Software-Solutions/ArtemisAgent
Length of output: 790
🏁 Script executed:
find . -type f -name "instrumented-test-recording.sh" 2>/dev/nullRepository: JPL-Software-Solutions/ArtemisAgent
Length of output: 129
Start the recording before launching the tests to capture the entire test execution.
The 10-second sleep on line 11 creates a blind spot where early crashes or fast test failures are not recorded. Starting the recorder first ensures the entire test run is captured for debugging. Additionally, quote the variables in the output filename on line 13 to address the ShellCheck warning and prevent word splitting, and quote the PID variable in the kill/wait commands on lines 20–21 for consistency and safety.
🎥 Proposed adjustment
-echo "Starting instrumented tests..."
-./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 &
+adb exec-out "while true; do screenrecord --bugreport --output-format=h264 -; done" | ffmpeg -i - "testRecording-${API_LEVEL}-${ORIENTATION}.mp4" &
RECORD_PID=$!
sleep 1
+echo "Starting instrumented tests..."
+./gradlew connectedCheck &
+TEST_PID=$!
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
+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
| echo "Starting instrumented tests..." | |
| ./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 | |
| 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 "Starting instrumented tests..." | |
| ./gradlew connectedCheck & | |
| TEST_PID=$! | |
| 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 |
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 13-13: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 13-13: Double quote to prevent globbing and word splitting.
(SC2086)
🤖 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 8 - 21, Start
the adb/ffmpeg screen recorder before launching the tests so no early output is
missed: start the adb exec-out ... | ffmpeg ... background pipeline first,
capture its PID into RECORD_PID, then start ./gradlew connectedCheck in
background and capture TEST_PID; remove the 10s blind sleep. Also quote the
output filename variable in the ffmpeg command (use
"testRecording-$API_LEVEL-$ORIENTATION.mp4") to avoid word-splitting, and quote
PID variables when terminating/waiting (use "$RECORD_PID" and "$TEST_PID") to be
safe and consistent.
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.
Summary by CodeRabbit
New Features
Tests
Chores
Chores