Skip to content

fix(e2e): chain on_exit in constraint test EXIT traps#1091

Merged
mergify[bot] merged 1 commit intopython-wheel-build:mainfrom
jlarkin09:AIPCC-14568-e2e-trap-fix
Apr 25, 2026
Merged

fix(e2e): chain on_exit in constraint test EXIT traps#1091
mergify[bot] merged 1 commit intopython-wheel-build:mainfrom
jlarkin09:AIPCC-14568-e2e-trap-fix

Conversation

@jlarkin09
Copy link
Copy Markdown
Contributor

fix(e2e): chain on_exit in constraint test EXIT traps

The three constraint e2e tests registered their own trap ... EXIT which silently replaced the on_exit handler from common.sh. Chain on_exit in each trap and switch to single quotes so $constraints_file is expanded at exit time rather than registration time.

Closes: #1062

The three constraint e2e tests registered their own trap ... EXIT which
silently replaced the on_exit handler from common.sh. Chain on_exit in
each trap and switch to single quotes so $constraints_file is expanded
at exit time rather than registration time.

Closes: python-wheel-build#1062
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Justin Larkin <jlarkin@redhat.com>
Made-with: Cursor
@jlarkin09 jlarkin09 requested a review from a team as a code owner April 24, 2026 22:04
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 24, 2026

📝 Walkthrough

Walkthrough

Three e2e test scripts are modified to prevent their EXIT traps from silently replacing the on_exit handler registered by common.sh. Each script's trap is updated to invoke on_exit after cleaning up its temporary constraints file, and the file variable is properly quoted to handle edge cases with special characters.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: fixing e2e tests to chain the on_exit handler in EXIT traps for constraint tests.
Description check ✅ Passed The description clearly explains the issue (trap replacement), the solution (chain on_exit), the quoting fix, and links to the issue #1062.
Linked Issues check ✅ Passed All code changes in the three test files implement the exact requirements from issue #1062: chaining on_exit in EXIT traps and switching to single quotes for proper variable expansion timing.
Out of Scope Changes check ✅ Passed All changes are directly scoped to fixing the trap handler issue in the three constraint test files specified in issue #1062, with no extraneous modifications.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@mergify mergify Bot added the ci label Apr 24, 2026
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
e2e/test_bootstrap_constraints.sh (1)

14-14: Consider restoring SIGINT/SIGTERM coverage.

common.sh originally registered trap on_exit EXIT SIGINT SIGTERM. Replacing it with EXIT only (as done here and in the other two scripts) still narrows signal coverage vs. the original. In practice bash usually runs EXIT on signal-induced exit, so impact is low, but matching the original list is cheap and safer:

Proposed tweak
-trap 'rm -f "$constraints_file"; on_exit' EXIT
+trap 'rm -f "$constraints_file"; on_exit' EXIT SIGINT SIGTERM
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@e2e/test_bootstrap_constraints.sh` at line 14, The trap registration was
narrowed to only EXIT; restore the original signal coverage by adding SIGINT and
SIGTERM back to the trap invocation so that on_exit (and the cleanup of
"$constraints_file") runs on those signals too—update the trap statement that
references on_exit and "$constraints_file" to trap 'rm -f "$constraints_file";
on_exit' EXIT SIGINT SIGTERM.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@e2e/test_bootstrap_constraints.sh`:
- Line 14: The trap registration was narrowed to only EXIT; restore the original
signal coverage by adding SIGINT and SIGTERM back to the trap invocation so that
on_exit (and the cleanup of "$constraints_file") runs on those signals
too—update the trap statement that references on_exit and "$constraints_file" to
trap 'rm -f "$constraints_file"; on_exit' EXIT SIGINT SIGTERM.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: b4b014d8-d373-47df-bc9a-29b38566c46b

📥 Commits

Reviewing files that changed from the base of the PR and between 05c65ab and becfaa4.

📒 Files selected for processing (3)
  • e2e/test_bootstrap_conflicting_requirements.sh
  • e2e/test_bootstrap_constraints.sh
  • e2e/test_bootstrap_multiple_versions.sh

@mergify mergify Bot merged commit 8602966 into python-wheel-build:main Apr 25, 2026
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(e2e): trap in constraint tests replaces common.sh on_exit EXIT handler

2 participants