From 2b45bb96d9166348a60fa1ce1c17298cc2b27e1e Mon Sep 17 00:00:00 2001 From: Chemaclass Date: Fri, 14 Aug 2026 18:55:08 +0200 Subject: [PATCH] fix(learn): gate lessons on the learner's code, not the hint comments Every lesson checks "did the learner use this API", but the template each lesson writes carries that name in its own TODO/Hint lines. A plain grep matched the hint, so all nine gates passed on an untouched file -- and a lesson could be completed with an unrelated passing assertion: # TODO: Check that message contains "bashunit" # Hint: assert_contains "substring" "$message" assert_same 1 1 gate: 1 match (on the comment), run: exit 0, "Lesson 2 completed" -- without ever using assert_contains, assert_matches or assert_not_empty, which is the whole lesson. One shared helper counts matches outside comment lines, used by all nine gates. The guard is scoped to gates that name an assertion. The others look for `function set_up()` or `function data_provider_`, which the template declares in code as a skeleton on purpose -- those cannot distinguish template from solution and are not meant to; --fail-on-risky (#1257) is what makes an unfilled skeleton fail. An earlier version asserted the invariant over every gate and passed only because it stopped after the first file, where it would have been false for five of them. It now also asserts it compared something. Closes #1258 --- CHANGELOG.md | 1 + src/learn/lessons/assertions.sh | 6 +-- src/learn/lessons/basics.sh | 2 +- src/learn/lessons/challenge.sh | 4 +- src/learn/lessons/data_providers.sh | 2 +- src/learn/lessons/exit_codes.sh | 2 +- src/learn/lessons/functions.sh | 2 +- src/learn/lessons/lifecycle.sh | 4 +- src/learn/lessons/mocking.sh | 2 +- src/learn/lessons/spies.sh | 2 +- src/learn/session.sh | 17 +++++++ tests/unit/project/learn_templates_test.sh | 58 +++++++++++++++++++++- 12 files changed, 87 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9bfa7591..3779e9b0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ - `--verbose` warns on Bash 3.x that coverage does not count lines run inside a subshell, so a percentage that reads lower there than on Bash 4+ explains itself (#1112) ### Changed +- `bashunit learn` checks the learner's **code**, not the hint comments it printed. Every lesson gates on "did you use this API", but the template each lesson writes carries that name in its own TODO/Hint lines, so all nine gates passed on an untouched file — and a lesson could be completed with an unrelated passing assertion, never using what it teaches (#1258) - `bashunit learn` generates starter files that are valid bash. A function body of only TODO comments is a syntax error, so 7 of the 10 lesson templates did not parse and a learner running the lesson saw `syntax error near unexpected token '}'` from a file bashunit itself wrote. Lesson verification also runs with `--fail-on-risky`, so an untouched template no longer completes a lesson — a test with no assertions is risky, and risky exits 0 (#1256) - The coverage HTML report handles filenames containing `|`, `<` or `&`. Rows were joined and split on `|`, so `src/a|b.sh` truncated to `a` in the index while the coverage numbers stayed right, and filenames went into the markup unescaped — `ac.sh` was parsed as a tag and leaked into the document. The escaper now lives in `src/util/str.sh`, shared with the test report instead of duplicated (#1254) - The HTML report summary counts risky and flaky tests. A run with a risky test showed `2 total` against categories summing to 1, with nothing on the page saying where the second test went — the row was there with its own CSS class, but the summary never counted it. The console and the Markdown report both report it (#1252) diff --git a/src/learn/lessons/assertions.sh b/src/learn/lessons/assertions.sh index e80345c5..42528165 100644 --- a/src/learn/lessons/assertions.sh +++ b/src/learn/lessons/assertions.sh @@ -75,9 +75,9 @@ function test_multiple_assertions() { return 1 fi - if [ "$("$GREP" -c "assert_contains" "$test_file" || true)" -eq 0 ] || - [ "$("$GREP" -c "assert_matches" "$test_file" || true)" -eq 0 ] || - [ "$("$GREP" -c "assert_not_empty" "$test_file" || true)" -eq 0 ]; then + if [ "$(bashunit::learn::count_in_code "$test_file" "assert_contains")" -eq 0 ] || + [ "$(bashunit::learn::count_in_code "$test_file" "assert_matches")" -eq 0 ] || + [ "$(bashunit::learn::count_in_code "$test_file" "assert_not_empty")" -eq 0 ]; then echo "${_BASHUNIT_COLOR_FAILED}Your test should use all three assertion types${_BASHUNIT_COLOR_DEFAULT}" read -p "Press Enter to continue..." -r return 1 diff --git a/src/learn/lessons/basics.sh b/src/learn/lessons/basics.sh index aaaac59a..a3708e81 100644 --- a/src/learn/lessons/basics.sh +++ b/src/learn/lessons/basics.sh @@ -58,7 +58,7 @@ function test_bashunit_works() { fi # Check if file contains assert_same - if [ "$("$GREP" -c "assert_same" "$test_file" || true)" -eq 0 ]; then + if [ "$(bashunit::learn::count_in_code "$test_file" "assert_same")" -eq 0 ]; then echo "${_BASHUNIT_COLOR_FAILED}Your test should use assert_same${_BASHUNIT_COLOR_DEFAULT}" read -p "Press Enter to continue..." -r return 1 diff --git a/src/learn/lessons/challenge.sh b/src/learn/lessons/challenge.sh index d115ea30..a97133d6 100644 --- a/src/learn/lessons/challenge.sh +++ b/src/learn/lessons/challenge.sh @@ -101,12 +101,12 @@ function test_backup_failure_when_source_missing() { local -a missing_components=() local missing_components_count=0 - if [ "$("$GREP" -c "function set_up()" "$test_file" || true)" -eq 0 ]; then + if [ "$(bashunit::learn::count_in_code "$test_file" "function set_up()")" -eq 0 ]; then missing_components[missing_components_count]="set_up function" missing_components_count=$((missing_components_count + 1)) fi - if [ "$("$GREP" -c "function tear_down()" "$test_file" || true)" -eq 0 ]; then + if [ "$(bashunit::learn::count_in_code "$test_file" "function tear_down()")" -eq 0 ]; then missing_components[missing_components_count]="tear_down function" missing_components_count=$((missing_components_count + 1)) fi diff --git a/src/learn/lessons/data_providers.sh b/src/learn/lessons/data_providers.sh index f187e8ac..c8412270 100644 --- a/src/learn/lessons/data_providers.sh +++ b/src/learn/lessons/data_providers.sh @@ -112,7 +112,7 @@ function test_invalid_emails() { return 1 fi - if [ "$("$GREP" -c "function data_provider_" "$test_file" || true)" -eq 0 ]; then + if [ "$(bashunit::learn::count_in_code "$test_file" "function data_provider_")" -eq 0 ]; then echo "${_BASHUNIT_COLOR_FAILED}Your test should define data provider functions${_BASHUNIT_COLOR_DEFAULT}" read -p "Press Enter to continue..." -r return 1 diff --git a/src/learn/lessons/exit_codes.sh b/src/learn/lessons/exit_codes.sh index f14b12d5..1cbcfa5a 100644 --- a/src/learn/lessons/exit_codes.sh +++ b/src/learn/lessons/exit_codes.sh @@ -113,7 +113,7 @@ function test_missing_file_returns_127() { fi local _exit_assert_pattern="assert_successful_code\|assert_exit_code\|assert_general_error" - if [ "$("$GREP" -c "$_exit_assert_pattern" "$test_file" || true)" -eq 0 ]; then + if [ "$(bashunit::learn::count_in_code "$test_file" "$_exit_assert_pattern")" -eq 0 ]; then echo "${_BASHUNIT_COLOR_FAILED}Your test should use exit code assertions${_BASHUNIT_COLOR_DEFAULT}" read -p "Press Enter to continue..." -r return 1 diff --git a/src/learn/lessons/functions.sh b/src/learn/lessons/functions.sh index 9eec1b00..52a3d813 100644 --- a/src/learn/lessons/functions.sh +++ b/src/learn/lessons/functions.sh @@ -90,7 +90,7 @@ function test_add_negative_numbers() { return 1 fi - if [ "$("$GREP" -c "source" "$test_file" || true)" -eq 0 ]; then + if [ "$(bashunit::learn::count_in_code "$test_file" "source")" -eq 0 ]; then echo "${_BASHUNIT_COLOR_FAILED}Your test should source the calculator.sh file${_BASHUNIT_COLOR_DEFAULT}" read -p "Press Enter to continue..." -r return 1 diff --git a/src/learn/lessons/lifecycle.sh b/src/learn/lessons/lifecycle.sh index 3865abd7..7840da04 100644 --- a/src/learn/lessons/lifecycle.sh +++ b/src/learn/lessons/lifecycle.sh @@ -89,8 +89,8 @@ function test_file_has_content() { return 1 fi - if [ "$("$GREP" -c "function set_up()" "$test_file" || true)" -eq 0 ] || - [ "$("$GREP" -c "function tear_down()" "$test_file" || true)" -eq 0 ]; then + if [ "$(bashunit::learn::count_in_code "$test_file" "function set_up()")" -eq 0 ] || + [ "$(bashunit::learn::count_in_code "$test_file" "function tear_down()")" -eq 0 ]; then echo "${_BASHUNIT_COLOR_FAILED}Your test should define set_up and tear_down functions${_BASHUNIT_COLOR_DEFAULT}" read -p "Press Enter to continue..." -r return 1 diff --git a/src/learn/lessons/mocking.sh b/src/learn/lessons/mocking.sh index e6c61797..c2fbd4e0 100644 --- a/src/learn/lessons/mocking.sh +++ b/src/learn/lessons/mocking.sh @@ -99,7 +99,7 @@ function test_system_info_on_macos() { return 1 fi - if [ "$("$GREP" -c "mock" "$test_file" || true)" -eq 0 ]; then + if [ "$(bashunit::learn::count_in_code "$test_file" "mock")" -eq 0 ]; then echo "${_BASHUNIT_COLOR_FAILED}Your test should use mock${_BASHUNIT_COLOR_DEFAULT}" read -p "Press Enter to continue..." -r return 1 diff --git a/src/learn/lessons/spies.sh b/src/learn/lessons/spies.sh index 9e315b87..71436d86 100644 --- a/src/learn/lessons/spies.sh +++ b/src/learn/lessons/spies.sh @@ -110,7 +110,7 @@ function test_deploy_calls_docker_twice() { return 1 fi - if [ "$("$GREP" -c "spy" "$test_file" || true)" -eq 0 ]; then + if [ "$(bashunit::learn::count_in_code "$test_file" "spy")" -eq 0 ]; then echo "${_BASHUNIT_COLOR_FAILED}Your test should use spy${_BASHUNIT_COLOR_DEFAULT}" read -p "Press Enter to continue..." -r return 1 diff --git a/src/learn/session.sh b/src/learn/session.sh index b3a28cef..d3c0e11d 100644 --- a/src/learn/session.sh +++ b/src/learn/session.sh @@ -46,6 +46,23 @@ function bashunit::learn::create_example_file() { ## # Run a lesson test and check results ## +## +# Counts matches of $2 in $1, ignoring comment lines. +# +# Lessons gate on "did the learner use this API", but the template each lesson +# writes carries the API name in its own TODO/Hint comments -- so a plain grep +# matched the hint and the gate passed before any work was done. All nine gates +# were satisfied by their own untouched template, which also let a learner +# complete a lesson with an unrelated passing assertion (#1258). +# Arguments: $1 - file to search, $2 - pattern +## +function bashunit::learn::count_in_code() { + local file=$1 + local pattern=$2 + + "$GREP" -v '^[[:space:]]*#' "$file" | "$GREP" -c "$pattern" || true +} + function bashunit::learn::run_lesson_test() { local test_file=$1 local lesson_number=$2 diff --git a/tests/unit/project/learn_templates_test.sh b/tests/unit/project/learn_templates_test.sh index 65355aa6..624ee8ab 100644 --- a/tests/unit/project/learn_templates_test.sh +++ b/tests/unit/project/learn_templates_test.sh @@ -29,9 +29,10 @@ function set_up_before_script() { # line in these files sits in the `cat <<'EOF'` lesson text, outside any # template. Checked against a perl extractor: both find 10 templates and agree # on which parse. -function _lesson_templates() { # $1 = lessons dir +function _lesson_templates() { # $1 = a lesson file, or a directory of them local file - for file in "$1"/*.sh; do + for file in "$1" "$1"/*.sh; do + [ -f "$file" ] || continue awk ' index($0, "local template=\047") { intpl = 1 @@ -100,3 +101,56 @@ function test_the_scan_flags_a_template_that_does_not_parse() { function test_the_lesson_runner_fails_a_test_with_no_assertions() { assert_file_contains "$ROOT_DIR/src/learn/session.sh" "--fail-on-risky" } + +# Lessons gate on "did the learner use this API", but each template carries the +# API name in its own TODO/Hint comments. A plain grep matched the hint and +# passed before any work was done, which let a learner finish a lesson with an +# unrelated passing assertion (#1258). +# +# Scoped to the gates that name an *assertion*. The others look for +# `function set_up()` or `function data_provider_`, which the template declares +# as a skeleton in code on purpose -- those gates cannot distinguish template +# from solution and are not meant to; the test run carries that signal, and +# --fail-on-risky is what makes an unfilled skeleton fail. +# +# An earlier version asserted the invariant over *every* gate and passed only +# because it stopped after the first file. It would have been false for five of +# them. +function test_no_assertion_gate_is_satisfied_by_its_own_template() { + local offenders="" + local file pattern tpl tmp checked=0 + tmp="$(bashunit::temp_file)" + + for file in "$ROOT_DIR"/src/learn/lessons/*.sh; do + while IFS= read -r pattern; do + case "$pattern" in + assert_*) ;; + *) continue ;; + esac + + while IFS= read -r -d '' tpl; do + printf '%s\n' "$tpl" >"$tmp" + checked=$((checked + 1)) + if [ "$(bashunit::learn::count_in_code "$tmp" "$pattern")" -ne 0 ]; then + offenders="$offenders$(basename "$file"):$pattern " + fi + done < <(_lesson_templates "$file") + done < <("$GREP" -oE 'count_in_code "\$test_file" "[^"]+"' "$file" | + sed -E 's/.*"\$test_file" "(.*)"/\1/') + done + + assert_empty "$offenders" + # Guards the guard: zero comparisons would make the assertion above vacuous. + assert_greater_than 0 "$checked" +} + +# The helper is what makes that true, so pin it directly: a hint in a comment +# must not count, the same call in code must. +function test_count_in_code_ignores_comments() { + local f + f="$(bashunit::temp_file)" + printf '%s\n' '# Hint: assert_contains "x" "$y"' 'assert_same 1 1' >"$f" + + assert_same "0" "$(bashunit::learn::count_in_code "$f" "assert_contains")" + assert_same "1" "$(bashunit::learn::count_in_code "$f" "assert_same")" +}