Repository navigation
fix(cli): wheels test preamble stops doubling _test for coreTestDataSourceName-only configs (#2489) - #2602
Conversation
…ourceName-only configs (#2489) `$resolveAppTestDataSource` in `cli/lucli/Module.cfc` matched `config/settings.cfm` with `dataSourceName\s*=\s*"([^"]*)"`. The case-insensitive matcher caught the trailing substring inside `set(coreTestDataSourceName="testappdb_test")` and re-appended `_test`, surfacing `testappdb_test_test` in the `Running app tests (...)` preamble. The matcher now uses `\bdataSourceName\b` and strips CFML comments first (matching the pattern already used by `info()`), and guards against re-appending `_test` if the resolved base already ends in `_test`. .env lookup is also anchored to start-of-line so an unrelated key whose name happens to end in `DATASOURCE_NAME` can't match by accident. Issue thread also reported `--filter` still being ignored on the latest snapshot. The CLI's `$normalizeTestFilter` + URL build trace correctly through `app-runner.cfm`'s `?directory=` regex (verified end-to-end against a local server: `--filter=models` correctly scopes to `tests.specs.models`). Most likely the user hit a stale CLI install pre-#2394. Extracted the app-runner's `?directory=` regex into `TestDirectoryResolver` alongside `TestDbResolver` so the silent-fallback path (bare `?directory=models` collapsing to `tests.specs`) is unit-testable instead of HTTP-only — if the regex ever regresses, a unit test will catch it before it ships. Also exposes `$resolveAppTestDataSource` as `public` (matching the `$`-prefix-for-internal convention used by `$normalizeTestFilter`) so the new regression specs can drive it directly.
![wheels-bot[bot]](https://avatars.githubusercontent.com/in/3657588?s=60&v=4){kind=small}
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: This PR correctly diagnoses and fixes the coreTestDataSourceName-leaks-into-dataSourceName regex match (issue #2489). The three-part fix — word-boundary anchoring, .env start-of-line anchoring, and the no-double-suffix guard — is technically sound, and the TestDirectoryResolver extraction follows the established TestDbResolver precedent cleanly. One stated fix (the .env start-of-line anchor) lacks a regression test, and the CLI specs are explicitly unverified locally. Everything else looks good. Comment (no blocking issues, one coverage gap to address before merge).
Correctness
The core logic is correct on all three axes:
Word-boundary fix (cli/lucli/Module.cfc line 5082):
var settingsMatch = reFindNoCase('\bdataSourceName\b\s*=\s*"([^"]*)"', settingsContent, 1, true);Java's \b correctly prohibits a match inside coreTestDataSourceName because the character preceding the D is t (a word character), so there is no word boundary. Verified against the Java Pattern spec.
_test doubling guard (line 5100):
return reFindNoCase("_test$", base) ? base : base & "_test";Sound. Correctly handles a user who names their app datasource myapp_test.
stripCfmlComments reuse (line 5081): Reuses the existing private helper already called by info() at line 951. Consistent and correct.
Tests
Coverage gap — the .env start-of-line anchor fix has no regression test.
The PR body explicitly claims: ".env matcher is anchored to start-of-line so MY_DATASOURCE_NAME=foo or unrelated keys whose names happen to end in DATASOURCE_NAME don't false-match." This is one of the three stated fixes. Looking at the 9 new specs in TestCommandSpec.cfc, the closest coverage is the precedence test (spec 9), which only confirms that .env wins over settings.cfm — it does not exercise the left-anchor behaviour at all.
Suggested addition alongside the existing describe block in cli/lucli/tests/specs/commands/TestCommandSpec.cfc:
it("does NOT match a .env key whose name ends in DATASOURCE_NAME", () => {
// Pre-fix, reFindNoCase("DATASOURCE_NAME=...", content) matched
// MY_DATASOURCE_NAME=foo because there was no start-of-line anchor.
var sandbox = $scaffold(envBody = "MY_DATASOURCE_NAME=wrongkey");
var localMod = new cli.lucli.Module(cwd = sandbox);
expect(localMod.$resolveAppTestDataSource(true)).toBe("(unknown)");
$tearDown(sandbox);
});CLI specs untested locally: The PR test plan has [ ] CLI test suite explicitly unchecked. The 9 new specs in TestCommandSpec.cfc use per-test new cli.lucli.Module(cwd = sandbox) via a fresh $scaffold helper rather than the suite-level fixture in beforeAll(). Whether Module.init() completes cleanly against a directory containing only config/settings.cfm and an empty vendor/wheels/ is untested outside CI. Not a blocker, but worth flagging.
AppRunnerTestDirectorySpec.cfc coverage is solid: All 13 specs in vendor/wheels/tests/specs/dispatch/ are clean WheelsTest BDD syntax, no legacy RocketUnit patterns, no # in string literals, no closure local-variable gotchas, and the specs run without network or database dependencies.
Conventions
public access on $resolveAppTestDataSource (cli/lucli/Module.cfc line 5064): Changing from private to public for testability is consistent with the $ + public pattern already used by $normalizeTestFilter at line 491. No issue.
Cross-engine
No cross-engine concerns in the framework code or AppRunnerTestDirectorySpec.cfc. TestDirectoryResolver.cfc is clean: StructKeyExists, Trim, Len, ReFindNoCase — all standard CFML with no closure gotchas and no client/application scope.
The $scaffold helper in TestCommandSpec.cfc calls directoryCreate(path, true, true) — two extra arguments. Per the cross-engine guide and issue #2567, this is Lucee-only. However, TestCommandSpec.cfc runs exclusively on the Lucee-backed LuCLI runtime, not Adobe CF, and the same three-argument form already appears in the existing beforeAll() at line 12. Not a new issue, not a blocker.
Docs
CHANGELOG entry is present in the correct [Unreleased] -> Fixed position. The entry is verbose but accurately covers all three fix dimensions.
Commits
Single commit fix(cli): wheels test preamble stops doubling _test for coreTestDataSourceName-only configs (#2489) — type fix, scope cli, subject 88 characters, not ALL-CAPS. Conforms to commitlint.config.js.
Action items before merge:
- Add the missing .env start-of-line anchor regression test (see suggested
it()above). - Verify CI green on
TestCommandSpec.cfcCLI specs — the unchecked test plan item is the main open gate.
Wheels Bot — Reviewer B (round 1)A's review is technically sound and well-evidenced. The one coverage gap A flagged (missing regression test for the SycophancyNone detected. A takes a clear position with evidence for every claim and does not issue an unconditional LGTM. False positivesNone detected.
Missed issuesNone detected. Re-scanning the diff:
A's observation that the CLI test plan item is unchecked is correctly labeled "worth flagging" but not blocking — CI is the authoritative gate for LuCLI-hosted specs. Verdict alignmentA issues COMMENT with two concrete action items (add the missing ConvergenceAligned. A correctly identified the one substantive gap: the |
- Add regression spec for the `.env` start-of-line anchor in $resolveAppTestDataSource (cli/lucli/Module.cfc:5072): confirms a key whose name ends in DATASOURCE_NAME (`MY_DATASOURCE_NAME=...`) does not false-match against the DATASOURCE_NAME extraction. cli/lucli/tests/specs/commands/TestCommandSpec.cfc. Closes A's one coverage-gap action item; B aligned in round 1. Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
Wheels Bot — Address Review (round 1)Applied consensus findings from Reviewer A and Reviewer B's convergence (round 1):
Both of A's action items now have either a test or a CI gate:
Reviewer B's round-1 critique surfaced no missed issues and no false positives, so no findings were intentionally skipped. The new commit will trigger a fresh Reviewer A run on the updated SHA. Convergence loop continues until reviewers align on |
There was a problem hiding this comment.
Wheels Bot -- Reviewer A
TL;DR: This PR resolves the coreTestDataSourceName-leaks-into-dataSourceName regex match (issue #2489) via three coordinated fixes: word-boundary anchoring, .env start-of-line anchoring, and a no-double-suffix guard. The previous Reviewer A cycle flagged one coverage gap (no regression spec for the .env anchor fix); commit ae98e145 closes it exactly. All three fix dimensions now have direct regression tests. Approve -- no blocking issues.
Correctness
All three fix axes hold up under scrutiny:
Word-boundary fix (cli/lucli/Module.cfc, $resolveAppTestDataSource):
Java word boundary correctly rejects a match inside coreTestDataSourceName because the character before d is t (a word character) -- no word boundary. Confirmed against the Java Pattern spec.
.env anchor fix (same function):
Pattern (?:^|\n)\s*DATASOURCE_NAME\s*=\s*([^\r\n]+) -- verified edge cases: MY_DATASOURCE_NAME=foo at start of file fails because after the start anchor, \s* cannot consume MY_ (no whitespace). A key on a subsequent line fails because after \n, \s* will not skip MY_. The capture group ([^\r\n]+) replacing the old (.+) also eliminates the trailing carriage-return edge case on Windows line-endings.
No-double-suffix guard: reFindNoCase("_test$", base) ? base : base & "_test" is clean and handles the myapp_test datasource naming case without ambiguity.
stripCfmlComments reuse: The settings-file path now pre-strips comments before matching, consistent with the pattern info() already uses.
Tests
The coverage gap from the previous review cycle has been addressed. Commit ae98e145 adds a spec that passes envBody = "MY_DATASOURCE_NAME=wrongkey" to $scaffold, instantiates a fresh new cli.lucli.Module(cwd=sandbox), calls $resolveAppTestDataSource(true), and asserts the result is (unknown). This directly exercises the start-of-line anchor behaviour and will catch any future regression that strips or weakens it. Logic is correct: an env file containing only MY_DATASOURCE_NAME=wrongkey leaves base empty and returns (unknown).
AppRunnerTestDirectorySpec.cfc (13 specs in vendor/wheels/tests/specs/dispatch/): BDD syntax throughout, no legacy assert() / test_ patterns, no unescaped hash characters in string literals that could crash the suite, no closure gotchas. Covers default fallback, accepted dotted paths, whitespace trimming, and all rejection cases (bare short names, namespace escapes, slashes, hyphens, trailing dots, double-dots).
Cross-engine
No cross-engine concerns in the framework-side changes. TestDirectoryResolver.cfc uses only StructKeyExists, Trim, Len, ReFindNoCase -- all standard CFML with no engine-specific gotchas.
TestCommandSpec.cfc calls directoryCreate(path, true, true) (three-argument form) in $scaffold(). Per the cross-engine guide this form is Lucee-only; however, TestCommandSpec.cfc runs exclusively on the Lucee-backed LuCLI runtime, and the same three-argument form already existed in the suite beforeAll() before this PR. Not a new issue.
Commits
The original commit (967281846) uses type fix, scope cli, 100 chars at the limit but valid. The follow-up bot commit (ae98e145) carries the required DCO trailer (Signed-off-by: claude[bot]). Minor style nit on the follow-up: type test would be more precise for a pure test addition, and the subject names the review process rather than the change -- neither is a commitlint violation.
Docs
CHANGELOG [Unreleased] -> Fixed entry is present and accurately covers all three fix dimensions.
No action items remain from the previous review cycle. The CI gate (compat-matrix and CLI suite) is the last open procedural item, expected per the test plan.
Auto-dismissed by Reviewer A guard: body is shorter than 200 characters or missing the canonical wheels-bot:review-a marker. See #2558 for context.
Wheels Bot — Reviewer A guardDetected and dismissed 1 bogus Reviewer A review(s) on commit |
Wheels Bot — Reviewer B (round 1)Review A's review is clean. The coverage gap flagged in round 1 (no regression spec for the SycophancyNone detected. A approves with specific evidence: names the new spec, explains why it closes the prior gap, and verifies the anchor logic against concrete edge cases. Not an unconditional LGTM. False positivesNone detected.
Missed issuesNone detected. Re-scan of the diff:
Verdict alignmentA's APPROVE is consistent with the findings: the one action item from round 1 was addressed directly, no new issues were introduced, and the only remaining open item is CI (procedural, expected per the test plan). ConvergenceAligned. A correctly identified and verified the closure of the sole coverage gap. No outstanding disputes, no unaddressed findings. Joint recommendation: approve and merge once the CI compat-matrix and CLI suite are green. |
Summary
Resolves #2489.
The reporter saw
Running app tests (testappdb_test_test)...when theirconfig/settings.cfmonly declaredset(coreTestDataSourceName="testappdb_test").$resolveAppTestDataSourcein cli/lucli/Module.cfc useddataSourceName\s*=\s*"([^"]*)"to scan settings.cfm — and becausereFindNoCaseis case-insensitive and the pattern had no anchors, it caught the trailing substring insidecoreTestDataSourceName, then re-appended_test. The CLI was lying about which datasource the run would use against.Fixes
$resolveAppTestDataSource(cli/lucli/Module.cfc):\bdataSourceName\b\s*=\s*"([^"]*)"and strips CFML/cfscript/tag-style comments first, matching the patterninfo()already uses one screen up the file..envmatcher is anchored to start-of-line soMY_DATASOURCE_NAME=fooor unrelated keys whose names happen to end inDATASOURCE_NAMEdon't false-match._test, don't append again. Prevents future regressions if a user names their app datasourcemyapp_test.public(matching the$-prefix-for-internal convention used by$normalizeTestFilter) so the new regression specs can drive it directly.Extracted
TestDirectoryResolverfrom app-runner.cfm alongside the existingTestDbResolver, so the directory-scope rule (^tests(\.[a-zA-Z0-9_]+)*$) is unit-testable instead of HTTP-only. The reporter's second complaint —--filter=modelsstill ignored on the latest snapshot — traces cleanly through the current CLI:$normalizeTestFilter("models")→"tests.specs.models"→?directory=tests.specs.models→ resolver accepts → TestBox scopes correctly. Verified end-to-end against a local server:curl ".../wheels/app/tests?directory=tests.specs.functional"runs only the one functional spec. The most likely explanation for the reporter's experience is a stale CLI install pre-fix(cli): normalize wheels test --filter so short names actually scope #2394; if--filterever silently regresses, the new resolver specs will catch it before it ships.Tests
AppRunnerTestDirectorySpec.cfccovering: default fallback, accepted dotted paths, deep nesting, underscore/digit segments, whitespace trimming, rejection of bare short names, rejection of paths outside thetests.*namespace, rejection of slashes / hyphens / trailing dots.TestCommandSpec.cfccovering:(unknown)fallback, app-datasource resolution with_testsuffix,useTestDB=falseshort-circuit, thecoreTestDataSourceNamecollision case (regression for this issue), commented-out settings (line / block / tag style), no-doubling guard,.envprecedence over settings.cfm.Test plan
AppRunnerTestDirectorySpecspecs.bash tools/test-cli-local.sh) — couldn't run locally (the wheels server in the worktree doesn't expose themodules.BaseModulemapping the CLI specs need), trusting CI.