Testing timeouts followup#156
Conversation
added SPEC_TEST_EXCLUDES support for comma/whitespace-separated paths or globs, matching both relative spec paths and basenames.
…has no set -u, "${timeout_cmd[@]}" expands to zero words when empty, so the timeout wrapper can be prepended unconditionally. The xvfb and non-xvfb branches each drop their inner if [[ ${#timeout_cmd[@]} -gt 0 ]] split — removing ~14 lines with identical behavior.
2. Added validation for DRAWIO_RENDER_TIMEOUT. render_timeout_command now rejects anything that isn't a GNU-timeout duration (^[0-9]+(\.[0-9]+)?[smhd]?$), warning to stderr and disabling the guard instead of passing a bad value to timeout (which would exit 125 and surface as an opaque "render failed").
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 31 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
![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 introduces the ability to skip large or slow spec fixtures by setting the SPEC_TEST_EXCLUDES environment variable, and simplifies timeout handling in drawio_export.sh. The review feedback identifies a critical compatibility issue with older Bash versions (such as Bash 3.2 on macOS) when set -u is enabled. Expanding empty arrays (timeout_cmd and spec_exclude_patterns) will trigger an unbound variable error and crash the script. To resolve this, the reviewer suggests using the parameter expansion workaround "${array[@]+"${array[@]}"}".
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| else | ||
| render_status="$?" | ||
| fi | ||
| if "${timeout_cmd[@]}" xvfb-run -a "${drawio_cmd[@]}" -x -f png -o "$output_file" "$input_file" > "$render_log" 2>&1; then |
There was a problem hiding this comment.
In Bash versions prior to 4.4 (such as the default /bin/bash 3.2 on macOS), expanding an empty array like "${timeout_cmd[@]}" when set -u (nounset) is enabled triggers an unbound variable error and terminates the script.
Since test_specs.sh enables set -euo pipefail and sources this script, this change will break execution on macOS if timeout_cmd is empty (e.g., when timeout is not installed or disabled).
To maintain compatibility with older Bash versions under set -u, use the parameter expansion workaround: "${timeout_cmd[@]+"${timeout_cmd[@]}"}".
| if "${timeout_cmd[@]}" xvfb-run -a "${drawio_cmd[@]}" -x -f png -o "$output_file" "$input_file" > "$render_log" 2>&1; then | |
| if "${timeout_cmd[@]+"${timeout_cmd[@]}"}" xvfb-run -a "${drawio_cmd[@]}" -x -f png -o "$output_file" "$input_file" > "$render_log" 2>&1; then |
| else | ||
| render_status="$?" | ||
| fi | ||
| if "${timeout_cmd[@]}" "${drawio_cmd[@]}" -x -f png -o "$output_file" "$input_file" > "$render_log" 2>&1; then |
There was a problem hiding this comment.
In Bash versions prior to 4.4 (such as the default /bin/bash 3.2 on macOS), expanding an empty array like "${timeout_cmd[@]}" when set -u (nounset) is enabled triggers an unbound variable error and terminates the script.
Since test_specs.sh enables set -euo pipefail and sources this script, this change will break execution on macOS if timeout_cmd is empty (e.g., when timeout is not installed or disabled).
To maintain compatibility with older Bash versions under set -u, use the parameter expansion workaround: "${timeout_cmd[@]+"${timeout_cmd[@]}"}".
| if "${timeout_cmd[@]}" "${drawio_cmd[@]}" -x -f png -o "$output_file" "$input_file" > "$render_log" 2>&1; then | |
| if "${timeout_cmd[@]+"${timeout_cmd[@]}"}" "${drawio_cmd[@]}" -x -f png -o "$output_file" "$input_file" > "$render_log" 2>&1; then |
| local basename="${rel_path##*/}" | ||
| local pattern | ||
|
|
||
| for pattern in "${spec_exclude_patterns[@]}"; do |
There was a problem hiding this comment.
In Bash versions prior to 4.4 (such as the default /bin/bash 3.2 on macOS), expanding an empty array like "${spec_exclude_patterns[@]}" when set -u (nounset) is enabled triggers an unbound variable error and terminates the script.
Since test_specs.sh enables set -euo pipefail, and spec_exclude_patterns is empty by default when SPEC_TEST_EXCLUDES is not set, this for loop will crash the script on macOS for all standard test runs.
To maintain compatibility with older Bash versions under set -u, use the parameter expansion workaround: "${spec_exclude_patterns[@]+"${spec_exclude_patterns[@]}"}".
| for pattern in "${spec_exclude_patterns[@]}"; do | |
| for pattern in "${spec_exclude_patterns[@]+"${spec_exclude_patterns[@]}"}"; do |
1 participant
CI now skips twopi/twopi2.xml