test: raise internal package coverage - #120
Conversation
Add package-private hooks in compiler and testrunner (osExit, filepath Abs, transform/generate/mkdirTemp, readDir) so CLI exits, pipeline failures, and filesystem errors are testable without exiting the process. Extend unit tests across astwalk, compiler, executor, parser, printer, testrunner, and transformer/ts.
WalkthroughThis PR introduces package-level indirection variables (osExit, filepathAbs/Rel variants, mkdirTemp, transform/generate hooks) across compiler and testrunner code to enable error-path testing, alongside a large volume of new unit tests spanning astwalk, compiler, executor, parser, printer, testrunner, and TypeScript transformer packages, improving branch and error-path coverage without changing production behavior. ChangesInjectable Hooks (Production Code)
Estimated code review effort: 2 (Simple) | ~12 minutes Test Coverage Additions
Estimated code review effort: 3 (Moderate) | ~25 minutes Estimated code review effort: 2 (Simple) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
forst/internal/compiler/args.go (1)
69-90: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRestore
-hexit handling for subcommands
flags.Parse(argv[2:])now sendsrun -h/build -hthrough the generic error path, so the explicitosExit(0)help flow is skipped for subcommands. Add anErrHelpbranch before returningArgs{}and coverrun -hin tests.Proposed fix
if err := flags.Parse(argv[2:]); err != nil { + if errors.Is(err, flag.ErrHelp) { + osExit(0) + } return Args{} }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@forst/internal/compiler/args.go` around lines 69 - 90, The subcommand argument parsing in args.go is treating `-h` like a generic parse error, so the explicit help exit path is skipped for commands handled by the Args parser. Update the `flags.Parse(argv[2:])` handling in the argument parsing flow to detect `flag.ErrHelp` and call the same `flags.Usage()`/`osExit(0)` path used by the `help` flag before returning `Args{}`. Also add a test that exercises `run -h` (and similar subcommand help cases) to verify the zero-exit help behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@forst/internal/astwalk/walk_test.go`:
- Around line 256-264: The test name claims FunctionCallNode arguments are
walked, but StmtVisitor{} with nil OnCall cannot observe that traversal. Update
TestWalkNode_nilOnCallStillWalksArgs to use a non-nil callback/counter on the
visitor (through StmtVisitor and the WalkNode/WalkExpr path) so visiting the
nested FunctionCallNode argument produces an assertable side effect and the test
proves Arguments are actually traversed.
In `@forst/internal/compiler/args_hooks_test.go`:
- Around line 97-110: The short-help test in TestParseArgsFrom_shortHelpExits
does not assert that ParseArgsFrom actually triggered osExit, so it can pass
even if the exit path is removed. Update the test to mirror the other exit tests
by making the deferred recover verify that a panic occurred from the osExit stub
and fail the test if it did not; keep the existing exitCode assertion so the -h
path is fully validated.
In `@forst/internal/compiler/args.go`:
- Around line 70-71: The flag setup in the args parsing path uses log.Writer()
directly, which leaves the returned writer open and routes flag diagnostics
through Info-level logging. Update the code around flags.SetOutput to ensure the
writer is explicitly closed before returning, and consider sending flag output
to a non-logger sink so diagnostics remain visible even when the logger is above
Info.
In `@forst/internal/executor/executor_error_paths_test.go`:
- Around line 200-276: The second context-cancel subtest in
executor_error_paths_test duplicates the setup from context_cancel_stops_stream
but never asserts any post-cancel behavior. Either remove
context_cancel_stops_stream_after_initial_item entirely or update it to verify
that executeStreamingGoCode’s results channel closes or settles within a timeout
after cancel() so it adds distinct coverage.
- Line 77: The temp-directory failure cases in executor_error_paths_test are
using TMPDIR in a way that only reliably breaks on Unix-like systems, so they
need to be made Windows-safe. Update the temp-dir error path coverage around
os.MkdirTemp to use a platform-agnostic failure hook or guard those cases with a
GOOS-specific skip, while leaving the PATH-based start-failure tests unchanged.
Keep the existing executor_error_paths_test scenarios and adjust only the
TMPDIR-driven ones.
In `@forst/internal/executor/go_module_manager_extra_test.go`:
- Around line 138-160: The CreateModule failure test is asserting on a
platform-specific filesystem error string, so update
TestGoModuleManager_CreateModule_failureFromInvalidPackageName to check for the
intended validation error from GoModuleManager.CreateModule instead of "no such
file or directory". Add explicit PackageName sanitization in CreateModule (or
the helper that builds the import/package path) to reject path separators and
".." before constructing paths, and make the test assert that invalid package
names are rejected with that validation error.
In `@forst/internal/parser/assignment_extra_test.go`:
- Around line 43-56: The test TestParseAssignment_compoundWithExplicitTypeFails
only checks that parsing fails, so it does not verify the intended
compound-assignment-with-explicit-type rule. Update the assertion in this test
to match the specific error message emitted by finishAssignment in
assignment.go, similar to the sibling test that checks
strings.Contains(err.Error(), ...), so the failure is specifically about “cannot
use compound assignment with explicit type” rather than any unrelated parse
error.
In `@forst/internal/parser/expression_extra_test.go`:
- Around line 27-37: The subtest in parseExpression is not actually validating
the index-and-call chain it claims to cover because ParseFile consumes the
parser state and the empty if-branch is a no-op. Update the test to parse a
fresh xs[0]() instance once, remove the dead ParseFile branch, and assert the
concrete AST shape using the relevant node types from parseExpression, such as
confirming a FunctionCallNode whose callee wraps an IndexExpressionNode.
In `@forst/internal/parser/parser_coverage_extra_test.go`:
- Around line 183-195: The test name is misleading because
TestParseVarStatement_andMultipleReturnsError only covers a simple var
declaration in a single-return function and does not exercise any
multiple-return error path. Rename the test to something that matches the actual
behavior, such as TestParseVarStatement_simpleDeclaration, and keep the body
unchanged since the multi-return error case is already covered elsewhere by
TestParseFunction_multiReturnTypeError.
In `@forst/internal/parser/type_extra_test.go`:
- Around line 15-106: The table-driven `check` closures in `type_extra_test.go`
should call `t.Helper()` at the start so failures report the `t.Run` case
instead of the closure body. Update each helper closure in this test table,
following the same pattern already used in `TestParseParameterType_Branches` in
`function_test.go`, and keep the rest of the assertions unchanged.
In `@forst/internal/printer/printer_stmt_branch_test.go`:
- Around line 161-228: Refactor the multi-case printer tests to use table-driven
subtests with t.Run instead of sequential assertions in one body. Update
TestPrintEnsure_Branches, TestPrintAssignment_branches, and
TestPrintTypeDefExpr_binaryAndShape so each distinct input/output branch is
represented as its own named case, while keeping the existing assertions around
printer.printEnsure and the other targeted helpers. This should make each branch
independently runnable and easier to debug, following the same pattern used by
TestShapeExprHasNestedFields.
- Around line 183-206: The ensure printer test in printEnsure is too weak about
the empty-block formatting check: it only rejects a brace followed by a newline,
so outputs like an empty brace pair could still pass. Tighten the assertion in
the test that covers ast.EnsureNode with an empty ast.EnsureBlockNode so it
explicitly verifies that no braces are rendered at all, while keeping the
existing checks for the ensure head and the error variable.
- Around line 266-284: The test in TestPrintWith_multilineShapeWiring only
checks for "with {" and can still pass for a single-line with block, so it
doesn’t validate the intended multiline wiring formatting. Update the assertion
to verify the multiline break produced by printer.printWith for ast.WithNode,
using the existing printer and output string to assert the wiring spans multiple
lines rather than only checking the opening brace.
In `@forst/internal/printer/typeprint_test.go`:
- Around line 185-204: Refactor TestFormatConstraintArg_branches to use a
table-driven structure instead of three sequential assertions: define cases for
the type-only, empty, and value forms of formatConstraintArg, and execute each
via t.Run subtests so failures are isolated. Keep the coverage the same, but
mirror the subtest style used by TestShapeExprHasNestedFields and other
table-driven tests in this PR.
In `@forst/internal/testrunner/discover_coverage_test.go`:
- Around line 9-30: The new TestDiscoverPackages_readDirErrorAfterWalk test does
not cover a distinct path: the failure happens in the initial filepath.WalkDir
scan, and the symlink setup is unused. Either remove this duplicate test or
rewrite it so it truly exercises a unique behavior in DiscoverPackages, such as
the readDirFn-driven package loop or asserting symlinks are not followed. Keep
the existing discover_extra_test.go and hooks_test.go coverage in mind when
choosing the corrected scenario.
In `@forst/internal/testrunner/discover.go`:
- Around line 13-17: Consolidate the duplicate filepath.Rel test hook in
discover.go by removing filepathRelDiscover and reusing the existing filepathRel
hook from runner.go within DiscoverPackages. Update any tests, especially
hooks_test.go’s TestDiscoverPackages_relPathErrorInLoop, to override filepathRel
instead of the old discover-specific variable so both code paths share the same
injectable behavior.
In `@forst/internal/testrunner/hooks_test.go`:
- Around line 17-27: The tests that mutate package-level hooks such as
filepathAbs, filepathRelDiscover, readDirFn, and filepathRel rely on running
serially and restoring state via t.Cleanup, so add a short warning near those
hook declarations or the affected tests that they must not use t.Parallel().
Reference the hook vars and the mutating test functions like
TestRun_filepathAbsError, TestDiscoverPackages_skipsSubdirectoriesInPackageDir,
and TestRelPath_returnsDirWhenRelFails so future edits avoid introducing races
with parallel tests.
In `@forst/internal/transformer/ts/forst_file_test.go`:
- Around line 51-145: Consolidate the repeated TransformForstFileFromPath test
scaffolding into one table-driven test using t.Run subtests, since the five
TestTransformForstFileFromPath_* cases only vary by input source, options, and
expected result. Build a test table around the TransformForstFileFromPath call
and cover the read-error, strict typecheck error/success, and relaxed typecheck
behaviors (including nil logger) as separate cases while reusing the same
temp-file and logger setup helpers.
---
Outside diff comments:
In `@forst/internal/compiler/args.go`:
- Around line 69-90: The subcommand argument parsing in args.go is treating `-h`
like a generic parse error, so the explicit help exit path is skipped for
commands handled by the Args parser. Update the `flags.Parse(argv[2:])` handling
in the argument parsing flow to detect `flag.ErrHelp` and call the same
`flags.Usage()`/`osExit(0)` path used by the `help` flag before returning
`Args{}`. Also add a test that exercises `run -h` (and similar subcommand help
cases) to verify the zero-exit help behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 37b49f5e-17da-4893-81a5-89d6e127e00c
📒 Files selected for processing (43)
forst/internal/astwalk/walk_test.goforst/internal/compiler/args.goforst/internal/compiler/args_hooks_test.goforst/internal/compiler/compile_pipeline.goforst/internal/compiler/compiler.goforst/internal/compiler/compiler_extra_test.goforst/internal/compiler/compiler_hooks_test.goforst/internal/compiler/debug_test.goforst/internal/compiler/package_collect_test.goforst/internal/executor/executor_error_paths_test.goforst/internal/executor/go_module_manager_extra_test.goforst/internal/parser/assertion_test.goforst/internal/parser/assignment_extra_test.goforst/internal/parser/block_test.goforst/internal/parser/control_flow_test.goforst/internal/parser/example_ft_bundle_test.goforst/internal/parser/expression_extra_test.goforst/internal/parser/expression_stretch_test.goforst/internal/parser/function_test.goforst/internal/parser/parser_branch_extra_test.goforst/internal/parser/parser_coverage_extra_test.goforst/internal/parser/parser_stretch_test.goforst/internal/parser/shape_test.goforst/internal/parser/type_extra_test.goforst/internal/printer/ops_test.goforst/internal/printer/printer_error_paths_test.goforst/internal/printer/printer_stmt_branch_test.goforst/internal/printer/typeprint_test.goforst/internal/testrunner/discover.goforst/internal/testrunner/discover_coverage_test.goforst/internal/testrunner/discover_extra_test.goforst/internal/testrunner/hooks_test.goforst/internal/testrunner/runner.goforst/internal/testrunner/runner_extra_test.goforst/internal/transformer/ts/client_gen_test.goforst/internal/transformer/ts/forst_file_project_test.goforst/internal/transformer/ts/forst_file_test.goforst/internal/transformer/ts/function_test.goforst/internal/transformer/ts/merge_test.goforst/internal/transformer/ts/output_test.goforst/internal/transformer/ts/providers_export_test.goforst/internal/transformer/ts/type_mapping_test.goforst/internal/transformer/ts/typedef_test.go
| func TestWalkNode_nilOnCallStillWalksArgs(t *testing.T) { | ||
| t.Parallel() | ||
| nested := ast.FunctionCallNode{Function: ast.Ident{ID: "nested"}} | ||
| call := ast.FunctionCallNode{ | ||
| Function: ast.Ident{ID: "f"}, | ||
| Arguments: []ast.ExpressionNode{nested}, | ||
| } | ||
| WalkNode(call, StmtVisitor{}) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test doesn't assert the behavior its name claims.
StmtVisitor{} passed here has OnCall == nil. Per WalkNode's FunctionCallNode case, WalkExpr(arg, ExprVisitor{OnCall: v.OnCall}) propagates that same nil into the nested visitor, so there is no observable side effect from visiting Arguments in this test — the test can only fail via a panic, never verifying that "args" were actually walked as the name claims.
Strengthen the assertion with a non-nil counter so the test actually proves traversal into Arguments occurs:
🔧 Proposed fix
func TestWalkNode_nilOnCallStillWalksArgs(t *testing.T) {
t.Parallel()
nested := ast.FunctionCallNode{Function: ast.Ident{ID: "nested"}}
call := ast.FunctionCallNode{
Function: ast.Ident{ID: "f"},
Arguments: []ast.ExpressionNode{nested},
}
- WalkNode(call, StmtVisitor{})
+ var calls int
+ WalkNode(call, StmtVisitor{
+ OnCall: func(ast.FunctionCallNode) bool { calls++; return true },
+ })
+ if calls != 2 {
+ t.Fatalf("calls = %d, want 2 (outer call + nested argument)", calls)
+ }
}As per coding guidelines, **/*_test.{go,ts} should "Ensure presence of precise, reproducing unit or integration tests (preferably unit tests) with precise names describing exactly what's under test."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestWalkNode_nilOnCallStillWalksArgs(t *testing.T) { | |
| t.Parallel() | |
| nested := ast.FunctionCallNode{Function: ast.Ident{ID: "nested"}} | |
| call := ast.FunctionCallNode{ | |
| Function: ast.Ident{ID: "f"}, | |
| Arguments: []ast.ExpressionNode{nested}, | |
| } | |
| WalkNode(call, StmtVisitor{}) | |
| } | |
| func TestWalkNode_nilOnCallStillWalksArgs(t *testing.T) { | |
| t.Parallel() | |
| nested := ast.FunctionCallNode{Function: ast.Ident{ID: "nested"}} | |
| call := ast.FunctionCallNode{ | |
| Function: ast.Ident{ID: "f"}, | |
| Arguments: []ast.ExpressionNode{nested}, | |
| } | |
| var calls int | |
| WalkNode(call, StmtVisitor{ | |
| OnCall: func(ast.FunctionCallNode) bool { calls++; return true }, | |
| }) | |
| if calls != 2 { | |
| t.Fatalf("calls = %d, want 2 (outer call + nested argument)", calls) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@forst/internal/astwalk/walk_test.go` around lines 256 - 264, The test name
claims FunctionCallNode arguments are walked, but StmtVisitor{} with nil OnCall
cannot observe that traversal. Update TestWalkNode_nilOnCallStillWalksArgs to
use a non-nil callback/counter on the visitor (through StmtVisitor and the
WalkNode/WalkExpr path) so visiting the nested FunctionCallNode argument
produces an assertable side effect and the test proves Arguments are actually
traversed.
Source: Coding guidelines
| func TestParseArgsFrom_shortHelpExits(t *testing.T) { | ||
| var exitCode int | ||
| orig := osExit | ||
| t.Cleanup(func() { osExit = orig }) | ||
| osExit = func(code int) { exitCode = code; panic("exit") } | ||
|
|
||
| log := logrus.New() | ||
| log.SetOutput(io.Discard) | ||
| defer func() { recover() }() | ||
| _ = ParseArgsFrom([]string{"forst", "-h"}, log) | ||
| if exitCode != 0 { | ||
| t.Fatalf("exitCode = %d", exitCode) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test doesn't actually verify the exit occurred.
Unlike TestParseArgsFrom_helpExits/_versionExits/_commandHelpFlagExits, this test's deferred function discards the recover() result (defer func() { recover() }()) instead of failing when no panic occurred. If the -h path stopped calling osExit, exitCode would remain its zero value and the if exitCode != 0 check would still pass — the test would give a false positive and never catch a regression in short-help handling.
✅ Proposed fix to assert the exit panic actually happened
- defer func() { recover() }()
+ defer func() {
+ if r := recover(); r == nil {
+ t.Fatal("expected exit panic")
+ }
+ }()As per path instructions, **/*_test.{go,ts}: "Ensure presence of precise, reproducing unit or integration tests (preferably unit tests) with precise names describing exactly what's under test."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestParseArgsFrom_shortHelpExits(t *testing.T) { | |
| var exitCode int | |
| orig := osExit | |
| t.Cleanup(func() { osExit = orig }) | |
| osExit = func(code int) { exitCode = code; panic("exit") } | |
| log := logrus.New() | |
| log.SetOutput(io.Discard) | |
| defer func() { recover() }() | |
| _ = ParseArgsFrom([]string{"forst", "-h"}, log) | |
| if exitCode != 0 { | |
| t.Fatalf("exitCode = %d", exitCode) | |
| } | |
| } | |
| func TestParseArgsFrom_shortHelpExits(t *testing.T) { | |
| var exitCode int | |
| orig := osExit | |
| t.Cleanup(func() { osExit = orig }) | |
| osExit = func(code int) { exitCode = code; panic("exit") } | |
| log := logrus.New() | |
| log.SetOutput(io.Discard) | |
| defer func() { | |
| if r := recover(); r == nil { | |
| t.Fatal("expected exit panic") | |
| } | |
| }() | |
| _ = ParseArgsFrom([]string{"forst", "-h"}, log) | |
| if exitCode != 0 { | |
| t.Fatalf("exitCode = %d", exitCode) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@forst/internal/compiler/args_hooks_test.go` around lines 97 - 110, The
short-help test in TestParseArgsFrom_shortHelpExits does not assert that
ParseArgsFrom actually triggered osExit, so it can pass even if the exit path is
removed. Update the test to mirror the other exit tests by making the deferred
recover verify that a panic occurred from the osExit stub and fail the test if
it did not; keep the existing exitCode assertion so the -h path is fully
validated.
Source: Path instructions
| flags := flag.NewFlagSet(command, flag.ContinueOnError) | ||
| flags.SetOutput(log.Writer()) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== args.go =="
cat -n forst/internal/compiler/args.go | sed -n '1,220p'
echo
echo "== search for log.Writer() usage =="
rg -n "log\.Writer\(\)|SetOutput\(" forst -S
echo
echo "== locate logger dependency =="
rg -n "logrus|sirupsen" go.mod go.sum forst -S || true
echo
echo "== if vendored or present, inspect logrus Writer docs/source =="
fd -a 'writer.go' . | rg 'logrus|sirupsen' || trueRepository: forst-lang/forst
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import os, glob
cands = []
for base in [os.environ.get("GOMODCACHE"), os.path.expanduser("~/go/pkg/mod"), os.path.expanduser("~/go")]:
if not base:
continue
for p in glob.glob(os.path.join(base, "github.com", "sirupsen", "logrus*", "**", "*.go"), recursive=True):
if p.endswith("writer.go") or p.endswith("exported.go") or p.endswith("logger.go"):
cands.append(p)
print("\n".join(sorted(set(cands))))
PYRepository: forst-lang/forst
Length of output: 155
🌐 Web query:
logrus Writer io.PipeWriter close responsibility WriterLevel docs v1.9.4
💡 Result:
In logrus v1.9.4, when you obtain an io.Writer using Logger.Writer or Logger.WriterLevel(level), the returned object is an io.PipeWriter [1][2]. According to the official documentation and the source code, it is the caller's explicit responsibility to close the returned io.PipeWriter when they are finished using it [3][4][2]. This is typically done using defer w.Close [4][5]. While the internal implementation includes a runtime finalizer (runtime.SetFinalizer) that calls writer.Close if the writer is garbage collected [1][2], relying on this is not recommended as it is non-deterministic and may lead to issues where log output is not flushed or displayed correctly if the application terminates before the garbage collector runs [6][7][8]. Therefore, manual closure is required to ensure all data is processed correctly [6].
Citations:
- 1: https://github.com/sirupsen/logrus/blob/master/writer.go
- 2: https://github.com/sirupsen/logrus/blob/v1.9.4/writer.go
- 3: https://pkg.go.dev/github.com/sirupsen/logrus
- 4: https://pkg.go.dev/github.com/sirupsen/logrus@v1.9.4
- 5: https://github.com/sirupsen/logrus?tab=readme-ov-file
- 6: Properly close the writer and wait for all writes to finish sirupsen/logrus#1092
- 7: PipeWriter closing is non-deterministic sirupsen/logrus#1111
- 8: Who closes the file handler if i use file logger sirupsen/logrus#706
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n forst/cmd/forst/main.go | sed -n '1,180p'
echo
echo "== ParseArgs / ParseArgsFrom call sites =="
rg -n "ParseArgsFrom\(|ParseArgs\(" forst/cmd/forst forst/internal/compiler -SRepository: forst-lang/forst
Length of output: 8761
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "func newLogger|func setLogLevel|SetLevel\(" forst/cmd/forst/main.go forst/cmd/forst -S
cat -n forst/cmd/forst/main.go | sed -n '180,320p'Repository: forst-lang/forst
Length of output: 6593
Close the log.Writer() handle before returning. flags.SetOutput(log.Writer()) leaves the returned io.PipeWriter open; logrus expects the caller to close it, so cleanup depends on GC/finalizers and can keep a goroutine around longer than necessary. This also sends flag diagnostics through the logger’s Info level, so they disappear when the logger is set above Info.
🔧 Proposed fix
- flags.SetOutput(log.Writer())
+ flagsOut := log.Writer()
+ defer flagsOut.Close()
+ flags.SetOutput(flagsOut)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| flags := flag.NewFlagSet(command, flag.ContinueOnError) | |
| flags.SetOutput(log.Writer()) | |
| flagsOut := log.Writer() | |
| defer flagsOut.Close() | |
| flags.SetOutput(flagsOut) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@forst/internal/compiler/args.go` around lines 70 - 71, The flag setup in the
args parsing path uses log.Writer() directly, which leaves the returned writer
open and routes flag diagnostics through Info-level logging. Update the code
around flags.SetOutput to ensure the writer is explicitly closed before
returning, and consider sending flag output to a non-logger sink so diagnostics
remain visible even when the logger is above Info.
| t.Run("start_failure_with_params", func(t *testing.T) { | ||
| e := testExecutor(t, t.TempDir()) | ||
| dir := t.TempDir() | ||
| t.Setenv("PATH", "") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does Go's os.MkdirTemp respect TMPDIR on Windows, or only TMP/TEMP?
💡 Result:
Go's os.MkdirTemp does not respect the TMPDIR environment variable on Windows [1][2][3]. On Windows, os.MkdirTemp uses the directory returned by os.TempDir when no directory is specified [1][4]. The os.TempDir function relies on the Windows GetTempPath (or GetTempPath2) API [1][5][2]. According to official Windows API behavior, this retrieves the temporary directory by checking environment variables in the following order [6]: 1. The path specified by the TMP environment variable. 2. The path specified by the TEMP environment variable. 3. The path specified by the USERPROFILE environment variable. 4. The Windows directory. Because Go uses the native Windows API for this process, it does not look for TMPDIR on Windows [1][2]. The TMPDIR variable is exclusively used by Go's os.TempDir on Unix-like systems [1][2]. Note that there is a separate Go-specific environment variable called GOTMPDIR, but it is primarily used by the 'go' command for build artifacts and does not automatically change the behavior of os.MkdirTemp for all Go programs [7][8].
Citations:
- 1: https://pkg.go.dev/os?GOOS=windows
- 2: https://go.googlesource.com/go/+/refs/heads/master/src/os/file.go
- 3: https://pkg.go.dev/os
- 4: https://go.googlesource.com/go/+/refs/heads/master/src/os/tempfile.go
- 5: https://github.com/golang/go/blob/99d4927f/src/os/file_windows.go
- 6: TempFile fails to find temporary directory golang/go#834
- 7: testing: use $GOTMPDIR for temporary files when set golang/go#61585
- 8: cmd/go: propagate GOTMPDIR to subprocess temp directory variables golang/go#78679
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the test file and inspect the relevant sections with line numbers.
git ls-files 'forst/internal/executor/executor_error_paths_test.go'
echo '---'
cat -n forst/internal/executor/executor_error_paths_test.go | sed -n '1,520p'Repository: forst-lang/forst
Length of output: 15314
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files 'forst/internal/executor/executor_error_paths_test.go'
echo '---'
cat -n forst/internal/executor/executor_error_paths_test.go | sed -n '1,520p'Repository: forst-lang/forst
Length of output: 15314
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the executor implementation around temp-dir creation and command execution.
ast-grep outline forst/internal/executor --view expanded
echo '--- createTempGoFile / ExecuteFunction / executeGoCode references ---'
rg -n "createTempGoFile|ExecuteFunction|executeGoCode|executeStreamingGoCode|exec.Command|os.MkdirTemp|os.TempDir" forst/internal/executor -g '!**/*_test.go'Repository: forst-lang/forst
Length of output: 12303
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline forst/internal/executor --view expanded
echo '--- createTempGoFile / ExecuteFunction / executeGoCode references ---'
rg -n "createTempGoFile|ExecuteFunction|executeGoCode|executeStreamingGoCode|exec.Command|os.MkdirTemp|os.TempDir" forst/internal/executor -g '!**/*_test.go'Repository: forst-lang/forst
Length of output: 12303
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the implementation of the temp-dir and command-start paths that the tests target.
ast-grep outline forst/internal/executor --view expanded
echo '--- matches ---'
rg -n "func .*createTempGoFile|func .*executeGoCode|func .*executeStreamingGoCode|exec\.Command|MkdirTemp|TempDir\(" forst/internal/executor -g '!**/*_test.go' -A 8 -B 8Repository: forst-lang/forst
Length of output: 16842
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether the repository advertises Windows support or Windows CI coverage.
git ls-files | rg -n '(^|/)(README|CHANGELOG|CONTRIBUTING|\.github/workflows/.*|go\.mod|go\.work)$' >/dev/null || true
rg -n -i '\bwindows\b|GOOS=windows|os\.Setenv\("TMPDIR"|PATHEXT|TMPDIR' . \
-g '!**/vendor/**' -g '!**/*_test.go'Repository: forst-lang/forst
Length of output: 5571
🌐 Web query:
Go exec.LookPath Windows PATH PATHEXT empty PATH behavior
💡 Result:
In Go's os/exec package on Windows, the behavior of LookPath regarding PATH and PATHEXT is defined by specific lookup rules designed to prioritize security [1][2]. PATHEXT Environment Variable When PATHEXT is set, LookPath uses those extensions to find a matching executable [1][2]. If PATHEXT is empty or unset, LookPath defaults to using the set:.com,.exe,.bat, and.cmd [3][1]. Behavior with Empty or Unset PATH When the PATH environment variable is empty or unset, LookPath does not search the system PATH, as there are no directories to iterate through [1]. Security and the Current Directory (ErrDot) Since Go 1.19, os/exec includes security measures to prevent "DLL hijacking" or similar vulnerabilities arising from accidental execution of files in the current working directory [4][2]. Even if an empty string or a dot (.) is present in the PATH (which historically might have included the current directory in the search), LookPath will explicitly return an error satisfying errors.Is(err, ErrDot) if the resolved path is relative to the current directory [2]. Consequently, LookPath will not successfully return a result that relies on implicit or explicit resolution to the current directory [2]. Technical Implementation Details 1. Lookup Order: LookPath first attempts to check if the file can be found relative to the current directory (which triggers the ErrDot security check) [1]. 2. PATH Iteration: It then splits the PATH string using filepath.SplitList and iterates through each directory, appending the file name and testing it against the extension list derived from PATHEXT (or the defaults) [1]. 3. If no match is found after checking the current directory (resulting in an error) and all directories in the PATH, it returns an error satisfying ErrNotFound [1].
Citations:
- 1: https://github.com/golang/go/blob/1724077b789ad92972ab1ac03788389645306cbb/src/os/exec/lp_windows.go
- 2: https://pkg.go.dev/os/exec?GOOS=windows
- 3: https://pkg.go.dev/os/exec
- 4: os/exec: exec.LookPath return errors when PATH starts with semicolon (parsed as contains empty string at first) and working directory is same as the target executable on windows golang/go#69636
TMPDIR failure injection is Windows-incompatible. The t.Setenv("TMPDIR", "/path/that/does/not/exist") cases won’t make os.MkdirTemp fail on Windows, so those temp-dir error paths need a platform-agnostic hook or a GOOS-specific skip. The PATH="" start-failure cases can stay as-is.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@forst/internal/executor/executor_error_paths_test.go` at line 77, The
temp-directory failure cases in executor_error_paths_test are using TMPDIR in a
way that only reliably breaks on Unix-like systems, so they need to be made
Windows-safe. Update the temp-dir error path coverage around os.MkdirTemp to use
a platform-agnostic failure hook or guard those cases with a GOOS-specific skip,
while leaving the PATH-based start-failure tests unchanged. Keep the existing
executor_error_paths_test scenarios and adjust only the TMPDIR-driven ones.
| t.Run("context_cancel_stops_stream", func(t *testing.T) { | ||
| e := testExecutor(t, t.TempDir()) | ||
| dir := t.TempDir() | ||
| writeFile(t, filepath.Join(dir, "go.mod"), "module streamtest\ngo 1.24\n") | ||
| writeFile(t, filepath.Join(dir, "main.go"), `package main | ||
| import ( | ||
| "fmt" | ||
| "time" | ||
| ) | ||
| func main() { | ||
| for i := 0; i < 100; i++ { | ||
| fmt.Println("{\"status\":\"ok\",\"data\":1}") | ||
| time.Sleep(20 * time.Millisecond) | ||
| } | ||
| } | ||
| `) | ||
|
|
||
| ctx, cancel := context.WithCancel(context.Background()) | ||
| defer cancel() | ||
| results, err := e.executeStreamingGoCode(ctx, dir, nil, false) | ||
| if err != nil { | ||
| t.Fatalf("executeStreamingGoCode: %v", err) | ||
| } | ||
|
|
||
| r, ok := <-results | ||
| if !ok { | ||
| t.Fatal("expected at least one stream result before cancel") | ||
| } | ||
| if r.Error != "" { | ||
| t.Fatalf("unexpected stream error before cancel: %s", r.Error) | ||
| } | ||
| cancel() | ||
|
|
||
| select { | ||
| case _, ok := <-results: | ||
| if ok { | ||
| // channel can still briefly produce buffered item; accept it. | ||
| } | ||
| case <-time.After(2 * time.Second): | ||
| t.Fatal("stream results channel did not settle after context cancel") | ||
| } | ||
| }) | ||
|
|
||
| t.Run("context_cancel_stops_stream_after_initial_item", func(t *testing.T) { | ||
| e := testExecutor(t, t.TempDir()) | ||
| dir := t.TempDir() | ||
| writeFile(t, filepath.Join(dir, "go.mod"), "module streamtest\ngo 1.24\n") | ||
| writeFile(t, filepath.Join(dir, "main.go"), `package main | ||
| import ( | ||
| "fmt" | ||
| "time" | ||
| ) | ||
| func main() { | ||
| for i := 0; i < 100; i++ { | ||
| fmt.Println("{\"status\":\"ok\",\"data\":1}") | ||
| time.Sleep(20 * time.Millisecond) | ||
| } | ||
| } | ||
| `) | ||
|
|
||
| ctx, cancel := context.WithCancel(context.Background()) | ||
| defer cancel() | ||
| results, err := e.executeStreamingGoCode(ctx, dir, nil, false) | ||
| if err != nil { | ||
| t.Fatalf("executeStreamingGoCode: %v", err) | ||
| } | ||
|
|
||
| r, ok := <-results | ||
| if !ok { | ||
| t.Fatal("expected at least one stream result before cancel") | ||
| } | ||
| if r.Error != "" { | ||
| t.Fatalf("unexpected stream error before cancel: %s", r.Error) | ||
| } | ||
| cancel() | ||
| }) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Second context-cancel subtest doesn't verify anything new.
context_cancel_stops_stream_after_initial_item (Lines 243-275) duplicates the setup of context_cancel_stops_stream but never checks that the stream actually stops/settles after cancel() — it just calls cancel() and returns. The name implies verification of post-cancel behavior that isn't asserted, adding subprocess overhead without added coverage.
♻️ Suggested fix
- t.Run("context_cancel_stops_stream_after_initial_item", func(t *testing.T) {
- e := testExecutor(t, t.TempDir())
- dir := t.TempDir()
- writeFile(t, filepath.Join(dir, "go.mod"), "module streamtest\ngo 1.24\n")
- writeFile(t, filepath.Join(dir, "main.go"), `package main
-import (
- "fmt"
- "time"
-)
-func main() {
- for i := 0; i < 100; i++ {
- fmt.Println("{\"status\":\"ok\",\"data\":1}")
- time.Sleep(20 * time.Millisecond)
- }
-}
-`)
-
- ctx, cancel := context.WithCancel(context.Background())
- defer cancel()
- results, err := e.executeStreamingGoCode(ctx, dir, nil, false)
- if err != nil {
- t.Fatalf("executeStreamingGoCode: %v", err)
- }
-
- r, ok := <-results
- if !ok {
- t.Fatal("expected at least one stream result before cancel")
- }
- if r.Error != "" {
- t.Fatalf("unexpected stream error before cancel: %s", r.Error)
- }
- cancel()
- })Either remove this subtest or add an assertion that results closes/drains within a bound after cancel().
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| t.Run("context_cancel_stops_stream", func(t *testing.T) { | |
| e := testExecutor(t, t.TempDir()) | |
| dir := t.TempDir() | |
| writeFile(t, filepath.Join(dir, "go.mod"), "module streamtest\ngo 1.24\n") | |
| writeFile(t, filepath.Join(dir, "main.go"), `package main | |
| import ( | |
| "fmt" | |
| "time" | |
| ) | |
| func main() { | |
| for i := 0; i < 100; i++ { | |
| fmt.Println("{\"status\":\"ok\",\"data\":1}") | |
| time.Sleep(20 * time.Millisecond) | |
| } | |
| } | |
| `) | |
| ctx, cancel := context.WithCancel(context.Background()) | |
| defer cancel() | |
| results, err := e.executeStreamingGoCode(ctx, dir, nil, false) | |
| if err != nil { | |
| t.Fatalf("executeStreamingGoCode: %v", err) | |
| } | |
| r, ok := <-results | |
| if !ok { | |
| t.Fatal("expected at least one stream result before cancel") | |
| } | |
| if r.Error != "" { | |
| t.Fatalf("unexpected stream error before cancel: %s", r.Error) | |
| } | |
| cancel() | |
| select { | |
| case _, ok := <-results: | |
| if ok { | |
| // channel can still briefly produce buffered item; accept it. | |
| } | |
| case <-time.After(2 * time.Second): | |
| t.Fatal("stream results channel did not settle after context cancel") | |
| } | |
| }) | |
| t.Run("context_cancel_stops_stream_after_initial_item", func(t *testing.T) { | |
| e := testExecutor(t, t.TempDir()) | |
| dir := t.TempDir() | |
| writeFile(t, filepath.Join(dir, "go.mod"), "module streamtest\ngo 1.24\n") | |
| writeFile(t, filepath.Join(dir, "main.go"), `package main | |
| import ( | |
| "fmt" | |
| "time" | |
| ) | |
| func main() { | |
| for i := 0; i < 100; i++ { | |
| fmt.Println("{\"status\":\"ok\",\"data\":1}") | |
| time.Sleep(20 * time.Millisecond) | |
| } | |
| } | |
| `) | |
| ctx, cancel := context.WithCancel(context.Background()) | |
| defer cancel() | |
| results, err := e.executeStreamingGoCode(ctx, dir, nil, false) | |
| if err != nil { | |
| t.Fatalf("executeStreamingGoCode: %v", err) | |
| } | |
| r, ok := <-results | |
| if !ok { | |
| t.Fatal("expected at least one stream result before cancel") | |
| } | |
| if r.Error != "" { | |
| t.Fatalf("unexpected stream error before cancel: %s", r.Error) | |
| } | |
| cancel() | |
| }) | |
| } | |
| t.Run("context_cancel_stops_stream", func(t *testing.T) { | |
| e := testExecutor(t, t.TempDir()) | |
| dir := t.TempDir() | |
| writeFile(t, filepath.Join(dir, "go.mod"), "module streamtest\ngo 1.24\n") | |
| writeFile(t, filepath.Join(dir, "main.go"), `package main | |
| import ( | |
| "fmt" | |
| "time" | |
| ) | |
| func main() { | |
| for i := 0; i < 100; i++ { | |
| fmt.Println("{\"status\":\"ok\",\"data\":1}") | |
| time.Sleep(20 * time.Millisecond) | |
| } | |
| } | |
| `) | |
| ctx, cancel := context.WithCancel(context.Background()) | |
| defer cancel() | |
| results, err := e.executeStreamingGoCode(ctx, dir, nil, false) | |
| if err != nil { | |
| t.Fatalf("executeStreamingGoCode: %v", err) | |
| } | |
| r, ok := <-results | |
| if !ok { | |
| t.Fatal("expected at least one stream result before cancel") | |
| } | |
| if r.Error != "" { | |
| t.Fatalf("unexpected stream error before cancel: %s", r.Error) | |
| } | |
| cancel() | |
| select { | |
| case _, ok := <-results: | |
| if ok { | |
| // channel can still briefly produce buffered item; accept it. | |
| } | |
| case <-time.After(2 * time.Second): | |
| t.Fatal("stream results channel did not settle after context cancel") | |
| } | |
| }) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@forst/internal/executor/executor_error_paths_test.go` around lines 200 - 276,
The second context-cancel subtest in executor_error_paths_test duplicates the
setup from context_cancel_stops_stream but never asserts any post-cancel
behavior. Either remove context_cancel_stops_stream_after_initial_item entirely
or update it to verify that executeStreamingGoCode’s results channel closes or
settles within a timeout after cancel() so it adds distinct coverage.
| func TestFormatConstraintArg_branches(t *testing.T) { | ||
| t.Parallel() | ||
| p := printer{cfg: DefaultConfig()} | ||
|
|
||
| if got := p.formatConstraintArg(ast.ConstraintArgumentNode{ | ||
| Type: &ast.TypeNode{Ident: ast.TypeInt}, | ||
| }); got != "Int" { | ||
| t.Fatalf("type arg = %q", got) | ||
| } | ||
|
|
||
| if got := p.formatConstraintArg(ast.ConstraintArgumentNode{}); got != "?" { | ||
| t.Fatalf("empty arg = %q", got) | ||
| } | ||
|
|
||
| if got := p.formatConstraintArg(ast.ConstraintArgumentNode{ | ||
| Value: ptrConstraintValue(ast.IntLiteralNode{Value: 3}), | ||
| }); got != "3" { | ||
| t.Fatalf("value arg = %q", got) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Prefer table-driven test for multi-case formatConstraintArg coverage.
This test checks three distinct argument shapes (type-only, empty, value) sequentially; a table-driven structure with t.Run subtests (as used elsewhere in this PR, e.g. TestShapeExprHasNestedFields) would isolate failures per case.
As per coding guidelines, "Implement table-driven tests for multiple inputs and name subtests with t.Run(\"case\", ...)."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@forst/internal/printer/typeprint_test.go` around lines 185 - 204, Refactor
TestFormatConstraintArg_branches to use a table-driven structure instead of
three sequential assertions: define cases for the type-only, empty, and value
forms of formatConstraintArg, and execute each via t.Run subtests so failures
are isolated. Keep the coverage the same, but mirror the subtest style used by
TestShapeExprHasNestedFields and other table-driven tests in this PR.
Source: Path instructions
| func TestDiscoverPackages_readDirErrorAfterWalk(t *testing.T) { | ||
| if os.Getuid() == 0 { | ||
| t.Skip("root bypasses chmod 000") | ||
| } | ||
| root := t.TempDir() | ||
| secret := filepath.Join(root, "secret") | ||
| if err := os.Mkdir(secret, 0o755); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| writeFile(t, filepath.Join(secret, "a_test.ft"), "package secret\n") | ||
| link := filepath.Join(root, "link") | ||
| if err := os.Symlink(secret, link); err != nil { | ||
| t.Skip("symlinks unavailable") | ||
| } | ||
| if err := os.Chmod(secret, 0o000); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| t.Cleanup(func() { _ = os.Chmod(secret, 0o755) }) | ||
| if _, err := DiscoverPackages(root, nil); err == nil { | ||
| t.Fatal("expected readdir error via symlink to unreadable dir") | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Test doesn't exercise what its name implies; duplicates an existing test.
The failure here is produced by the raw, unhooked filepath.WalkDir call hitting the chmod'd secret directory directly during the initial discovery walk — this happens before the code ever reaches the readDirFn-based per-package loop. The link symlink is never traversed (WalkDir doesn't follow symlinks and treats link as a non-directory leaf, which gets filtered out by the _test.ft suffix check), so it has no effect on the outcome.
As written, this duplicates discover_extra_test.go's TestDiscoverPackages_walkPermissionError, which already covers the raw walk-permission-error path more directly and without the unused symlink setup. If the intent was to cover the new readDirFn hook's error path post-walk, that's already covered by hooks_test.go's TestDiscoverPackages_readDirErrorInPackageLoop. Consider removing this test or rewriting it to actually target a distinct scenario (e.g. asserting the symlink is not followed, if that's the real intent).
As per path instructions, "**/*_test.{go,ts}: Ensure presence of precise, reproducing unit or integration tests ... with precise names describing exactly what's under test."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@forst/internal/testrunner/discover_coverage_test.go` around lines 9 - 30, The
new TestDiscoverPackages_readDirErrorAfterWalk test does not cover a distinct
path: the failure happens in the initial filepath.WalkDir scan, and the symlink
setup is unused. Either remove this duplicate test or rewrite it so it truly
exercises a unique behavior in DiscoverPackages, such as the readDirFn-driven
package loop or asserting symlinks are not followed. Keep the existing
discover_extra_test.go and hooks_test.go coverage in mind when choosing the
corrected scenario.
Source: Path instructions
| var ( | ||
| filepathRelDiscover = filepath.Rel | ||
| readDirFn = os.ReadDir | ||
| ) | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Duplicate filepath.Rel wrapper — consolidate with runner.go's filepathRel.
filepathRelDiscover here and filepathRel in runner.go (same package) both wrap the identical stdlib call filepath.Rel. There's no naming collision forcing two variables — discover.go could simply reuse filepathRel from runner.go. Keeping two independent hooks for the same operation is confusing: a future test author overriding one (e.g. filepathRel to test Run/relPath) will not affect the other (DiscoverPackages's rel computation), and vice versa, which can silently under-test or mask regressions.
♻️ Proposed consolidation
var (
- filepathRelDiscover = filepath.Rel
- readDirFn = os.ReadDir
+ readDirFn = os.ReadDir
)- rel, err := filepathRelDiscover(moduleRoot, dir)
+ rel, err := filepathRel(moduleRoot, dir)(Requires updating hooks_test.go's TestDiscoverPackages_relPathErrorInLoop to override filepathRel instead of filepathRelDiscover.)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@forst/internal/testrunner/discover.go` around lines 13 - 17, Consolidate the
duplicate filepath.Rel test hook in discover.go by removing filepathRelDiscover
and reusing the existing filepathRel hook from runner.go within
DiscoverPackages. Update any tests, especially hooks_test.go’s
TestDiscoverPackages_relPathErrorInLoop, to override filepathRel instead of the
old discover-specific variable so both code paths share the same injectable
behavior.
| func TestRun_filepathAbsError(t *testing.T) { | ||
| orig := filepathAbs | ||
| t.Cleanup(func() { filepathAbs = orig }) | ||
| filepathAbs = func(string) (string, error) { | ||
| return "", errors.New("abs failed") | ||
| } | ||
| code, err := Run(Options{ModuleRoot: ".", Log: testLog(t)}) | ||
| if err == nil || code != ExitError { | ||
| t.Fatalf("code=%d err=%v", code, err) | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Global hook mutation relies on all mutating tests staying non-parallel.
These tests mutate package-level vars (filepathAbs, filepathRelDiscover, readDirFn, filepathRel) and restore them via t.Cleanup. This is currently safe because none of these tests call t.Parallel(), so Go's test scheduler runs them (and their cleanup) to completion before any parallel test in the package (e.g. discover_coverage_test.go's TestDiscoverPackages_skipsSubdirectoriesInPackageDir, runner_extra_test.go's TestRelPath_returnsDirWhenRelFails) executes its body. If a future edit marks any of these mutating tests t.Parallel(), it would introduce a data race and flaky failures for any parallel test reading the same hook. Worth a short comment on the var declarations (or on these tests) warning against adding t.Parallel() here.
Also applies to: 80-91, 93-109, 169-179
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@forst/internal/testrunner/hooks_test.go` around lines 17 - 27, The tests that
mutate package-level hooks such as filepathAbs, filepathRelDiscover, readDirFn,
and filepathRel rely on running serially and restoring state via t.Cleanup, so
add a short warning near those hook declarations or the affected tests that they
must not use t.Parallel(). Reference the hook vars and the mutating test
functions like TestRun_filepathAbsError,
TestDiscoverPackages_skipsSubdirectoriesInPackageDir, and
TestRelPath_returnsDirWhenRelFails so future edits avoid introducing races with
parallel tests.
| func TestTransformForstFileFromPath_readError(t *testing.T) { | ||
| log := logrus.New() | ||
| log.SetOutput(io.Discard) | ||
| _, err := TransformForstFileFromPath("/definitely/missing/file.ft", log, TransformForstFileOptions{}) | ||
| if err == nil || !strings.Contains(err.Error(), "failed to read file") { | ||
| t.Fatalf("expected read-file error, got %v", err) | ||
| } | ||
| } | ||
|
|
||
| func TestTransformForstFileFromPath_strictTypecheck_reportsTypeErrors(t *testing.T) { | ||
| dir := t.TempDir() | ||
| ft := filepath.Join(dir, "bad_types.ft") | ||
| src := `package main | ||
|
|
||
| func Broken(x UnknownType) { | ||
| return x | ||
| } | ||
| ` | ||
| if err := os.WriteFile(ft, []byte(src), 0644); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| log := logrus.New() | ||
| log.SetOutput(io.Discard) | ||
| _, err := TransformForstFileFromPath(ft, log, TransformForstFileOptions{RelaxedTypecheck: false}) | ||
| if err == nil || !strings.Contains(err.Error(), "failed to type check") { | ||
| t.Fatalf("expected strict typecheck error, got %v", err) | ||
| } | ||
| } | ||
|
|
||
| func TestTransformForstFileFromPath_relaxedTypecheck_continuesAfterTypeErrors(t *testing.T) { | ||
| dir := t.TempDir() | ||
| ft := filepath.Join(dir, "bad_relaxed.ft") | ||
| src := `package main | ||
|
|
||
| func Broken(x UnknownType) { | ||
| return x | ||
| } | ||
| ` | ||
| if err := os.WriteFile(ft, []byte(src), 0644); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| log := logrus.New() | ||
| log.SetOutput(io.Discard) | ||
| out, err := TransformForstFileFromPath(ft, log, TransformForstFileOptions{RelaxedTypecheck: true}) | ||
| if err != nil { | ||
| t.Fatalf("expected relaxed mode to continue, got %v", err) | ||
| } | ||
| if out == nil || out.SourceFileStem != "bad_relaxed" { | ||
| t.Fatalf("unexpected output: %#v", out) | ||
| } | ||
| } | ||
|
|
||
| func TestTransformForstFileFromPath_relaxedTypecheck_withNilLogger(t *testing.T) { | ||
| dir := t.TempDir() | ||
| ft := filepath.Join(dir, "bad_relaxed_nil_log.ft") | ||
| src := `package main | ||
|
|
||
| func Broken(x UnknownType) { | ||
| return x | ||
| } | ||
| ` | ||
| if err := os.WriteFile(ft, []byte(src), 0644); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| out, err := TransformForstFileFromPath(ft, nil, TransformForstFileOptions{RelaxedTypecheck: true}) | ||
| if err != nil { | ||
| t.Fatalf("expected relaxed mode to continue with nil logger, got %v", err) | ||
| } | ||
| if out == nil || out.SourceFileStem != "bad_relaxed_nil_log" { | ||
| t.Fatalf("unexpected output: %#v", out) | ||
| } | ||
| } | ||
|
|
||
| func TestTransformForstFileFromPath_strictTypecheck_success(t *testing.T) { | ||
| dir := t.TempDir() | ||
| ft := filepath.Join(dir, "ok.ft") | ||
| src := `package main | ||
|
|
||
| func Echo(x String) { | ||
| return x | ||
| } | ||
| ` | ||
| if err := os.WriteFile(ft, []byte(src), 0644); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| log := logrus.New() | ||
| log.SetOutput(io.Discard) | ||
| out, err := TransformForstFileFromPath(ft, log, TransformForstFileOptions{RelaxedTypecheck: false}) | ||
| if err != nil { | ||
| t.Fatalf("expected strict successful transform, got %v", err) | ||
| } | ||
| if out == nil || out.SourceFileStem != "ok" { | ||
| t.Fatalf("unexpected output: %#v", out) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Consolidate repetitive setup into a table-driven test.
Five new test functions (TestTransformForstFileFromPath_readError, _strictTypecheck_reportsTypeErrors, _relaxedTypecheck_continuesAfterTypeErrors, _relaxedTypecheck_withNilLogger, _strictTypecheck_success) repeat the same write-temp-file / create-logger / call-TransformForstFileFromPath scaffolding with only the source, options, and expected outcome varying. As per coding guidelines, forst/**/*_test.go tests should "Implement table-driven tests for multiple inputs and name subtests with t.Run("case", ...)."
♻️ Example table-driven consolidation
-func TestTransformForstFileFromPath_readError(t *testing.T) { ... }
-func TestTransformForstFileFromPath_strictTypecheck_reportsTypeErrors(t *testing.T) { ... }
-func TestTransformForstFileFromPath_relaxedTypecheck_continuesAfterTypeErrors(t *testing.T) { ... }
-func TestTransformForstFileFromPath_relaxedTypecheck_withNilLogger(t *testing.T) { ... }
-func TestTransformForstFileFromPath_strictTypecheck_success(t *testing.T) { ... }
+func TestTransformForstFileFromPath_variants(t *testing.T) {
+ tests := []struct {
+ name string
+ src string
+ relaxed bool
+ nilLogger bool
+ missing bool
+ wantErr bool
+ wantErrSub string
+ wantStem string
+ }{
+ {name: "read error", missing: true, wantErr: true, wantErrSub: "failed to read file"},
+ {name: "strict typecheck failure", src: brokenSrc, wantErr: true, wantErrSub: "failed to type check"},
+ {name: "relaxed continues after type error", src: brokenSrc, relaxed: true, wantStem: "x"},
+ {name: "relaxed with nil logger", src: brokenSrc, relaxed: true, nilLogger: true, wantStem: "x"},
+ {name: "strict success", src: okSrc, wantStem: "ok"},
+ }
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) { /* shared setup + assertions */ })
+ }
+}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@forst/internal/transformer/ts/forst_file_test.go` around lines 51 - 145,
Consolidate the repeated TransformForstFileFromPath test scaffolding into one
table-driven test using t.Run subtests, since the five
TestTransformForstFileFromPath_* cases only vary by input source, options, and
expected result. Build a test table around the TransformForstFileFromPath call
and cover the read-error, strict typecheck error/success, and relaxed typecheck
behaviors (including nil logger) as separate cases while reusing the same
temp-file and logger setup helpers.
Source: Coding guidelines
1 participant
Add package-private hooks in compiler and testrunner (osExit, filepath Abs, transform/generate/mkdirTemp, readDir) so CLI exits, pipeline failures, and filesystem errors are testable without exiting the process.
Extend unit tests across astwalk, compiler, executor, parser, printer, testrunner, and transformer/ts.
Summary by CodeRabbit
Bug Fixes
Tests