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")" +}