test(e2e): add test-mode failure handling tests#988
test(e2e): add test-mode failure handling tests#988shifa-khan wants to merge 1 commit intopython-wheel-build:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (5)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds end-to-end tests exercising Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 (1)
e2e/test_build_failure/setup.py (1)
2-2: Unused import:sysis imported but never used.This import can be removed as it serves no purpose in the current implementation.
🧹 Proposed fix
"""Setup script that intentionally fails during build.""" -import sys # Fail immediately when this module is imported during build raise RuntimeError("Intentional build failure for e2e testing")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/test_build_failure/setup.py` at line 2, Remove the unused import by deleting the lone "import sys" statement at the top of the module (the unused symbol "sys"); update the file so there is no unused import left and run the linter/pytest to verify no remaining import warnings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/test_mode_build.sh`:
- Around line 88-91: The conditional uses `if ! grep -q "pre-built fallback"
"$OUTDIR/bootstrap.log"` but the echo message is inverted; update the message or
the condition so it reflects the actual check: either remove the `!` to print
"INFO: Log mentions fallback (may or may not be present)" when the pattern
exists, or (preferably) keep the negation and change the echo to "INFO: Log does
not mention fallback" so the `grep -q "pre-built fallback"` check and the `echo`
text align; modify the line with `grep -q` and the `echo` accordingly.
In `@e2e/test_mode_fallback.sh`:
- Around line 66-106: Ensure the script actually fails the test when fallback
doesn't happen: validate EXIT_CODE is 0 or 1 and set pass=false otherwise;
require that either the log contains "successfully used pre-built wheel" (treat
as success) or a valid failures file exists (FAILURES_FILE with .failures length
> 0) — if neither is true set pass=false; change the informational checks around
"applying patch\|patch", "pre-built fallback", and the test mode summary grep
("test mode:" and "test mode enabled") so they set pass=false when their
expected positive evidence is missing (use the variables EXIT_CODE,
OUTDIR/bootstrap.log, FAILURES_FILE, FAILURE_COUNT and the exact grep strings
"successfully used pre-built wheel", "pre-built fallback", "test mode enabled",
and "test mode:").
---
Nitpick comments:
In `@e2e/test_build_failure/setup.py`:
- Line 2: Remove the unused import by deleting the lone "import sys" statement
at the top of the module (the unused symbol "sys"); update the file so there is
no unused import left and run the linter/pytest to verify no remaining import
warnings.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1073cfef-4117-4697-bab9-123724b65de7
📒 Files selected for processing (8)
e2e/ci_bootstrap_suite.she2e/test_build_failure/pyproject.tomle2e/test_build_failure/setup.pye2e/test_build_failure/test_build_failure/__init__.pye2e/test_mode_build.she2e/test_mode_deps.she2e/test_mode_fallback.she2e/test_mode_resolution.sh
e2e/test_mode_build.sh
Outdated
| # Initialize git repo at runtime (fixture files are committed without .git) | ||
| FIXTURE_DIR="$SCRIPTDIR/test_build_failure" | ||
| if [ ! -d "$FIXTURE_DIR/.git" ]; then | ||
| (cd "$FIXTURE_DIR" && git init -q && git add -A && git commit -q -m "init") |
There was a problem hiding this comment.
This test is failing CI
Error: https://github.com/python-wheel-build/fromager/actions/runs/23619151714/job/68794088620?pr=988#step:8:5039
Fix: The git commit needs user.name and user.email to be set. Use placeholder values ex: testuser@example.com , testuser
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
e2e/test_mode_fallback.sh (1)
79-111:⚠️ Potential issue | 🟠 MajorKey checks are still permissive and can hide regressions.
Line 79-82 and Line 106-111 still allow pass-through when expected evidence is missing, and Line 96-104 skips validation when the failures report is missing. Tighten these to fail the test on missing expected artifacts/log markers.
Proposed change
# Check 3: Look for evidence of the patch failure if grep -q "applying patch\|patch" "$OUTDIR/bootstrap.log"; then echo "Patch application was attempted" +else + echo "FAIL: Expected patch activity in bootstrap log" 1>&2 + pass=false fi @@ -FAILURES_FILE=$(find "$OUTDIR/work-dir" -name "test-mode-failures-*.json" 2>/dev/null | head -1) -if [ -n "$FAILURES_FILE" ] && [ -f "$FAILURES_FILE" ]; then +FAILURES_FILE=$(find "$OUTDIR/work-dir" -name "test-mode-failures-*.json" -print -quit 2>/dev/null) +if [ -n "$FAILURES_FILE" ] && [ -f "$FAILURES_FILE" ]; then FAILURE_COUNT=$(jq '.failures | length' "$FAILURES_FILE") if [ "$FAILURE_COUNT" -gt 0 ]; then echo "FAIL: Expected no failures (fallback should succeed), got $FAILURE_COUNT" 1>&2 jq '.failures[] | {package, failure_type, exception_type}' "$FAILURES_FILE" 1>&2 pass=false fi +else + echo "FAIL: Expected test-mode failures report file to be present" 1>&2 + pass=false fi @@ else - echo "NOTE: Test mode summary not found in log" 1>&2 + echo "FAIL: Test mode summary not found in log" 1>&2 + pass=false fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/test_mode_fallback.sh` around lines 79 - 111, The current checks are permissive; update the grep and failure-file logic so missing expected artifacts cause the script to fail by setting pass=false and emitting an error to stderr. Specifically: for the "applying patch\|patch" grep (check 3) treat absence as an error (set pass=false and echo to stderr); for the "pre-built fallback" and "successfully used pre-built wheel" checks (check 4) ensure both missing and partial matches set pass=false and print failure messages to stderr instead of only echoing success; for FAILURES_FILE handling (variable FAILURES_FILE and FAILURE_COUNT logic) treat a missing FAILURES_FILE as a test failure (set pass=false and echo an error) rather than skipping validation; and for the "test mode:" grep (check 6) treat a missing summary as a failure (set pass=false and echo to stderr) instead of a note. Ensure all error messages go to stderr and the global pass variable is updated consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/test_mode_fallback.sh`:
- Around line 12-14: Add nounset handling by enabling -u in the script: after
sourcing common.sh (the line "source \"$SCRIPTDIR/common.sh\""), add an explicit
set -u (or set -euo pipefail) so the script treats unset variables as errors;
this ensures test_mode_fallback.sh honors nounset even if common.sh doesn't set
it.
---
Duplicate comments:
In `@e2e/test_mode_fallback.sh`:
- Around line 79-111: The current checks are permissive; update the grep and
failure-file logic so missing expected artifacts cause the script to fail by
setting pass=false and emitting an error to stderr. Specifically: for the
"applying patch\|patch" grep (check 3) treat absence as an error (set pass=false
and echo to stderr); for the "pre-built fallback" and "successfully used
pre-built wheel" checks (check 4) ensure both missing and partial matches set
pass=false and print failure messages to stderr instead of only echoing success;
for FAILURES_FILE handling (variable FAILURES_FILE and FAILURE_COUNT logic)
treat a missing FAILURES_FILE as a test failure (set pass=false and echo an
error) rather than skipping validation; and for the "test mode:" grep (check 6)
treat a missing summary as a failure (set pass=false and echo to stderr) instead
of a note. Ensure all error messages go to stderr and the global pass variable
is updated consistently.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c32c822f-a251-47b0-bfee-1a87bcea06fd
📒 Files selected for processing (8)
e2e/ci_bootstrap_suite.she2e/test_build_failure/pyproject.tomle2e/test_build_failure/setup.pye2e/test_build_failure/test_build_failure/__init__.pye2e/test_mode_build.she2e/test_mode_deps.she2e/test_mode_fallback.she2e/test_mode_resolution.sh
✅ Files skipped from review due to trivial changes (5)
- e2e/test_build_failure/setup.py
- e2e/ci_bootstrap_suite.sh
- e2e/test_mode_deps.sh
- e2e/test_build_failure/pyproject.toml
- e2e/test_build_failure/test_build_failure/init.py
🚧 Files skipped from review as they are similar to previous changes (2)
- e2e/test_mode_resolution.sh
- e2e/test_mode_build.sh
| SCRIPTDIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )" | ||
| source "$SCRIPTDIR/common.sh" | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether strict mode is currently defined only in sourced helpers.
fd -i '^common\.sh$' --exec sh -c '
for f in "$@"; do
echo "== $f =="
rg -n "set -[a-zA-Z]*e|set -o pipefail|trap " "$f" || true
done
' sh {}Repository: python-wheel-build/fromager
Length of output: 155
🏁 Script executed:
# Check test_mode_fallback.sh for unset variables and current strict mode usage
cat e2e/test_mode_fallback.sh | head -80Repository: python-wheel-build/fromager
Length of output: 2613
🏁 Script executed:
# Also verify what line 13 contains to confirm sourcing order
sed -n '12,15p' e2e/test_mode_fallback.shRepository: python-wheel-build/fromager
Length of output: 216
Consider adding explicit set -u for nounset handling.
Strict mode (set -e and set -o pipefail) is already enabled via common.sh, but this script lacks the -u (nounset) flag. Adding explicit set -euo pipefail would catch any unset variable references:
Proposed change
SCRIPTDIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"
source "$SCRIPTDIR/common.sh"
+set -euo pipefail📝 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.
| SCRIPTDIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )" | |
| source "$SCRIPTDIR/common.sh" | |
| SCRIPTDIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )" | |
| source "$SCRIPTDIR/common.sh" | |
| set -euo pipefail |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/test_mode_fallback.sh` around lines 12 - 14, Add nounset handling by
enabling -u in the script: after sourcing common.sh (the line "source
\"$SCRIPTDIR/common.sh\""), add an explicit set -u (or set -euo pipefail) so the
script treats unset variables as errors; this ensures test_mode_fallback.sh
honors nounset even if common.sh doesn't set it.
There was a problem hiding this comment.
set -u is already enabled via common.sh
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "set -" e2e/common.sh | head -20
Length of output: 58
@shifa-khan, you're right — common.sh already has set -u at line 5 (along with set -e, set -o pipefail, and set -x). My suggestion was unnecessary. Thanks for the correction!
✏️ Learnings added
Learnt from: shifa-khan
Repo: python-wheel-build/fromager PR: 988
File: e2e/test_mode_fallback.sh:12-14
Timestamp: 2026-03-30T19:40:19.951Z
Learning: In the python-wheel-build/fromager repository, `e2e/common.sh` already sets strict mode with `set -e`, `set -u`, `set -o pipefail`, and `set -x` (lines 3–6). Do not suggest adding `set -euo pipefail` to individual e2e test scripts that source `common.sh`, as they already inherit these settings.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
e2e/test_mode_build.sh (1)
55-55: Make failure-file discovery pipefail-safe.Line 55 uses
find ... | head -1, which is brittle underset -o pipefail(upstreamfindcan exit on SIGPIPE). Usefind ... -print -quitinstead.Suggested patch
-FAILURES_FILE=$(find "$OUTDIR/work-dir" -name "test-mode-failures-*.json" 2>/dev/null | head -1) +FAILURES_FILE=$(find "$OUTDIR/work-dir" -name "test-mode-failures-*.json" -print -quit 2>/dev/null)As per coding guidelines,
e2e/**/*.sh: Ensure set -euo pipefail is used.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/test_mode_build.sh` at line 55, The current FAILURES_FILE assignment uses a pipeline `find ... | head -1` which is unsafe under set -o pipefail; modify the FAILURES_FILE assignment (the line that sets the FAILURES_FILE variable) to use find's builtin exit-after-first-match flags (e.g., `-print -quit`) and keep the stderr redirection (2>/dev/null) instead of piping to head, and ensure the script enables strict mode (`set -euo pipefail`) near the top of the file so the shell honors pipefail semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/test_mode_build.sh`:
- Around line 18-23: The fixture .git is created without teardown and may
persist between runs; add a trap to remove "$FIXTURE_DIR/.git" when the script
exits if this script created it. Implement a flag (e.g., CREATED_FIXTURE_GIT=1)
set when you run git init in the block that checks "$FIXTURE_DIR/.git", register
a trap like trap '[[ $CREATED_FIXTURE_GIT ]] && rm -rf "$FIXTURE_DIR/.git"'
EXIT, and after the init block ensure the repository has a valid HEAD (for
example, check git rev-parse --verify HEAD or test for "$FIXTURE_DIR/.git/HEAD"
and if missing perform a commit in FIXTURE_DIR to create HEAD using git add -A
&& git commit -q -m "init").
- Around line 12-14: The script relies on shell strict mode coming implicitly
from common.sh; explicitly enable strict mode to avoid fragile coupling by
adding set -euo pipefail after sourcing common.sh so the script enforces failure
on errors/unset variables and pipe failures even if common.sh changes; modify
the file that defines SCRIPTDIR and sources common.sh (the lines using SCRIPTDIR
and source "$SCRIPTDIR/common.sh") to include an immediate set -euo pipefail
following the source statement.
---
Nitpick comments:
In `@e2e/test_mode_build.sh`:
- Line 55: The current FAILURES_FILE assignment uses a pipeline `find ... | head
-1` which is unsafe under set -o pipefail; modify the FAILURES_FILE assignment
(the line that sets the FAILURES_FILE variable) to use find's builtin
exit-after-first-match flags (e.g., `-print -quit`) and keep the stderr
redirection (2>/dev/null) instead of piping to head, and ensure the script
enables strict mode (`set -euo pipefail`) near the top of the file so the shell
honors pipefail semantics.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fcca1ffb-2624-40a2-9920-45a60f65111b
📒 Files selected for processing (8)
e2e/ci_bootstrap_suite.she2e/test_build_failure/pyproject.tomle2e/test_build_failure/setup.pye2e/test_build_failure/test_build_failure/__init__.pye2e/test_mode_build.she2e/test_mode_deps.she2e/test_mode_fallback.she2e/test_mode_resolution.sh
✅ Files skipped from review due to trivial changes (6)
- e2e/test_build_failure/test_build_failure/init.py
- e2e/ci_bootstrap_suite.sh
- e2e/test_build_failure/setup.py
- e2e/test_build_failure/pyproject.toml
- e2e/test_mode_fallback.sh
- e2e/test_mode_deps.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/test_mode_resolution.sh
| SCRIPTDIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )" | ||
| source "$SCRIPTDIR/common.sh" | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify strict-mode declarations in this test and shared helper.
rg -n 'set -euo pipefail|set \+e|set -e' e2e/test_mode_build.sh e2e/common.shRepository: python-wheel-build/fromager
Length of output: 159
🏁 Script executed:
head -50 e2e/test_mode_build.sh | cat -nRepository: python-wheel-build/fromager
Length of output: 2174
🏁 Script executed:
head -10 e2e/common.sh | cat -nRepository: python-wheel-build/fromager
Length of output: 335
🏁 Script executed:
rg -n '\|' e2e/test_mode_build.shRepository: python-wheel-build/fromager
Length of output: 592
🏁 Script executed:
git ls-files e2e/ | grep '\.sh$' | head -3Repository: python-wheel-build/fromager
Length of output: 153
Add explicit strict mode to this script.
The strict mode settings (set -euo pipefail) come from common.sh, but sourcing a shared helper for shell options creates fragile implicit coupling. Add an explicit set -euo pipefail after the source to ensure robustness against changes to common.sh.
Suggested fix
SCRIPTDIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"
source "$SCRIPTDIR/common.sh"
+set -euo pipefail📝 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.
| SCRIPTDIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )" | |
| source "$SCRIPTDIR/common.sh" | |
| SCRIPTDIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )" | |
| source "$SCRIPTDIR/common.sh" | |
| set -euo pipefail |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/test_mode_build.sh` around lines 12 - 14, The script relies on shell
strict mode coming implicitly from common.sh; explicitly enable strict mode to
avoid fragile coupling by adding set -euo pipefail after sourcing common.sh so
the script enforces failure on errors/unset variables and pipe failures even if
common.sh changes; modify the file that defines SCRIPTDIR and sources common.sh
(the lines using SCRIPTDIR and source "$SCRIPTDIR/common.sh") to include an
immediate set -euo pipefail following the source statement.
There was a problem hiding this comment.
set -u is already enabled via common.sh
There was a problem hiding this comment.
@shifa-khan, you're right — I already have this recorded in my knowledge base: common.sh enables set -e, set -u, set -o pipefail, and set -x, and sourced scripts inherit those settings, making the explicit addition redundant. My suggestion was incorrect. Apologies for the noise!
🧠 Learnings used
Learnt from: shifa-khan
Repo: python-wheel-build/fromager PR: 988
File: e2e/test_mode_fallback.sh:12-14
Timestamp: 2026-03-30T19:40:19.951Z
Learning: In this repo’s e2e shell scripts (e.g., under e2e/), don’t add redundant strict-mode flags like `set -e`, `set -u`/`-o pipefail`, or `set -x` if the script sources `e2e/common.sh`. `e2e/common.sh` already enables strict mode (`set -e`, `set -u`, `set -o pipefail`, `set -x`), and sourced scripts inherit those settings, so adding them again should not be suggested during review.
Add e2e tests for --test-mode bootstrapping failures as requested in issue Tests cover: - Top-level resolution failure (non-existent package) - Secondary dependency resolution failure (constrained pbr) - Build failure without prebuilt fallback (local git fixture) - Build failure with prebuilt fallback (broken patch on setuptools) Co-Authored-By: Claude <claude@anthropic.com> Closes: python-wheel-build#895
2 participants
Add e2e tests for --test-mode bootstrapping failures as requested in issue
Tests cover:
Closes: #895