feat(formula): add onTest callback#109
Conversation
Introduce the onTest hook as a sibling of onBuild in the ModuleF classfile: - formula/classfile.go: add fOnTest field and OnTest() setter on ModuleF. - internal/formula/formula.go: expose OnTest on the loader-side Formula and extract fOnTest via reflection in loadFS. - internal/formula/testdata/formula/hello_llar.gox: add an onTest block so the loader test exercises the new field. - internal/formula/formula_test.go: assert f.OnTest is non-nil and can be invoked without panic. This commit only introduces the DSL primitive and loader plumbing; no runtime wiring runs onTest yet.
Wire the onTest DSL callback introduced in the previous commit into the
build runtime and expose it through a new 'llar test' command.
internal/build:
- Options gains a RunTest bool; Builder stores it as runTest.
- When runTest is set, Build() skips the build-cache read/write path and
invokes mod.OnTest (if any) on the same Context/Project used by
OnBuild. OnTest errors are surfaced with clear context identifying the
failing <modPath>@<version>.
- build_test.go adds three focused tests (OnTest callbacks are injected
onto loaded modules, so no new testdata is required):
* RunTest disabled -> OnTest is not invoked
* RunTest enabled -> OnTest errors surface with module context
* RunTest enabled -> cache read/write are bypassed
cmd/llar/internal:
- Extract host-default matrix selection into hostMatrixCombo() and
reuse it from both make and test.
- buildModule gains a runTest bool parameter that maps to build.Options.
- New test command (cmd/llar/internal/test.go) mirrors the make command
layout, reuses buildModule, and toggles makeVerbose via testVerbose
for the duration of the run.
- make_test.go updated for the new buildModule signature.
The existing onTest tests in build_test.go inject OnTest callbacks
programmatically from Go. That validates the Builder's runTest wiring
but does not exercise the DSL -> classfile -> loader path. Add two E2E
tests that run real formulas through the interpreter.
testdata:
- test/testhook/1.0.0/Testhook_llar.gox: onBuild sets metadata; onTest
uses os.writeFile to drop an "ontest.stamp" marker under ctx.outputDir()
whose content is ctx.currentMatrix(). This proves onTest actually ran
and that the build Context is correctly wired into the interpreted
callback.
- test/testfail/1.0.0/Testfail_llar.gox: onTest triggers an error via
proj.readFile("nonexistent.txt") and records it with out.addErr.
e2e_test.go:
- TestE2E_OnTest_SucceedsAndRuns: builds test/testhook with runTest=true
and asserts the stamp file exists with the expected matrix content.
- TestE2E_OnTest_FailureSurfaces: builds test/testfail with runTest=true
and asserts Build() returns an error wrapped with
"onTest failed for test/testfail@1.0.0".
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the llar test command and an onTest hook for formulas, enabling post-build verification. The changes include refactoring the host matrix logic, updating the builder to handle test execution, and adding comprehensive tests. The review feedback highlights opportunities to optimize the test build process by limiting cache bypass and hook execution to the target module only, rather than applying it to all transitive dependencies. There is also a suggestion to use the command's context for better cancellation support.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #109 +/- ##
==========================================
+ Coverage 78.11% 79.69% +1.58%
==========================================
Files 34 35 +1
Lines 1855 1921 +66
==========================================
+ Hits 1449 1531 +82
+ Misses 303 285 -18
- Partials 103 105 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
The initial implementation applied RunTest semantics to every module in
the build list: OnTest was invoked for each module, and the build cache
was bypassed for all of them. That wastes work and contradicts the test
product design (issues/106 §9):
onTest runs only after the main module build completes.
Narrow the scope so only the root target — targets[0] in the MVS build
list — is treated specially under runTest:
- Cache read/write are bypassed for the root only; dependencies still
honor the existing cache, preserving fast iteration on shared deps.
- OnTest is invoked on the root only; a dependency that happens to
define OnTest is NOT triggered by a test run whose user-facing target
is a downstream consumer. Dep maintainers verify their packages via
their own `llar test <dep>` invocations.
Implementation notes:
- Root identity is captured once at the top of Build() and matched by
pointer equality inside the per-module closure. constructBuildList
already treats targets[0] as the root, so the invariant is shared.
- Cache write for the root under runTest remains disabled: OnTest may
leave side-effects inside installDir (e.g. a stamp file written by the
e2e testhook fixture) that normal `llar make` must not inherit.
Tests:
- All three existing RunTest tests continue to pass unchanged because
they use test/liba as a single-module target (isRoot is trivially
true).
- Two new tests exercise the multi-module case via test/depresult
(root) depending on test/liba (dep):
* DepOnTestNotInvoked: OnTest injected onto the dep is never
called, only the root's OnTest runs.
* DepCacheStillUsed: a pre-populated dep cache entry is consulted
under runTest=true, proving deps short-circuit through cache.
Aggregate coverage across the four MVP-affected packages now reaches
91.4% of statements. Per-package improvements:
- formula/ 82.1% -> 100.0%
- internal/build/ 92.4% (unchanged; already above target)
- internal/formula/ 88.9% (unchanged; ixgo-internal error paths are
difficult to exercise without invasive
test doubles)
- cmd/llar/internal/ 67.6% -> 84.1%
New test files:
* formula/classfile_test.go
Covers the classfile DSL surface by constructing a minimal type that
embeds ModuleF and provides a MainEntry driving every setter. Going
through Gopt_ModuleF_Main also exercises the unexported app() helper.
* cmd/llar/internal/test_test.go
Mirrors runMakeCmd with runTestCmd and exercises runTest failure
paths reachable without network:
- invalid local ".@Version" syntax
- newRemoteStore returning an error
- local ./@Version with a mock VCS (build fails at git sync,
which still exercises the whole local branch)
- local path pointing at a nonexistent module
The success return-nil path is intentionally not covered: runTest
bypasses cache for the root, so the happy path requires a real git
sync which is out of scope for short-mode unit tests.
Extended test files:
* formula/project_test.go
Adds TestNewContext for the public constructor and TestContext_OutputDir
for the OutputDir__0 / OutputDir__1 overloads, verifying that the
dep-routing variant threads the active matrix and module.Version
through the injected getOutputDir callback.
No production code is touched by this commit.
|
/review |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
A reviewer flagged that root identity in Builder.Build used pointer
equality:
var root *modules.Module
if len(targets) > 0 {
root = targets[0]
}
...
isRoot := mod == root
This works today only because constructBuildList happens to reuse the
same *modules.Module pointers from the input slice. It is a fragile
invariant — once constructBuildList clones module structs (for
example to support the "Parallel build" TODO on line 329), the
comparison silently starts returning false for the root and:
* bypassCache stays false, so runTest reads/writes the build cache
for the root (defeating the whole point of "fresh build").
* OnTest never fires, so `llar test` becomes a silent no-op.
Both failure modes are invisible to existing tests.
Switch to (Path, Version) value comparison, which is the real identity
invariant we care about. The pair is unique in an MVS build list, so
it is a safe identity key regardless of whether *modules.Module
pointers are reused.
No behavior change in the current pointer-reuse regime:
- len(targets)==0: rootID is zero value {"", ""}; no real module has
empty Path/Version (modules.Load guarantees both are set), so isRoot
stays false — same as the previous "mod == nil" branch.
- len(targets)>0: rootID matches exactly one module in the build list,
and pointer identity in the current constructBuildList also matches
exactly one module. Same set of modules flip isRoot true.
Verified:
go test -ldflags="-checklinkname=0" ./internal/build/... passes,
including TestBuild_RunTest_* which exercises bypassCache and OnTest
gating on the root and asserts deps are NOT tested / DO use cache.
Existing docstring also extended to document the invariant so future
refactorers (especially for parallel builds) do not reintroduce a
pointer-equality check.
The old godoc on buildModule claimed:
// buildModule loads and builds a single module. When runTest is
// true, the builder also runs each module's onTest hook after
// onBuild succeeds and bypasses the build cache.
Both claims were inaccurate after 0320afe limited runTest semantics
to the root target:
* OnTest fires only on the root target (see build.go:283
`if b.runTest && isRoot && mod.OnTest != nil`). Transitive
dependencies with their own OnTest hooks are NOT triggered.
* Cache read and cache write are bypassed only for the root
(see build.go:214-217 and build.go:291-294). Dependencies still
short-circuit through cache lookup when available.
Rewrite the docstring to match the implementation, consistent with
build.Options.RunTest's field comment, and note the `llar test <dep>`
per-dependency verification model.
No behavior change.
The onTest hook previously reused classfile.BuildResult as its output
type. That worked functionally but polluted the DSL surface:
- A .gox author writing `onTest (ctx, proj, out) => ...` saw
`out.setMetadata` in IDE completion even though metadata has no
meaning in a test context.
- `out.setMetadata "-lfoo"` inside onTest would silently do nothing
because the builder discards testOut.metadata, leading to
confusing no-op behavior.
- Semantic overloading of BuildResult made future evolution awkward:
extending BuildResult for tests (pass/fail counts, captured logs,
skip markers) would bloat the build type as well.
Split onTest's output into a dedicated type:
type TestResult struct {
errs []error
}
func (t *TestResult) AddErr(err error)
func (t *TestResult) Errs() []error
The method name `AddErr` is preserved, so existing .gox formulas that
call `out.addErr` continue to work without changes. Only the Go-side
signature of ModuleF.OnTest changes, plus the callers in the loader,
the builder, and mock tests.
Files touched:
- formula/classfile.go
Add TestResult with AddErr/Errs. Change fOnTest and OnTest setter
to take *TestResult instead of *BuildResult.
- formula/classfile_test.go
Update the DSL setter mock for OnTest.
- internal/formula/formula.go
Change Formula.OnTest field type to the new signature. Update the
reflection cast in loadFS to match fOnTest's new type.
- internal/formula/formula_test.go
Pass *formula.TestResult into the extracted OnTest callback.
- internal/build/build.go
Use classfile.TestResult for testOut. Errs() still surfaces onTest
failures through the existing fmt.Errorf("onTest failed for ...")
wrapper; no behavioral change.
- internal/build/build_test.go
Update the 5 m.OnTest mock assignments.
- internal/ixgo/pkg/github.com/goplus/llar/formula/export.go
Register TestResult in the xgo interpreter's NamedTypes table so
.gox onTest blocks can resolve *formula.TestResult at runtime.
(This file is machine-generated by qexp, but regenerating is
overkill for a single entry.)
No changes to .gox fixtures or user-facing CLI.
Verification:
- go build -ldflags="-checklinkname=0" ./... passes
- go test -ldflags="-checklinkname=0" on ./formula/..., ./internal/formula/...,
./internal/build/..., ./cmd/llar/internal/ all pass, including the
two e2e tests that exercise the full xgo interpretation of
`onTest ... => { out.addErr err }` against TestResult.
- Pre-existing TestMakeReal_* failures (unrelated; workspaces dir
owned by root on the dev machine) are not in this test sweep.
- Drop bypassCache flag; onTest no longer forces a rebuild. - On cache hit during a test run, skip onBuild but still run onTest against the cached artifacts. - On cache miss, run onBuild, persist the cache, then run onTest. - onTest is invoked only on the root target. - Split TestBuild_RunTest_BypassesCache into TestBuild_RunTest_ReusesCacheWhenHit and TestBuild_RunTest_SavesCacheOnMiss to reflect the new semantics. - Update godoc on Options.RunTest, buildModule, and testCmd.Long.
|
/review |
3 similar comments
|
/review |
|
/review |
|
/review |
There was a problem hiding this comment.
Review Summary
Well-structured PR that adds a cleanly scoped onTest DSL callback and llar test CLI command. The cache logic refactoring in Build() is thoughtful, the TestResult type separation from BuildResult is a good design call, and the root-target-only gating via (Path, Version) value comparison is robust. Test coverage is thorough across unit, integration, and E2E layers.
A few items worth considering below — one performance concern around unnecessary git clones on cache hits, a couple of stale comments from the earlier "bypass cache" iteration, and some minor code organization suggestions.
| // Identify the root target. By MVS convention (see constructBuildList | ||
| // and modules.Load), targets[0] is the main module requested by the | ||
| // caller; runTest semantics (fresh build + OnTest invocation) only | ||
| // apply to it. |
There was a problem hiding this comment.
Stale comment: "fresh build" is misleading after cache-reuse refactor
The parenthetical says "fresh build + OnTest invocation" but after the latest refactor, runTest no longer forces a fresh build — on a cache hit, OnBuild is skipped entirely (line 288). Consider rewording to something like "build-or-cache-reuse + OnTest invocation" or simply removing the parenthetical, since the full semantics are already documented in Options.RunTest godoc above.
| // Run OnTest (root only) against the just-built or cached | ||
| // artifacts, reusing the same build context so tests see a | ||
| // consistent environment either way. | ||
| if testThisMod { |
There was a problem hiding this comment.
Integrity: OnTest receives full write access to installDir
The OnTest callback receives the same *Context as OnBuild, including write access to installDir via ctx.OutputDir(). The testhook test fixture demonstrates this by writing ontest.stamp into the install dir. Since the cache is not re-saved after OnTest (line 312), any files a buggy/malicious test hook writes into installDir persist and would be visible to downstream dependents consuming cached artifacts.
This may be intentional for the MVP, but consider documenting the contract ("test hooks must not modify installDir") or, in a future iteration, providing OnTest with a read-only view or a separate scratch directory.
| // OnTest event is used to run post-build verification for a project. | ||
| // It fires after OnBuild has completed successfully, reusing the same build |
There was a problem hiding this comment.
Docs: misleading for cache-hit path
This says "It fires after OnBuild has completed successfully" but on a cache hit, OnBuild never runs — OnTest fires against cached artifacts directly. Since this is the formula-author-facing API doc, consider rewording to: "It fires after build artifacts are available (either freshly built or reused from cache)".
| savedVerbose := makeVerbose | ||
| makeVerbose = testVerbose | ||
| defer func() { makeVerbose = savedVerbose }() |
There was a problem hiding this comment.
Code quality: shared mutable state for verbose flag
The save/mutate/restore of makeVerbose works because CLI commands are single-threaded, but it makes buildModule depend on a global side-channel. If parallelism is ever introduced (the TODO at build.go:342 mentions parallel builds), this becomes a data race.
Consider passing verbose as an explicit parameter to buildModule (or bundling it into an options struct alongside runTest), which would eliminate this ceremony entirely.
| return err | ||
| } | ||
|
|
||
| if !isLocal { |
There was a problem hiding this comment.
Nit: code duplication with runMake
Lines 55-83 duplicate the local module resolution logic from runMake (make.go:79-110) almost verbatim — the only difference is the final true/false argument to buildModule. Consider extracting a shared helper like resolveAndBuild(ctx, remoteStore, pattern, version, matrixStr, runTest) to keep the two commands in sync as the local resolution logic evolves.
There was a problem hiding this comment.
这里 OnTest 复用了带有可写 installDir 的 build context，test 有可能就有可能对产物文件产生修改，这里看了一下 brew 的 测试，会有一个临时目录去作为测试运行的环境，对于构建出产物就像普通用户一样是读取消费，这样边界是否可能会更清晰
There was a problem hiding this comment.
onTest这里并不是写错了，而是有意为之，这主要是因为onTest需要使用构建产物才能完成编译
一个onTest E2E例子如下：
tc := cmake.new(testSrc, testBuild, testBuild+"/_out")
tc.buildType "Release"
tc.define "CMAKE_POLICY_VERSION_MINIMUM", "3.5"
tc.use installDirThere was a problem hiding this comment.
或者说我们现在对测试逻辑就是预期对installDir可写的嘛
There was a problem hiding this comment.
其实是因为我们没办法控制它不可写，我们不是操作系统没有控制它仅可写的权限
- New fixture test/cmaketest: tiny static library (libcmtadd) with a sibling tests/ project that links against the installed library and asserts cmt_add(2,3) == 5. - Cmaketest_llar.gox onBuild: standard cmake configure/build/install. - Cmaketest_llar.gox onTest: cmake.new on tests/, cmake.use(installDir) for prefix injection, configure+build, exec the check binary, write ontest.stamp on success. - TestE2E_OnTest_RealCMakeBuild verifies the full path: real linkable artifact, TestContext/TestResult wiring through interpreted code, cmake.use prefix injection, and exec/lastErr exit code propagation. - TestE2E_OnTest_RealCMakeBuild_ReusesCacheOnTestRerun is the headline cache-reuse regression: phase 1 populates cache with runTest=false, phase 2 runs runTest=true on the same workspace and asserts the build cache mtime is unchanged (onBuild skipped) yet ontest.stamp is written (onTest still ran against the cached install tree).
| // Build the test program in its own tree so onTest works identically | ||
| // on cache hit (where onBuild was skipped and no _build dir exists) | ||
| // and on cache miss (where onBuild just populated _build). | ||
| testSrc := ctx.SourceDir + "/tests" | ||
| testBuild := ctx.SourceDir + "/_testbuild" |
There was a problem hiding this comment.
在homebrew 里会存在一个helper 目录，由主流程提供一个 testpath,然后在这个文件夹里做测试，结束之后清理回收，我们需要提供一个helper么，否则其实还会挺纠结，测试到底在哪里执行的问题
There was a problem hiding this comment.
这里提供源码目录是因为，测试集一般会随着源码分发，但其实这个源码目录，本来就是临时的...
2 participants
Design: #106