fix(gnovm): reset package state between test functions#5577
Open
notJoon wants to merge 8 commits intognolang:masterfrom
Open
fix(gnovm): reset package state between test functions#5577notJoon wants to merge 8 commits intognolang:masterfrom
notJoon wants to merge 8 commits intognolang:masterfrom
Conversation
Collaborator
🛠 PR Checks Summary🔴 Pending initial approval by a review team member, or review from tech-staff Manual Checks (for Reviewers):
Read More🤖 This bot helps streamline PR reviews by verifying automated checks and providing guidance for contributors and reviewers. ✅ Automated Checks (for Contributors):🟢 Maintainers must be able to edit this pull request (more info) ☑️ Contributor Actions:
☑️ Reviewer Actions:
📚 Resources:Debug
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Per-test `NewPackageInstance` gives each test a fresh `PackageValue` for the package under test, but mutations to *imported* realms were leaking through two paths: 1. The initial tgs seed ran test-file `init()` functions, persisting their cross-package effects (e.g. `gov/dao.allowedDAOs` set by a test's `InitWithUsers` call) into shared state before any test ran. 2. Per-test inner transactions shared their `baseStore` with `tgs`, so realm finalization writes (`SetObject` → `baseStore.Set`) propagated to tgs and to every sibling test. Fix both: - Add `Machine.RunMemPackageSkipTestFileInits`; `runMemPackage` drops init.* FuncValues from test files (via `IsTestFile`) before calling `runInitFromUpdates`. File parsing, preprocessing, and top-level var decls still run, so xxx_test integration imports see test-file symbols. Per-test machines re-run all inits on their fresh pv as before. - `runTestFiles` CacheWraps the base/iavl stores per test iteration so realm finalization writes stay local to the inner txn; dropping innerBase without `Write()` discards the mutations. Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
gno testpreviously shared a singlePackageValueacross every test function in a package, so mutation to package-level variables in one test leaked into the next. This PR runs each test against a freshly instantiatedPackageValuecreated from the preprocessedPackageNode, inside its own nested transaction store. Package-level globals andinit()side effects start from a clean slate for every test.fixes #1982
Problem
The reproducer from the original issue:
Both tests assume
countstarts at0, so a singleIncrement()call should leave it at1. On master branch,TestIncrement1passes(count goes from 0 to 1), butTestIncrement2failed:countcarries over as1from the first test,Increment()takes it to2.Any test relying on a zero-initialized package global is fragile under the old behavior, with outcomes depending on which tests ran before it.
Why it happeneds on master
runTestFilesloaded the package once and reused the resultingPackageValueacross everytfin thetestsloop:gno/gnovm/pkg/test/test.go
Lines 391 to 405 in 066f15a
pv.Block.Valuesholds the mutable package-level variables. Even though each iteration creates a new machine,SetActivePackage(pv)attaches the samepv, so mutations from test N remain visible in test N+1. The pre-existing TODO at that site already flagged this limitation, noting that a proper fix required a way to create a freshPackageValuewithout re-parsing the source.Fix requirements
The fix needs three properties:
PackageValuefor each test so package-level variables are re-initialized from their declarations andinit()runs again.*_test.gnoinit()functions that mutate imported-realm state (e.g. a test file callingdao.UpdateImplto set allow-lists) must not leak into the shared seed, or every test would inherit that pollution before it even starts.baseStore, since realm finalization during a test writes throughSetObject→baseStore.Setand would otherwise persist into sibling tests via the shared cache.Naively re-running
RunMemPackage(mpkg, false)per test satisfies (1) but re-parses and preprocesses every file on every test, which regresses performance significantly (see Performance section below). The fix instead separates preprocessing from runtime instantiation: preprocess once, then hand the preprocessedPackageNodeto each test to cheaply instantiate its ownPackageValueinside a nested transaction.Solution
Two changes.
1.
gnovm/pkg/gnolang/machine.goSplit
runFileDeclsinto a preprocesses phase and a runtime instantiation phase. The instantiation half becomesinstantiatePackageFiles, which:*Blocks onm.Packagewith values copies from each preprocessedFileNode.StaticBlock.PrepareNewValuesto populatepv.Block.Values.Meanwhile,
runFileDeclskeeps its existing behavior (preprocess + initantiate in one call) by delegating its second half toinstantiatePackageFiles, so nothing outside the test runner sees a behavior change. A new public methodNewPackageInstanceproduces a freshPackageValuefrom an already processedPackageNode:The explicit
pv.GetBlock(m.Store).Valuesargument is load-bearing:pn.NewPackagealready callsPrepareNewValuesinternally, so the second call insideinstantiatePacageFilesseesppl == ppland returnsnil. Feeding thosenilupdates torunInitFromUpdateswould silently skip everyinit()function, so we pass the values thatpn.NewPackageactually populated instead(related test:init_and_isolation.txtar).2.
gnovm/pkg/test/test.gorunTestFilesstill preprocesses the package once, but now on the outer transaction store. The seed uses a newRunMemPackageSkipTestFileInitsentry point so thatinit()functions in*_test.gnofiles are not executed against the sharedtgs; file parsing, preprocessing, and top-level var declarations still run so that xxx_test integration imports continue to see test-file symbols. Each test then runs in its own nested transaction with a freshly instantiatedPackageValueand a CacheWrap'd base store, so realm-finalization writes stay local to that test:RunMemPackageSkipTestFileInitsthreads askipTestFileInitsbool through torunInitFromUpdates, where init.*FuncValues whoseFileNamematchesIsTestFileare skipped inline alongside the existing init-detection check — no extra pass overupdates, no duplicated predicate.NewPackageInstanceandRunFilescallrunInitFromUpdateswithskipTestFileInits=false, so the per-test freshpvstill runs everyinit(), including test-file ones.innerBase := baseStore.CacheWrap()gives each test its own overlay on top oftgs's base store, so realm finalization writes performed during the test (e.g. an imported realm's block being persisted at a crossing-call boundary) stay oninnerBase. DroppinginnerBasebetween iterations — we never callWrite()on it — discards those writes, which is what keeps mutations to imported realms from bleeding between tests.The
tmpkgfilter handles a type system characteristic:AsRunnable()demotesMPUser/StdlibAlltoProd, which strips*_test.gnofiles unless the package is pre-filtered withMPTest, TheIsAll()guard avoids passing integration types throughMPFTest, which panics on them since they already carry their test files.How it works
Flow under the fix:
tgsrunsRunMemPackageSkipTestFileInitsonce, parsing and preprocessing the package. Non-testinit()functions run ontgsas usual; test-fileinit()functions are deliberately skipped so they cannot mutate imported-realm state in the shared seed. The preprocessedPackageNodelives intgs.cacheNodes.tgs.BeginTransaction(innerBase, innerBase, ...)returnsinnerTxn, a nested transaction with its owncacheObjectsmap for object writes andinnerBase = baseStore.CacheWrap()layered on top of the outer base store so realm-finalization writes stay local.cacheNodesis shared viatxlog.Wrap, soinnerTxnsees the preprocessedpnwithout re-processing.m.NewPackageInstance(pn)creates a freshPackageValue:pn.NewPackage(m.Alloc)allocates a newpv.Blockand seedspv.Block.Valuesfrompn.Values, so the init function values are already in place.instantiatePackageFilescreates fresh file blocks and runs top-level var declarations in dependency order, re-initializing every package global from its source expression.runInitFromUpdates(pv, pv.Block.Values)schedules thoseinit()functions — including test-file ones — for execution against the clean per-test pv.pvunderinnerTxn. Any mutations to package globals live in the newpv.Block.Values; any object writes go toinnerTxn.cacheObjects, and realm finalizations that write through to the base store land oninnerBaseinstead oftgs's base.innerTxnandinnerBaseare dropped without callingWrite(), so both thecacheObjectsoverlay and the base-store overlay are discarded. The next iteration starts from scratch againsttgs.Under this flow,
TestIncrement1starts withcount == 0, increments it to1, and its mutation dies withinnerTxn.TestIncrement2then starts from a freshpvwherecount == 0again, and observescount == 1as expected.Performance
Re-running
RunMemPackageper test regresses heavy packages by about 9x, because parse and preprocess repeat every test function. Splitting preprocess-once from instantiate per test brings the overhead down to a modest constant per test function.Wall-clock measurement:
gno test .on each package, median of 5 runs after warm-up run.masteris 066f15a,branchis this PR. Both binaries are built with identical flags and run against the same sources, and subtest counts were verified equal between them.p/onbloc/uint256p/onbloc/int256p/onbloc/jsonThe overhead is the cost of per-test
PackageValuecreation and re-runninginit(). Preprocessing happens once per package and is amortized across all tests. The measured overhead is consistent across the three packages -- roughly 1.7 to 1.8ms per test function -- so the cost scales with test function count rather than file size. Packages with expensiveinit()will see proportionally more.Known limitation
PackageValuecreation andinit()re-execution is strictly more work than sharing a singlePackageValueacross tests. See the Performance section for measurements.PackageNodeis immutable at runtime, which letstgs.cacheNodesbe shared across tests viatxlog.Wrap. If a future change makesPackageNode mutable, this assumption needs to be revisited.runTestFiles. Filetests and other entrypoints still userunFileDeclsdirectly and keep their prior semantics; those paths already run each test against its own package state, so no change was needed.examples/gno.land/r/(sys/params,tests/vm/map_delete,demo/todolist,leon/hor,archive/nir1218_evaluation_proposal,gov/dao/v3/impl) were written against the old leaky behavior — e.g. expecting an item created in one test to persist into the next, or hard-coding proposal IDs that assumed state carried over from prior tests. They are updated in this PR to set up their own preconditions; new tests added after this change should follow the same pattern.