Improve Depot test workflow: diagnostics, filter, timeout - #720
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR modifies CI/CD workflows to expand test coverage in the main CI pipeline and introduces parameterized test control in the test-depot workflow, including optional unit test skipping, test filtering, and execution time allowance settings. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f3885c1a4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /usr/sbin/system_profiler SPSoftwareDataType | grep -i "boot mode" || true | ||
| echo "" | ||
| echo "=== Accessibility permissions ===" | ||
| tccutil reset Accessibility 2>/dev/null || true |
There was a problem hiding this comment.
In test-depot.yml, the diagnostics block calls tccutil reset Accessibility, which mutates runner state instead of just collecting diagnostics; on Depot hosts that rely on pre-granted Accessibility access for UI automation, this revokes permission immediately before the Run UI tests step and can cause event-synthesis failures or hangs. Because this runs unconditionally for every dispatch, it can introduce flaky or consistently failing UI test runs.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR enhances the Depot test workflow with three key improvements: diagnostic logging, test filtering, and timeout controls. Key Changes:
The implementation uses bash conditionals to build the Confidence Score: 4/5
Important Files Changed
Last reviewed commit: 6f3885c |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
166-174:⚠️ Potential issue | 🟠 MajorAdd
-maximum-test-execution-time-allowanceto prevent hung UI tests from blocking the self-hosted runner.Line 174 is missing the per-test timeout that the test-depot.yml workflow includes by default (120 seconds). Without it, a single hung UI test can block the self-hosted queue indefinitely. The test-depot.yml version explicitly sets this flag; ci.yml should match.
Suggested patch
- name: Run UI tests run: | set -euo pipefail SOURCE_PACKAGES_DIR="$PWD/.ci-source-packages" xcodebuild -project GhosttyTabs.xcodeproj -scheme cmux -configuration Debug \ -clonedSourcePackagesDirPath "$SOURCE_PACKAGES_DIR" \ -disableAutomaticPackageResolution \ -destination "platform=macOS" \ + -maximum-test-execution-time-allowance 120 \ -only-testing:cmuxUITests test🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 166 - 174, The xcodebuild invocation in the "Run UI tests" step (the xcodebuild call that specifies -project GhosttyTabs.xcodeproj, -scheme cmux and -only-testing:cmuxUITests) is missing the per-test timeout flag; add the -maximum-test-execution-time-allowance 120 (or your desired seconds) to the xcodebuild arguments so a hung UI test won't block the self-hosted runner, ensuring the flag is added alongside the existing options in that Run UI tests step.
🤖 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/test-depot.yml:
- Around line 85-95: The "Display diagnostics" workflow step mutates runner
state by calling tccutil reset Accessibility; remove the mutation to keep
diagnostics read-only by deleting or commenting out the tccutil reset
Accessibility (and the "Accessibility reset attempted" echo) in the "Display
diagnostics" step so only non-mutating commands like system_profiler and greps
remain, ensuring XCUITest accessibility permissions are preserved for the
subsequent "Run UI tests" step.
- Around line 173-184: The xcodebuild command currently expands $ONLY_TESTING
unquoted which allows user-supplied TEST_FILTER to cause word-splitting or
injection; update the xcodebuild invocation to quote the variable (use
"$ONLY_TESTING") so the entire value is passed as a single argument, keeping the
existing logic that sets ONLY_TESTING from TEST_FILTER intact and preventing
malformed CLI args when TEST_FILTER contains spaces or metacharacters.
- Around line 24-27: Validate the workflow input `test_timeout` before it's
passed to xcodebuild: in the "Run UI tests" step add a check that `test_timeout`
is a positive integer (or empty) and if it isn't, fail the step with a clear
error message explaining that `test_timeout` must be a numeric value in seconds;
only pass `-maximum-test-execution-time-allowance` to xcodebuild when validation
succeeds and include the validated numeric value, otherwise omit the flag to
avoid xcodebuild cryptic failures.
---
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 166-174: The xcodebuild invocation in the "Run UI tests" step (the
xcodebuild call that specifies -project GhosttyTabs.xcodeproj, -scheme cmux and
-only-testing:cmuxUITests) is missing the per-test timeout flag; add the
-maximum-test-execution-time-allowance 120 (or your desired seconds) to the
xcodebuild arguments so a hung UI test won't block the self-hosted runner,
ensuring the flag is added alongside the existing options in that Run UI tests
step.
| test_timeout: | ||
| description: "Per-test timeout in seconds (default: 120)" | ||
| required: false | ||
| default: "120" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Read the relevant sections of the test-depot.yml file
head -n 35 .github/workflows/test-depot.ymlRepository: manaflow-ai/cmux
Length of output: 977
🏁 Script executed:
# Check how TEST_TIMEOUT is used around line 24-27 and later invocations
cat -n .github/workflows/test-depot.yml | sed -n '20,40p'Repository: manaflow-ai/cmux
Length of output: 802
🏁 Script executed:
# Look at lines around 167 and 183 to see if similar issues exist there
cat -n .github/workflows/test-depot.yml | sed -n '160,190p'Repository: manaflow-ai/cmux
Length of output: 1135
🏁 Script executed:
# Find all references to test_timeout and TEST_TIMEOUT in the workflow
rg -n 'test_timeout|TEST_TIMEOUT' .github/workflows/test-depot.ymlRepository: manaflow-ai/cmux
Length of output: 219
🏁 Script executed:
# Get the full workflow file to identify all test invocations
wc -l .github/workflows/test-depot.ymlRepository: manaflow-ai/cmux
Length of output: 96
🏁 Script executed:
# Check if there are other test sections beyond what we've seen
cat -n .github/workflows/test-depot.yml | tail -100Repository: manaflow-ai/cmux
Length of output: 4688
🏁 Script executed:
# Check xcodebuild documentation or search for any error handling patterns
# Let's also verify if there's any validation elsewhere in the codebase
rg -n 'maximum-test-execution-time-allowance|test.timeout|TEST_TIMEOUT' . --type yaml --type sh --type bash | head -20Repository: manaflow-ai/cmux
Length of output: 88
🏁 Script executed:
# Search for any xcodebuild error handling or validation patterns
rg -n '\[0-9\]|^[0-9]|=~.*[0-9]' .github/workflows/ --type yaml | grep -i timeoutRepository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
# Get full context around the UI tests section to understand the error handling
cat -n .github/workflows/test-depot.yml | sed -n '163,185p'Repository: manaflow-ai/cmux
Length of output: 1085
Add validation for test_timeout before passing to xcodebuild.
test_timeout is a free-form string input with no validation. Non-numeric values pass directly to the -maximum-test-execution-time-allowance xcodebuild flag, causing xcodebuild to fail with a cryptic error message. Validate early in the "Run UI tests" step to fail clearly with an actionable message.
Suggested patch
- name: Run UI tests
if: ${{ !inputs.skip_ui_tests }}
env:
TEST_FILTER: ${{ inputs.test_filter }}
TEST_TIMEOUT: ${{ inputs.test_timeout || '120' }}
run: |
set -euo pipefail
SOURCE_PACKAGES_DIR="$PWD/.ci-source-packages"
+ if ! [[ "$TEST_TIMEOUT" =~ ^[0-9]+$ ]] || [ "$TEST_TIMEOUT" -le 0 ]; then
+ echo "Invalid test_timeout: '$TEST_TIMEOUT'. Provide a positive integer (seconds)." >&2
+ exit 1
+ fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/test-depot.yml around lines 24 - 27, Validate the workflow
input `test_timeout` before it's passed to xcodebuild: in the "Run UI tests"
step add a check that `test_timeout` is a positive integer (or empty) and if it
isn't, fail the step with a clear error message explaining that `test_timeout`
must be a numeric value in seconds; only pass
`-maximum-test-execution-time-allowance` to xcodebuild when validation succeeds
and include the validated numeric value, otherwise omit the flag to avoid
xcodebuild cryptic failures.
| - name: Display diagnostics | ||
| run: | | ||
| echo "=== Screen resolution ===" | ||
| system_profiler SPDisplaysDataType 2>/dev/null || echo "No display info available" | ||
| echo "" | ||
| echo "=== Window server ===" | ||
| /usr/sbin/system_profiler SPSoftwareDataType | grep -i "boot mode" || true | ||
| echo "" | ||
| echo "=== Accessibility permissions ===" | ||
| tccutil reset Accessibility 2>/dev/null || true | ||
| echo "Accessibility reset attempted" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n .github/workflows/test-depot.yml | head -100 | tail -20Repository: manaflow-ai/cmux
Length of output: 951
🏁 Script executed:
# Get the exact section around lines 85-95
sed -n '75,100p' .github/workflows/test-depot.ymlRepository: manaflow-ai/cmux
Length of output: 1059
🏁 Script executed:
# Check if there are other steps in the same job that might use UI automation
rg -n "ui|UI|accessibility|Accessibility|automation|screenshot|xcode" .github/workflows/test-depot.yml -iRepository: manaflow-ai/cmux
Length of output: 1927
Keep diagnostics read-only; remove Accessibility reset.
The Display diagnostics step runs tccutil reset Accessibility (line 94), which mutates runner state by clearing accessibility permissions in the TCC database. The subsequent Run UI tests step requires those permissions for XCUITest framework to interact with UI elements. Resetting accessibility before UI tests invalidates the permissions needed for test automation.
Suggested patch
- name: Display diagnostics
run: |
echo "=== Screen resolution ==="
system_profiler SPDisplaysDataType 2>/dev/null || echo "No display info available"
echo ""
echo "=== Window server ==="
/usr/sbin/system_profiler SPSoftwareDataType | grep -i "boot mode" || true
echo ""
- echo "=== Accessibility permissions ==="
- tccutil reset Accessibility 2>/dev/null || true
- echo "Accessibility reset attempted"
+ echo "=== Accessibility diagnostics ==="
+ echo "Skipping TCC reset in diagnostics step to avoid mutating test environment"📝 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.
| - name: Display diagnostics | |
| run: | | |
| echo "=== Screen resolution ===" | |
| system_profiler SPDisplaysDataType 2>/dev/null || echo "No display info available" | |
| echo "" | |
| echo "=== Window server ===" | |
| /usr/sbin/system_profiler SPSoftwareDataType | grep -i "boot mode" || true | |
| echo "" | |
| echo "=== Accessibility permissions ===" | |
| tccutil reset Accessibility 2>/dev/null || true | |
| echo "Accessibility reset attempted" | |
| - name: Display diagnostics | |
| run: | | |
| echo "=== Screen resolution ===" | |
| system_profiler SPDisplaysDataType 2>/dev/null || echo "No display info available" | |
| echo "" | |
| echo "=== Window server ===" | |
| /usr/sbin/system_profiler SPSoftwareDataType | grep -i "boot mode" || true | |
| echo "" | |
| echo "=== Accessibility diagnostics ===" | |
| echo "Skipping TCC reset in diagnostics step to avoid mutating test environment" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/test-depot.yml around lines 85 - 95, The "Display
diagnostics" workflow step mutates runner state by calling tccutil reset
Accessibility; remove the mutation to keep diagnostics read-only by deleting or
commenting out the tccutil reset Accessibility (and the "Accessibility reset
attempted" echo) in the "Display diagnostics" step so only non-mutating commands
like system_profiler and greps remain, ensuring XCUITest accessibility
permissions are preserved for the subsequent "Run UI tests" step.
| if [ -n "$TEST_FILTER" ]; then | ||
| ONLY_TESTING="-only-testing:cmuxUITests/$TEST_FILTER" | ||
| else | ||
| ONLY_TESTING="-only-testing:cmuxUITests" | ||
| fi | ||
|
|
||
| xcodebuild -project GhosttyTabs.xcodeproj -scheme cmux -configuration Debug \ | ||
| -clonedSourcePackagesDirPath "$SOURCE_PACKAGES_DIR" \ | ||
| -disableAutomaticPackageResolution \ | ||
| -destination "platform=macOS" \ | ||
| -only-testing:cmuxUITests/UpdatePillUITests test | ||
| -maximum-test-execution-time-allowance "$TEST_TIMEOUT" \ | ||
| $ONLY_TESTING test |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n .github/workflows/test-depot.yml | sed -n '170,190p'Repository: manaflow-ai/cmux
Length of output: 803
🏁 Script executed:
rg -n 'TEST_FILTER' .github/workflows/test-depot.ymlRepository: manaflow-ai/cmux
Length of output: 227
🏁 Script executed:
head -100 .github/workflows/test-depot.yml | rg -n 'inputs|test_filter|on:'Repository: manaflow-ai/cmux
Length of output: 528
🏁 Script executed:
head -30 .github/workflows/test-depot.ymlRepository: manaflow-ai/cmux
Length of output: 820
Quote ONLY_TESTING when passing it to xcodebuild.
The test_filter input on line 20 accepts arbitrary user-provided text with no validation. Line 184 expands $ONLY_TESTING unquoted, which contains this user input via line 174. If test_filter contains spaces or shell metacharacters, unquoted expansion will cause word splitting and produce malformed CLI arguments.
Suggested patch
xcodebuild -project GhosttyTabs.xcodeproj -scheme cmux -configuration Debug \
-clonedSourcePackagesDirPath "$SOURCE_PACKAGES_DIR" \
-disableAutomaticPackageResolution \
-destination "platform=macOS" \
-maximum-test-execution-time-allowance "$TEST_TIMEOUT" \
- $ONLY_TESTING test
+ "$ONLY_TESTING" test🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/test-depot.yml around lines 173 - 184, The xcodebuild
command currently expands $ONLY_TESTING unquoted which allows user-supplied
TEST_FILTER to cause word-splitting or injection; update the xcodebuild
invocation to quote the variable (use "$ONLY_TESTING") so the entire value is
passed as a single argument, keeping the existing logic that sets ONLY_TESTING
from TEST_FILTER intact and preventing malformed CLI args when TEST_FILTER
contains spaces or metacharacters.
…orkflow - Display diagnostics step to debug headless display issues - test_filter input to run specific XCUITest classes - test_timeout input (default 120s) to prevent test hangs
6f3885c to
6ca8972
Compare
…orkflow (manaflow-ai#720) - Display diagnostics step to debug headless display issues - test_filter input to run specific XCUITest classes - test_timeout input (default 120s) to prevent test hangs
This branch was successfully deployed
1 participant
Summary
test_filterinput to run a specific test class (e.g.UpdatePillUITests) instead of the entire suitetest_timeoutinput (default 120s) with-maximum-test-execution-time-allowanceto prevent individual tests from hanging forever (SidebarResizeUITests hung for 35+ minutes last run)Testing
gh workflow run test-depot.yml -f skip_unit_tests=true -f test_filter=UpdatePillUITestsRelated
Summary by CodeRabbit
Release Notes