Skip to content

Improve bin/verify-exercises failure handling - #221

Open
ageron wants to merge 5 commits into
exercism:mainfrom
ageron:verify-exercises-continue-and-timeout
Open

ageron wants to merge 5 commits into
exercism:mainfrom
ageron:verify-exercises-continue-and-timeout

Conversation

@ageron

@ageron ageron commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Continue after failures and summarize them at the end, support multiple exercise globs, time out frozen tests after 30 seconds, and handle Ctrl-C reliably.

Continue after failures, support multiple exercise globs, time out frozen
tests after 30 seconds, and handle Ctrl-C reliably.

@Anton-4 Anton-4 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @ageron, I did not spot any issues but Claude did 😄 :

Findings

1. The timeout and the Ctrl-C trap kill only the direct roc PID, not its descendants

run_tests:102, stop_test:67

Verified: with a stub that hangs in a child process, the timeout fires and
reports correctly, but the grandchild survives the run:

roc test timed out after 2 seconds.
--- leftover processes: ---
 2311    00:02 sleep 600

Same on Ctrl-C. This is a regression for the Ctrl-C case. On main,
roc test ran in the foreground, so the terminal's process-group SIGINT reached
the whole tree — I confirmed the old script leaves nothing behind. The PR moves
it to a background job, and bash sets SIGINT to SIG_IGN for background jobs in
a non-interactive shell; that ignore is inherited by its children, so the group
signal now reaches nothing and the trap's kill -KILL $roc_pid covers only the
top process. So the PR trades cleaner exit-status reporting (130 instead of dying
by signal) for a leaked process tree.

Fix I tested and confirmed reaps everything — enable job control so the job gets
its own pgid, then kill the group:

set -m
FORCE_COLOR=1 roc test ${opts} "${test_file}" &
roc_pid=$!
set +m
# then, in stop_test and in the watchdog:
kill -KILL -- "-${roc_pid}" 2>/dev/null || true

CI already apt-get installs coreutils, so timeout -k 5 30 roc test … would
also replace ~50 lines — but it isn't in the macOS base system, which I assume is
why you hand-rolled it. Your call.

2. if verify_exercise … silently disables set -e for the entire call

verify_exercises:170

Bash suspends errexit for the whole dynamic extent of an if condition,
including functions and subshells called from it. Confirmed:

--- called bare (errexit active):      (aborts at `false`)
--- called in if condition:            REACHED-after-false   f ok

So cp -r "${dir}/." "${tmp_dir}", cd "${tmp_dir}", the jq calls and
cp "${example_file}" "${module_file}" no longer abort — a malformed
.meta/config.json now runs tests against a half-populated temp dir instead of
failing loudly. The exercise still ends up reported as failed, so this is low
severity, but explicit || exit 1 on those setup commands would restore the old
behaviour.

3. Add required_tool roc next to required_tool jq

:24

Previously a missing/broken roc stopped at the first exercise. Now it fans out
— with PATH stripped, three exercises produced three roc: command not found
lines and listed all three as failed. Across ~90 exercises that buries the real
cause. One line fixes it.

Kill the full roc test process group on timeout or interruption, stop
immediately when exercise setup fails, and validate that roc is installed.
@ageron

ageron commented Sep 15, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @Anton-4, I worked with Sol 5.6, apparently not quite as smart as Claude. So I just switched to Terra, and it fixed the issues. 👍

@ageron
ageron requested a review from Anton-4 September 15, 2026 23:30

@Anton-4 Anton-4 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @ageron!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants