fix(gnovm): PrevRealm in _test files #896

tbruyelle · 2023-06-14T15:17:29Z

Description

Related to #667 #891

When _test files are executed, the name of the package is set to the real path to those files, which starts by ./examples/gno.land/.... This leads to interpreting the tested package as a non-realm, because a realm must start with gno.land/r.

Because of that, when calling PrevRealm, a tested package is never considered as a realm.

~~The fix truncates the ./examples prefix from the tested package, which ensures it is well detected as a realm when required.~~
The fix replaces the usage of the package directory /examples/gno.land/... by a real gno package path.

To determine it, we try to read a gno.mod in the directory passed in argument or in its parents:

If found, the gno package path is set to the module name found in gno.mod.
If not found, a randomly generated one is used, that will start with gno.land/r/ (so it will be considered as a realm).

Tests

A test TestPrevRealm has been added:

$ gno test -verbose ./examples/gno.land/r/demo/tests -run TestPrevRealm 
// FAIL with gno binary compiled from master
// OK with gno binary compiled from this PR

Contributors Checklist

Added new tests, or not needed, or not feasible
Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
Updated the official documentation or not needed
No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
Added references to related issues and PRs
Provided any useful hints for running manual tests

Maintainers Checklist

Checked that the author followed the guidelines in CONTRIBUTING.md
Checked the conventional-commit (especially PR title and verb, presence of BREAKING CHANGE: in the body)
Ensured that this PR is not a significant change or confirmed that the review/consideration process was appropriate for the change

r3v4s · 2023-06-15T04:59:39Z

Hmm.. unfortunately It doesn't work on my side.

$ git rev-parse HEAD
c2c716420c67369cc58584e10b9ac342676b8374

$ gno test -root-dir ~/0615/gno -verbose=true ./
=== RUN   TestAssertOriginCall
--- PASS: TestAssertOriginCall (0.00s)
=== RUN   TestPrevRealm
--- FAIL: TestPrevRealm (0.00s)
output: want GetRSubtestsPrevRealm().Addr==g1gz4ycmx0s6ln2wdrsh4e00l9fsel2wskqa3snq, got g1wymu47drhr0kuq2098m792lytgtj2nyx77yrsm
=== RUN   file/z0_filetest.gno
--- PASS: file/z0_filetest.gno (0.00s)
=== RUN   file/z1_filetest.gno
--- PASS: file/z1_filetest.gno (0.00s)
./.: test pkg: failed: "TestPrevRealm"
FAIL
FAIL    ./. 	0.47s
FAIL
?       ./subtests 	[no test files]
FAIL
FAIL: 0 build errors, 1 test errors

tbruyelle · 2023-06-15T08:23:26Z

@r3v4s Sorry to ask, but are you sure that you recompiled the gno binary with this PR ?

r3v4s · 2023-06-15T08:44:09Z

@tbruyelle Yep, I did.
I've removed all of existing gno related binary, then ran make install

