fix: mxcli test finds test files in subfolders (closes #1363)

[stagknee]

Closes #1363.

What it does

• testrunner.ParseTestDir walks the folder tree (filepath.WalkDir, lexical order) through a small helper, walkTestTree, instead of reading one level and skipping directories. mxcli test tests now runs tests/<Module>/*.test.mdl.
• Dot-folders below the root (.git, .mxcli, …) are skipped. The root itself is always read, so mxcli test . still works. A symlinked root is resolved first, as os.ReadDir effectively did. Symlinked subfolders are not followed, which avoids cycles.
• FilesRead, FileErrors and the suite name are unchanged.
• The "no tests found" hint (emptySuiteError) uses the same walk, so it still names a misnamed file in a subfolder.
• I updated the "Run all tests in a directory" wording in cmd_test_run.go and in docs-site/src/tools/{testing,running-tests}.md, plus a CHANGELOG entry (Unreleased → Fixed) and a finding in findings/cmd-mxcli/.

Evidence

• Test-first: parser_dir_test.go adds 4 tests. Against the old ParseTestDir:

  • the nested-layout test got FilesRead = 1, want 3;
  • the broken-file-in-subfolder test got FilesRead = 0, want 3;
  • the hint test failed with "nested skipped candidate not named".

  All pass with the fix. The fourth test, "hidden root is still read", guards against skipping too much.

• Manual, Windows 11: with tests/Sales/math.test.mdl, mxcli test tests --list prints Found 1 test(s) (it was Found 0 on v0.24.0).

• go test ./cmd/mxcli/testrunner/ on Windows: the only failures are the three pre-existing handshake tests (Five tests fail on Windows (POSIX file-mode assertions + a path assumption) — never run there until now #897 family, and On Windows, mxcli test --attach refuses a running app: the handshake liveness check uses Signal(0), which never succeeds there #1284/fix: detect a live process on Windows in the test and dev-loop handshakes (closes #1284) #1285), and none is newly failing. go vet is clean for windows, linux and darwin.

• Linux (golang:1.26.6-bookworm, ANTLR 4.13.2, the same as CI): make grammar build test check-mdl check-findings check-conformance check-migration-reference lint-go all pass.

• No mdl-examples/bug-tests script, since the bug is in test-file discovery, not MDL execution.

Update (rebased on v0.27.0, second commit)

A second report on #1363 asked for a positive-control folder to stay out of the default run, and for a 0-test result not to look like a pass. The second commit, fix: mxcli test skips folders starting with _ and says why --list found nothing, adds the following.

• Folders starting with _ below the root are skipped, like dot-folders. This follows the Go toolchain convention. The folder you name is always read:

  • mxcli test tests/ skips tests/unit/_control/;
  • mxcli test tests/unit/_control/ runs it.

• Skipped _ folders are named in the output, so nothing disappears silently:

  Skipped folder(s) starting with _: tests/unit/_control (name one explicitly to run it)
  

  The line comes after the Found N test(s) line of a run, and at the end of a --list. Dot-folders are not reported. JUnit and JSON output are unchanged.

• --list with 0 tests says why on stderr. It gives the same explanation a real run gives, plus the _ folders it skipped. Its exit code stays 0, because --list is an inspection command.

• A real run with 0 tests still exits 1, as it has since v0.20.

• ListTests gains a hints io.Writer argument. Its one caller passes os.Stderr.

Evidence:

• 8 new tests in parser_dir_test.go. Against the first commit's behaviour, 7 fail; the eighth (TestListTestsWithTestsWritesNoHint) is the control and passes both ways. All pass with the change.
• go vet and gofmt are clean. Across cmd/mxcli/testrunner and cmd/mxcli, the failing tests on Windows are the same as on main: the 0600-mode and handshake family.
• A second findings record is in findings/cmd-mxcli/.
• Linux (golang:1.26.6-bookworm, ANTLR 4.13.2): make grammar build test check-mdl check-findings check-conformance check-migration-reference lint-go all pass.

Real run on Windows, with suites only in onlyctl/unit/_control/:

$ mxcli test onlyctl/ --list
Found 0 test(s):
  Skipped folder(s) starting with _: onlyctl/unit/_control (name one explicitly to run it)
no tests found: a test file is named *.test.mdl or *.test.md, and holds one or more `/** @test ... */` blocks      <- stderr
Folders starting with _ are skipped below the folder you name; name one to run it:                                 <- stderr
  onlyctl/unit/_control                                                                                            <- stderr
[exit 0]

$ mxcli test onlyctl/ -p app.mpr
  Found 0 test(s) in 0 file(s)
  Skipped folder(s) starting with _: onlyctl/unit/_control (name one explicitly to run it)
Error: no tests found: …
[exit 1]

🤖 Generated with Claude Code

[github-actions]

AI Code Review

What Looks Good

This PR successfully fixes the issue where mxcli test tests wasn't finding test files in subfolders. The solution is well-implemented and thoroughly tested:

Test Coverage:

• Added 4 comprehensive tests in parser_dir_test.go covering nested layouts, broken files in subfolders, hint behavior, and hidden root handling
• All tests follow test-first methodology (were failing before fix, passing after)
• Manual verification on Windows 11 confirmed the fix works

Implementation Quality:

• Replaced shallow directory reading with proper recursive walk using filepath.WalkDir
• Correctly handles edge cases: symlinked roots (resolved), symlinked subfolders (not followed to avoid cycles), dot-folders (skipped below root)
• Maintains lexical walk order for consistent test ordering
• Proper error handling: individual file parse errors don't halt the walk
• Updated the "no tests found" hint to use the same walk logic for consistency

Documentation & Process:

• Updated command examples and documentation to reflect subfolder support
• Added appropriate CHANGELOG entry
• Added a bug finding file as required by the project's process
• No extraneous changes - stays focused on the single issue

Robustness:

• Uses standard Go libraries (io/fs, filepath) appropriately
• Follows project conventions for error wrapping and context
• No security concerns (no command injection, XSS risks, etc.)
• File I/O is appropriate for this initialization-time operation

The fix addresses the exact issue described in #1363 and maintains backward compatibility (e.g., mxcli test . still works).

Recommendation

Approve - The PR is ready to merge. It correctly fixes the reported issue with comprehensive tests, proper documentation, and follows all project practices. No changes are needed.

---

Automated review via OpenRouter (Nemotron Super 120B) — workflow source

[ako]

Thanks — the fix is right, and the evidence holds up: the four new tests pass, and with the old parser.go/runner.go put back, three of them fail (HiddenRootIsStillRead is a guard that also passes on the old code, which is fine). One blocker before merge, plus a few smaller notes.

Blocker: one unreadable subfolder now makes the whole run fail

In walkTestTree (cmd/mxcli/testrunner/parser.go) the WalkDir callback does if err != nil { return err }. The old code never opened subfolders, so this couldn't happen before. Now any unreadable folder below the root (a locked folder, a root-owned folder in a mounted volume) makes ParseTestDir return a hard error, and the good tests in the other folders are dropped too. That contradicts the PR's own claim that "a file that cannot be parsed is still reported on its own and does not stop the others".

Reproduced as an unprivileged user (setpriv --reuid=65534 …), with tests/Sales/a.test.mdl next to a chmod 000 folder tests/locked/:

reading test directory: open …/locked: permission denied

No tests ran. The suite can't catch this, because CI and devcontainers run as root, and root ignores chmod 000.

Suggested fix: fail hard only on the root, and treat an error below it as that entry's failure:

if err != nil {
    if path == root {
        return err
    }
    onErr(path, err) // ParseTestDir: append to suite.FileErrors
    return nil       // WalkDir already skips the unreadable dir
}

The run still goes red through FileErrors, but every readable test is kept. emptySuiteError can ignore these errors. Please add a test that runs as non-root, skipping when os.Geteuid() == 0.

Worth considering

• mxcli test . from the project root now searches the whole project. That includes deployment/, node_modules/, javasource/, vendorlib/, mprcontents/ and mdl-examples/, so it can be slow and picks up any stray *.test.mdl. Either skip a few well-known non-test folders, or say in the docs that discovery is fully recursive.

Minor

• Symlinked subfolders are silently ignored. If tests/Sales is a symlink, WalkDir reports it as a file and its tests vanish with no message. Skipping them is a reasonable way to avoid symlink loops, but please mention it in running-tests.md.
• A symlinked root shows the resolved path in errors, not the path the user typed.
• watch.go uses its own walk, and it does enter hidden folders. Routing testFilesMTime through walkTestTree would keep the two discovery rules from drifting apart.

---

Generated by Claude Code

[github-actions]

AI Code Review

Review Summary

This PR improves test discovery in mxcli test to recursively search subdirectories while skipping dot-folders (.git, .mxcli) and underscore-prefixed folders (following Go toolchain convention). It adds proper reporting of skipped folders and enhances error messaging when no tests are found.

Critical Issues

None found.

Moderate Issues

None found.

Minor Issues

• Error handling in emptySuiteError: The call to walkTestTree ignores its error return value (dirs, _ := walkTestTree(...)). While consistent with existing patterns in the codebase (where directory walk errors are often ignored), it would be safer to at least log unexpected errors. However, given that:
  1. The directory was already validated via os.Stat
  2. The existing code had similar error-ignoring behavior
  3. The error would only occur in edge cases (permission issues, etc.)
     This is acceptable but worth noting.

What Looks Good

• Test-first approach: Comprehensive test suite added (parser_dir_test.go) with 11 test cases covering all scenarios
• Clear documentation: Updated help text, documentation files, and CHANGELOG entry
• Thoughtful UX:
  • Skipped _ folders are explicitly named in output (avoids silent disappearance)
  • --list provides helpful hints on stderr when no tests found
  • Positive control folder pattern supported (tests/unit/_control)
• Platform compatibility: Tested on Windows, Linux, and Darwin
• Atomic commits:
  • First commit fixes core issue (recursive search)
  • Second commit addresses follow-up feedback (underscore folder reporting)
• Code quality:
  • Consistent with Go idioms and project patterns
  • Proper symlink handling (resolves root symlinks but avoids following subfolder symlinks to prevent cycles)
  • Lexical walk order preserved
  • Deduplication and sorting of skipped directories

Recommendation

Approve the PR. It fully addresses issue #1363 with thoughtful implementation, comprehensive tests, and excellent documentation. The minor error handling concern does not block merging given the existing codebase patterns and low risk.

Note: The PR correctly avoids touching MDL syntax/generator files (as noted in the review guidelines) since this is purely a test discovery improvement in the CLI.

---

Automated review via OpenRouter (Nemotron Super 120B) — workflow source

[stagknee]

Thanks. The blocker and every note are addressed in a third commit, fix: mxcli test keeps going past a subfolder it cannot read. (The second commit, which skips _ folders and makes --list explain a 0 result, came from the second report on #1363; it's described in the PR description.)

Blocker: an unreadable subfolder no longer fails the run.

• walkTestTree fails hard only on the root, as you suggested. An error below the root goes to an onErr callback, and ParseTestDir turns it into FileError{Path, Err, Dir: true}. The walk continues, and every readable test is kept.

• The run still goes red through FileErrors. emptySuiteError and the watch walk pass nil and ignore these errors.

• Every FileErrors consumer now words a folder correctly, so it isn't reported as a parse failure:

  • the summary says N test file(s) could not be parsed and/or N folder(s) could not be read;
  • a suite entry reads <name> (folder could not be read);
  • the 0-test message distinguishes files from folders.

• Tests:

  • TestParseTestDirUnreadableSubfolderKeepsTheOtherTests is the real chmod 000 case. It skips when os.Geteuid() == 0 and on Windows.
  • Run as an unprivileged user (setpriv --reuid=65534) in the CI image, it runs (no skip) and passes, as do the two symlink tests:

  --- PASS: TestParseTestDirUnreadableSubfolderKeepsTheOtherTests (0.00s)
  --- PASS: TestParseTestDirSymlinkedRootReportsThePathTheUserTyped (0.00s)
  --- PASS: TestWalkTestTreeSymlinkedRootReportsErrorsUnderTheTypedName (0.00s)
  

  • So that the logic is also covered where the real case can't run, a small seam (walkTestTreeWith, which takes the filepath.WalkDir shape) lets three tests inject an error:
    • below the root, the error is kept and the walk continues;
    • with no callback, it is ignored;
    • on the root, it is fatal.

• On Windows I also checked it end to end with an ACL deny on tests/locked next to tests/Sales. --list found the other test, named the folder that could not be read, and exited 1.

mxcli test . from the project root: documented rather than filtered. running-tests.md and the help text now say:

• discovery is fully recursive;
• mxcli test . from the project root also searches deployment/, node_modules/, javasource/ and vendorlib/, and picks up any stray *.test.mdl;
• point it at the tests folder instead.

I didn't add a hard-coded skip list, because it could hide someone's tests.

Minor:

1. Symlinked subfolders: running-tests.md and the help text say they aren't followed, which also keeps a link cycle from looping.
2. Symlinked root: the walk still uses the resolved path. Every path we hand back is mapped under the name the user typed: test source files, FileErrors, skipped _ folders, and the path inside an I/O error. The tests include TestUnderTypedMapsAResolvedRootBackToWhatWasTyped and two symlink tests. The symlink tests skip where symlinks can't be created.
3. watch.go: testFilesMTime now walks through walkTestTree, so hidden and _ folders and the root rule are the same as discovery. This is tested in TestTestFilesMTimeFollowsTheDiscoveryRules.

Also: the CHANGELOG entry for #1363 is extended, and there's a new findings record. Its insight: moving from a one-level ReadDir to a recursive walk turns every subfolder's error into the whole run's error, and a test suite running as root can't see it.

Evidence:

• The new tests failed before the change and pass after it.
• go vet and gofmt are clean.
• On Windows, the only failure in cmd/mxcli/testrunner is the pre-existing handshake mode test.
• Linux (golang:1.26.6-bookworm, ANTLR 4.13.2): make grammar build test check-mdl check-findings check-conformance check-migration-reference lint-go all pass.

[github-actions]

AI Code Review

Review Summary

Critical Issues

None found.

Moderate Issues

None found.

Minor Issues

• None identified; all observed issues in the diff were already corrected (e.g., the ListTests signature update in test files).

What Looks Good

• Test coverage: Extensive new tests covering nested folders, hidden folders, underscore folders, symlinks, and error conditions. All tests pass and validate the fix.
• Documentation: CHANGELOG and user-facing docs (running-tests.md, testing.md) clearly explain the new recursive behavior, skipped folder reporting, and --list hint improvements.
• Error handling: Unreadable subfolders no longer fail the entire run; errors are isolated and reported appropriately. Root-level errors remain fatal as expected.
• UX improvements: Skipped "_" folders are explicitly named in output (after Found N test(s) and in --list), preventing silent omission. The hint system now works correctly for nested misnamed files.
• Conventions followed:
  • Skips dot-folders (.git, .mxcli) and underscore folders (_*) below root per Go toolchain conventions.
  • Root folder is always read (so mxcli test . works).
  • Symlinked root is resolved; symlinked subfolders are not followed (avoids cycles).
  • Lexical walk order preserves deterministic test ordering.
• Code quality:
  • Introduces clean, reusable helpers (walkTestTree, skippedDirsLine, etc.).
  • No duplication; replaces the non-recursive os.ReadDir approach with a proper tree walk.
  • Maintains existing behavior for FilesRead, FileErrors, and suite naming.
  • Handles edge cases (hidden root, symlinks, permission errors) with dedicated tests.

Recommendation

Approve. The PR fully resolves #1363 with robust, well-tested changes that align with project conventions. It enhances test discovery without breaking existing workflows and provides clear feedback to users about skipped folders and missing tests. No further changes are needed.

---

Automated review via OpenRouter (Nemotron Super 120B) — workflow source