Uh oh!
There was an error while loading. Please reload this page.
refactor(tests): mirror the src/ module layout in tests/unit/ - #960
Merged
Conversation
Closes#957. tests/unit/ was 71 flat files with the module encoded in a filename prefix; src/ has been seventeen modules since #948. Now the tree matches: tests/unit/coverage/engine_test.sh for src/coverage/engine.sh tests/unit/runner/exec_test.sh for src/runner/exec.sh Sixteen directories named for their source module, plus tests/unit/project/ for the ten tests that cover the repo's own tooling and invariants rather than a module: the five release_*, package_json, build, bash_version, bash_compatibility and redirect_error. Naming that honestly beats filing them under a module name that would be a lie. Three classes of breakage, all found by running rather than by reading: - Fifteen tests resolve paths from their own location. Moving them one level deeper broke `$(bashunit::current_dir)/fixtures/...`. Fixtures stay shared at tests/unit/fixtures/, so those become ../fixtures/. The four find_files_recursive tests now pass a repo-relative input instead, because a ../ in the input reappears in the output they assert on. - The assert_snapshot snapshots had to move and be renamed with their test. Two of them had already been auto-recorded at the new path by the failing runs, with the wrong content: the "unsuccessful" cases record the expected text rather than the actual, which would have inverted the two tests that exist to demonstrate a mismatch. The originals were restored and diffed. - Renaming introduced three duplicate basenames, which cost 52 tests under --parallel. That is a real bashunit bug, filed as #959: results are bucketed by basename in runner/result.sh, so same-named files in different directories share a bucket and overwrite each other, silently and green. Same class as #923. Worked around here by keeping basenames unique; the fix belongs to #959. Verified by the number the whole issue was designed around: the sequential total is 1629 before and after, and --parallel --simple --strict is 1588, which is the 1585 baseline plus the three collection-guard tests from #958.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🤔 Background
Closes#957.
tests/unit/was 71 flat files with the module encoded in a filename prefix;src/has been 17 modules since #948.16 directories named for their source module, plus
tests/unit/project/for the ten tests covering the repo's own tooling and invariants (the fiverelease_*,package_json,build,bash_version,bash_compatibility,redirect_error) — naming that honestly beats filing them under a module name that would be a lie.1. Fifteen tests resolve paths from their own location. Moving them one level deeper broke
$(bashunit::current_dir)/fixtures/.... Fixtures stay shared attests/unit/fixtures/, so those become../fixtures/. The fourfind_files_recursivetests now take a repo-relative input instead — a../in the input reappears in the output they assert on.2. Snapshots had to move and be renamed with their test — and two were already poisoned. The failing runs had auto-recorded them at the new path (documented behaviour: a missing snapshot gets written). For the two "unsuccessful" cases they captured
Expected snapshotinstead ofActual snapshot text— those tests exist to demonstrate a mismatch, so keeping the auto-recorded files would have silently inverted them. Originals restored and diffed.3. Renaming introduced three duplicate basenames, which cost 52 tests under
--parallel.🐛 That third one is a real bashunit bug — #959
src/runner/result.sh:28buckets per-test results by basename, soa/foo_test.shandb/foo_test.shshare a bucket and overwrite each other. Silently, and green.--parallel --simple --strictSame class as #923, which fixed exactly this in
build.sh. It affects any user project whose tests mirror a source tree — the layout this very PR adopts. Worked around here by keeping basenames unique; the fix belongs to #959.✅ Verification
The number this whole issue was designed around:
--parallel --simple --strict: 1588 = the 1585 baseline + the 3 collection-guard tests from build(make): collect tests recursively so tests/ can mirror src/ #958make testcollects the same set as./bashunit tests/, enforced by the guardGreen:
make sa·make lint·bash build.sh bin -v→✅ Build verified ✅.