can you share me pre-built gno binary? (I'm using m1)

tbruyelle · 2023-06-15T15:36:45Z

@tbruyelle Yep, I did. I've removed all of existing gno related binary, then ran make install

can you share me pre-built gno binary? (I'm using m1)

If you run make install at the root directory, that's enough, I don't understand why the test isn't working. I can't reproduce on my side, it's working well.

examples/gno.land/r/demo/tests/tests.gno

gnovm/cmd/gno/test.go

moul · 2023-06-17T11:30:41Z

Can you also check how it goes with external repos? (Not using examples/)?

Will give a deeper look of the diff later, thank you.

tbruyelle · 2023-06-20T12:12:40Z

Can you also check how it goes with external repos? (Not using examples/)?

Will give a deeper look of the diff later, thank you.

Thank you. Do you have any example of external repos that contains gno packages ?

tbruyelle · 2023-06-22T12:31:13Z

Can you also check how it goes with external repos? (Not using examples/)?

Will give a deeper look of the diff later, thank you.

@moul So thanks to yesterday meeting I found the gnodaos external repo and I tried to use the gno binary from this PR.

After a couple of changes :

fix gnodaos code because it is based on gno test3, so for instance it uses gno.land/p/avl while now it's gno.land/p/demo/avl
fix panic in gno so we don't try to assign an ID to non realm (see commit 336edab)

I was able to run the tests successfully ✔️

Note that gno.land/r/gnodaos is actually a realm, but it's not detected as is because the pkg path in the code is ./r/gnodaos, and again a realm must start with gno.land/r/. I think this will be mitigated by reading the pkg path in the gno.mod, and @harry-hov already has something in the oven for that (see conversation).

So the plan for this PR is to wait for #682, then use the pkgPath found in gno.mod, which should avoid the need for stripping the ./examples prefix.

anarcher · 2023-06-23T07:37:35Z

On the master branch, the following command works fine, but on this PR I get the following error.
(I built a fresh gno with make install. )

$ gno test -verbose ./examples/gno.land/r/demo/tests -run TestPrevRealm
=== RUN   TestAssertOriginCall
panic: cannot modify external-realm or non-realm object

goroutine 1 [running]:
github.com/gnolang/gno/gnovm/pkg/gnolang.(*Realm).DidUpdate(0x140002c6460, {0x1011da070, 0x1400041c000}, {0x0, 0x0}, {0x0, 0x0})
        /Users/anarch/go/blockchain/src/gno/gnovm/pkg/gnolang/realm.go:148 +0x270
github.com/gnolang/gno/gnovm/pkg/gnolang.PointerValue.Assign2({0x14008e094a0, {0x1011c8ea0, 0x1400041c000}, 0x14, 0x0}, 0x3?, {0x1011dac38, 0x14000166120}, 0x140002c6460, {{0x1011cee88, ...}, ...}, ...)
        /Users/anarch/go/blockchain/src/gno/gnovm/pkg/gnolang/values.go:281 +0x530
github.com/gnolang/gno/gnovm/pkg/gnolang.(*Machine).doOpAssign(0x14000122b40)
        /Users/anarch/go/blockchain/src/gno/gnovm/pkg/gnolang/op_assign.go:44 +0xd0
github.com/gnolang/gno/gnovm/pkg/gnolang.(*Machine).Run(0x14000122b40)
        /Users/anarch/go/blockchain/src/gno/gnovm/pkg/gnolang/machine.go:1251 +0xed4

I'm not sure, but could these two IDs be different?

	if po.GetObjectID().PkgID != rlm.ID {
		panic("cannot modify external-realm or non-realm object")
	}

tbruyelle · 2023-06-23T09:03:08Z

@anarcher wow thank you for pointing that, it appears this only happens when the -run flag is provided !

So the fix for that is the following :

diff --git a/gnovm/stdlibs/testing/match.gno b/gnovm/stdlibs/testing/match.gno
index 5b7f509d..13a7edd4 100644
--- a/gnovm/stdlibs/testing/match.gno
+++ b/gnovm/stdlibs/testing/match.gno
@@ -166,13 +166,13 @@ func isSpace(r rune) bool {
 	return false
 }
 
-var (
-	matchPat string
-	matchRe  *regexp.Regexp
-)
-
 // based on testing/internal/testdeps.TestDeps.MatchString.
 func matchString(pat, str string) (result bool, err error) {
+	var (
+		matchPat string
+		matchRe  *regexp.Regexp
+	)
+
 	if matchRe == nil || matchPat != pat {
 		matchPat = pat
 		matchRe, err = regexp.Compile(matchPat)

(this patch should be reworked, there's no reason to turn the global var into local var like that, but that's for the demo)

The funny thing behind this fix is :

since this PR turned the test into a realm
any global variables used in this realm or in any package dependencies are aimed to be stored at the end of the transaction
when -run is provided, it uses testing/match.go which updates 2 global variables for caching
those 2 global variables doesn't share the same package ID as the test realm, so the VM panics.

You can have the same behavior by calling a realm that calls a package that mutates its own global variables.

So that means with this PR, we can no longer have global variables in testing or in any imported packages in a test. Do you think that's still acceptable ? (cc @moul)

moul · 2023-06-27T06:51:00Z

To keep track of the previous discussion:

When persisting the snapshot of the finished state, exclude globals from _test.gno files in realms' store. This offers flexibility in _test.gno files while maintaining stricter controls in realms.

tbruyelle · 2023-07-03T08:55:37Z

@moul I reverted the last change so now when the tx is finalized, the realm's global variables that come from *_test.gno files are ignored.

That works for test files, and for the template file generated at runtime (because it has been renamed from testmain.go to main_test.gno), but unfortunately that doesn't work for testing/match.gno because the file name doesn't end with *_test.gno.

I didn't find a way to determine that testing/match.gno is a dependency of a *_test.gno file, and so should inherit its property of being ignored by the realm persistency code. And because it could potentially be a dependency of multiple other files (with some that aren't *_test.gno files), I believe this is not straightforward to do so.

On the other hand, I found some other gno code imported from the stdlib, where the global variables are commented with the label masterRNG deleted to make package pure. So maybe we should simply remove/comment those global variables from testing, and make the package pure. WDYT?

moul · 2023-07-03T18:43:33Z

Thinking out loud, what about ignoring files importing « testing »?

tbruyelle · 2023-07-03T21:33:19Z

Thinking out loud, what about ignoring files importing « testing »?

I believe no production code would import testing, so that could work. But that doesn't solve the problem, we also need to ignore testing itself.

tbruyelle · 2023-07-04T14:55:16Z

@moul I was able to ignore the testing package only in test environement, by introducing a condition in the TestStore. Now with the other condition related to _test files, I have something that works, even if it looks like an ugly hack :/

not so well and add too much diff

This reverts commit 763ac29.

moul · 2023-08-03T16:35:52Z

gnovm/pkg/doc/dirs.go

@@ -70,6 +70,9 @@ func newDirs(dirs []string, modDirs []string) *bfsDirs {

 // tries to parse gno mod file given the filename, using Parse and Validate from
 // the gnomod package
+//
+// TODO(tb): replace by `gnomod.ParseAt` ? The key difference is the latter


yes, and later, gnomod.ParseAt can also support new ways to guess a module when there is no gno.mod file.

moul · 2023-08-03T16:38:33Z

gnovm/pkg/gnolang/realm.go

@@ -1121,6 +1121,10 @@ func copyValueWithRefs(parent Object, val Value) Value {
 		}
 	case *FuncValue:
 		source := toRefNode(cv.Source)
+		if strings.HasSuffix(source.Location.File, "_test.gno") {


Suggested change

if strings.HasSuffix(source.Location.File, "_test.gno") {

func (v Val) InTestFile() bool {return strings.HasSuffix(source.Location.File, "_test.gno")}

if val.InTestFile()) {

can be done later

We don't have to change it now. I created an issue so that we can track it for later discussion.

Dose this mean if someone deployed a realm package with a source file named xyz_test.gno it will be ignored by the VM? If that is the case, it means we should include this file naming convention to the VM specs.

Also If we do not allow _test.gno files in the realm package, we should guarded it before we parse the file and load it to VM, instead of at the persistent stage.

If this purely serves the purpose for off chain realm testing, would sticking with realm file test be a better option?

gnovm/pkg/gnomod/parse.go

Co-authored-by: Manfred Touron <94029+moul@users.noreply.github.com>

apply gnolang/gno#896

`TestPackages` wasn't actually reporting test failures, due to a bug in the underlying function `machine.TestMemPackage()`. `TestMemPackage` was running the gno tests by just calling the test function with a new `testing.T`, but then it's not possible to detect if there's any failure, because `testing.T` has only private fields. The fix is to change that to a call to `testing.RunTest()` function, which is already used by `gno test`, and returns a `testing.Report` on which we can detect failures. A test has been added (see `TestMachineTestMemPackage()`) in gnovm/tests folder (because a `TestStore` is needed). On master, this test is failing. ``` go test ./gnovm/tests -v -run TestMachineTestMemPackage ``` The next step should be to merge the 2 ways that currently exists to run gno tests, i.e. `gno test` and `go test -run TestPackages`. Potentially this could fix #896. ## Contributors Checklist - [x] Added new tests, or not needed, or not feasible - [ ] Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory - [ ] Updated the official documentation or not needed - [ ] No breaking changes were made, or a `BREAKING CHANGE: xxx` message was included in the description - [ ] Added references to related issues and PRs - [x] Provided any useful hints for running manual tests - [ ] Added new benchmarks to [generated graphs](https://gnoland.github.io/benchmarks), if any. More info [here](../.benchmarks/README.md). ## Maintainers Checklist - [x] Checked that the author followed the guidelines in `CONTRIBUTING.md` - [x] Checked the conventional-commit (especially PR title and verb, presence of `BREAKING CHANGE:` in the body) - [x] Ensured that this PR is not a significant change or confirmed that the review/consideration process was appropriate for the change </details> --------- Co-authored-by: Manfred Touron <94029+moul@users.noreply.github.com>

@jaekwon

Asked by @jaekwon in #896 (comment) Add a long description for `gno test -h`, that explains the different files, `*_test.gno` and `*_filetest.gno`, and gives the list of instructions related to the latter. Thanks in advance for correcting my frenglish :) <details><summary>Contributors' checklist...</summary> - [ ] Added new tests, or not needed, or not feasible - [ ] Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory - [ ] Updated the official documentation or not needed - [ ] No breaking changes were made, or a `BREAKING CHANGE: xxx` message was included in the description - [ ] Added references to related issues and PRs - [ ] Provided any useful hints for running manual tests - [ ] Added new benchmarks to [generated graphs](https://gnoland.github.io/benchmarks), if any. More info [here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md). </details> --------- Co-authored-by: Manfred Touron <94029+moul@users.noreply.github.com> Co-authored-by: Morgan <git@howl.moe>

Co-authored-by: Manfred Touron <94029+moul@users.noreply.github.com>

`TestPackages` wasn't actually reporting test failures, due to a bug in the underlying function `machine.TestMemPackage()`. `TestMemPackage` was running the gno tests by just calling the test function with a new `testing.T`, but then it's not possible to detect if there's any failure, because `testing.T` has only private fields. The fix is to change that to a call to `testing.RunTest()` function, which is already used by `gno test`, and returns a `testing.Report` on which we can detect failures. A test has been added (see `TestMachineTestMemPackage()`) in gnovm/tests folder (because a `TestStore` is needed). On master, this test is failing. ``` go test ./gnovm/tests -v -run TestMachineTestMemPackage ``` The next step should be to merge the 2 ways that currently exists to run gno tests, i.e. `gno test` and `go test -run TestPackages`. Potentially this could fix gnolang#896. ## Contributors Checklist - [x] Added new tests, or not needed, or not feasible - [ ] Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory - [ ] Updated the official documentation or not needed - [ ] No breaking changes were made, or a `BREAKING CHANGE: xxx` message was included in the description - [ ] Added references to related issues and PRs - [x] Provided any useful hints for running manual tests - [ ] Added new benchmarks to [generated graphs](https://gnoland.github.io/benchmarks), if any. More info [here](../.benchmarks/README.md). ## Maintainers Checklist - [x] Checked that the author followed the guidelines in `CONTRIBUTING.md` - [x] Checked the conventional-commit (especially PR title and verb, presence of `BREAKING CHANGE:` in the body) - [x] Ensured that this PR is not a significant change or confirmed that the review/consideration process was appropriate for the change </details> --------- Co-authored-by: Manfred Touron <94029+moul@users.noreply.github.com>

@jaekwon

Asked by @jaekwon in gnolang#896 (comment) Add a long description for `gno test -h`, that explains the different files, `*_test.gno` and `*_filetest.gno`, and gives the list of instructions related to the latter. Thanks in advance for correcting my frenglish :) <details><summary>Contributors' checklist...</summary> - [ ] Added new tests, or not needed, or not feasible - [ ] Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory - [ ] Updated the official documentation or not needed - [ ] No breaking changes were made, or a `BREAKING CHANGE: xxx` message was included in the description - [ ] Added references to related issues and PRs - [ ] Provided any useful hints for running manual tests - [ ] Added new benchmarks to [generated graphs](https://gnoland.github.io/benchmarks), if any. More info [here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md). </details> --------- Co-authored-by: Manfred Touron <94029+moul@users.noreply.github.com> Co-authored-by: Morgan <git@howl.moe>

tbruyelle requested review from jaekwon, moul and a team as code owners June 14, 2023 15:17

github-actions bot assigned tbruyelle Jun 14, 2023

github-actions bot added 📦 🤖 gnovm Issues or PRs gnovm related 🧾 package/realm Tag used for new Realms or Packages. labels Jun 14, 2023

tbruyelle mentioned this pull request Jun 14, 2023

feat: std.TestSetPrevRealm #891

Merged

9 tasks

tbruyelle marked this pull request as draft June 14, 2023 15:21

tbruyelle commented Jun 15, 2023

View reviewed changes

examples/gno.land/r/demo/tests/tests.gno Show resolved Hide resolved

r3v4s mentioned this pull request Jun 16, 2023

feat(stdlibs/std)!: namespace minted coins with realm path #875

Merged

9 tasks

tbruyelle commented Jun 16, 2023

View reviewed changes

gnovm/cmd/gno/test.go Outdated Show resolved Hide resolved

tbruyelle force-pushed the fix/test-std.PrevRealm branch from 3f95ce1 to 18eb207 Compare June 21, 2023 11:49

tbruyelle mentioned this pull request Jun 21, 2023

feat: addpkg command respects gno.mod file #922

Draft

7 tasks

tbruyelle force-pushed the fix/test-std.PrevRealm branch from 3b9001b to caa1fc5 Compare June 27, 2023 08:53

tbruyelle force-pushed the fix/test-std.PrevRealm branch 2 times, most recently from b90c4fb to ded4971 Compare July 4, 2023 14:52

tbruyelle added 9 commits July 31, 2023 22:18

remove rename

2f3f30f

not so well and add too much diff

better use the machine API

47fbd41

do not assign id to testmain if it's not a realm

917337b

remove global var from testing

cd3213d

Revert "replace _test filter with change in tests functions"

a3e86bd

This reverts commit 763ac29.

exclude testing from saved objects

85cc135

rename pkgName to gnoPkgPath

a8ab7e9

read gnoPkgPath from gno.mod, if not found, generate a random realm one

13089a5

add comment

7798dfb

tbruyelle force-pushed the fix/test-std.PrevRealm branch from 88d71b4 to 7798dfb Compare July 31, 2023 20:18

more splitRegexp outside the loop

836b71d

tbruyelle mentioned this pull request Aug 1, 2023

docs(gnovm): gno test help long description #1010

Merged

7 tasks

zivkovicmilos self-requested a review August 3, 2023 16:32

moul reviewed Aug 3, 2023

View reviewed changes

gnovm/pkg/gnomod/parse.go Outdated Show resolved Hide resolved

moul approved these changes Aug 3, 2023

View reviewed changes

tbruyelle and others added 2 commits August 4, 2023 14:47

Update gnovm/pkg/gnomod/parse.go

724d3b7

Co-authored-by: Manfred Touron <94029+moul@users.noreply.github.com>

fix after apply suggestion

0404c36

moul merged commit e8016f3 into gnolang:master Aug 6, 2023
47 checks passed

r3v4s added a commit to gnoswap-labs/gnoswap that referenced this pull request Aug 7, 2023

fix: remove TestSet helpers in logic code

201ece0

apply gnolang/gno#896

r3v4s added a commit to gnoswap-labs/gnoswap that referenced this pull request Aug 7, 2023

fix: remove TestSet helpers in logic code

807ef16

apply gnolang/gno#896

tbruyelle deleted the fix/test-std.PrevRealm branch August 7, 2023 09:36

piux2 mentioned this pull request Aug 8, 2023

Discussion: check _test.gno in realm.go #1032

Open

tbruyelle mentioned this pull request Aug 11, 2023

refactor(gnovm): make IsOriginCall/AssertOriginCall rely on PrevRealm #1048

Closed

7 tasks

Doozers pushed a commit to Doozers/gno that referenced this pull request Aug 31, 2023

fix(gnovm): PrevRealm in _test files (gnolang#896)

e2f8505

Co-authored-by: Manfred Touron <94029+moul@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(gnovm): PrevRealm in _test files #896

fix(gnovm): PrevRealm in _test files #896

tbruyelle commented Jun 14, 2023 •

edited

Loading

r3v4s commented Jun 15, 2023

tbruyelle commented Jun 15, 2023

r3v4s commented Jun 15, 2023

tbruyelle commented Jun 15, 2023

moul commented Jun 17, 2023

tbruyelle commented Jun 20, 2023

tbruyelle commented Jun 22, 2023 •

edited

Loading

anarcher commented Jun 23, 2023

tbruyelle commented Jun 23, 2023 •

edited

Loading

moul commented Jun 27, 2023

tbruyelle commented Jul 3, 2023

moul commented Jul 3, 2023

tbruyelle commented Jul 3, 2023

tbruyelle commented Jul 4, 2023

moul Aug 3, 2023

moul Aug 3, 2023 •

edited

Loading

piux2 Aug 8, 2023

	if strings.HasSuffix(source.Location.File, "_test.gno") {
	func (v Val) InTestFile() bool {return strings.HasSuffix(source.Location.File, "_test.gno")}
	if val.InTestFile()) {

fix(gnovm): PrevRealm in _test files #896

fix(gnovm): PrevRealm in _test files #896

Conversation

tbruyelle commented Jun 14, 2023 • edited Loading

Description

Tests

Contributors Checklist

Maintainers Checklist

r3v4s commented Jun 15, 2023

tbruyelle commented Jun 15, 2023

r3v4s commented Jun 15, 2023

tbruyelle commented Jun 15, 2023

moul commented Jun 17, 2023

tbruyelle commented Jun 20, 2023

tbruyelle commented Jun 22, 2023 • edited Loading

anarcher commented Jun 23, 2023

tbruyelle commented Jun 23, 2023 • edited Loading

moul commented Jun 27, 2023

tbruyelle commented Jul 3, 2023

moul commented Jul 3, 2023

tbruyelle commented Jul 3, 2023

tbruyelle commented Jul 4, 2023

moul Aug 3, 2023

Choose a reason for hiding this comment

moul Aug 3, 2023 • edited Loading

Choose a reason for hiding this comment

piux2 Aug 8, 2023

Choose a reason for hiding this comment

tbruyelle commented Jun 14, 2023 •

edited

Loading

tbruyelle commented Jun 22, 2023 •

edited

Loading

tbruyelle commented Jun 23, 2023 •

edited

Loading

moul Aug 3, 2023 •

edited

Loading