Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test files cannot contain a tests variable #1084

Closed
thehowl opened this issue Aug 30, 2023 · 0 comments · Fixed by #3119
Closed

test files cannot contain a tests variable #1084

thehowl opened this issue Aug 30, 2023 · 0 comments · Fixed by #3119
Labels
🐞 bug Something isn't working 📦 🤖 gnovm Issues or PRs gnovm related

Comments

@thehowl
Copy link
Member

thehowl commented Aug 30, 2023

I don't know if the issue is as specific like the title, but here is a minimal example:

package xx

import (
        "testing"
)

type pair [2]uint64

var tests = []pair{
        {
                uint64(1),
                uint64(11),
        },
}

func TestHello(t *testing.T) {
        for _, p := range tests {
                t.Log(p)
        }
}
$ gno test -verbose .
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
        panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x38 pc=0x9f066e]

goroutine 1 [running]:
github.com/gnolang/gno/gnovm/pkg/gnolang.predefineNow.func1()
        /home/howl/oc/gno/gnovm/pkg/gnolang/preprocess.go:2851 +0x19c
panic({0xc29400, 0x142d9d0})
        /usr/lib/go/src/runtime/panic.go:884 +0x213
github.com/gnolang/gno/gnovm/pkg/gnolang.(*StaticBlock).Define2(0xc0002b3ba0, 0x0, {0xc003043b1b, 0x5}, {0x0?, 0x0}, {{0x0, 0x0}, {0x0, 0x0}, ...})
        /home/howl/oc/gno/gnovm/pkg/gnolang/nodes.go:1773 +0x64e
github.com/gnolang/gno/gnovm/pkg/gnolang.(*StaticBlock).Predefine(...)
        /home/howl/oc/gno/gnovm/pkg/gnolang/nodes.go:1736
github.com/gnolang/gno/gnovm/pkg/gnolang.tryPredefine({0xf651b0, 0xc000188240}, {0xf62cd8, 0xc009d9a2c0}, {0xf5dda0, 0xc009585030?})
        /home/howl/oc/gno/gnovm/pkg/gnolang/preprocess.go:3006 +0x17ef
github.com/gnolang/gno/gnovm/pkg/gnolang.predefineNow2({0xf651b0, 0xc000188240}, {0xf62cd8?, 0xc009d9a2c0}, {0xf5dda0, 0xc009585030?}, 0x44ebb2?)
        /home/howl/oc/gno/gnovm/pkg/gnolang/preprocess.go:2870 +0x125
github.com/gnolang/gno/gnovm/pkg/gnolang.predefineNow({0xf651b0, 0xc000188240}, {0xf62cd8, 0xc009d9a2c0}, {0xf5dda0, 0xc009585030})
        /home/howl/oc/gno/gnovm/pkg/gnolang/preprocess.go:2859 +0x149
github.com/gnolang/gno/gnovm/pkg/gnolang.PredefineFileSet({0xf651b0, 0xc000188240}, 0xc0002b3b80, 0xc00050ca98)
        /home/howl/oc/gno/gnovm/pkg/gnolang/preprocess.go:90 +0x2d3
github.com/gnolang/gno/gnovm/pkg/gnolang.(*Machine).runFiles(0xc0000c4fc0, {0xc0037c02a8, 0x1, 0x1})
        /home/howl/oc/gno/gnovm/pkg/gnolang/machine.go:509 +0x291
github.com/gnolang/gno/gnovm/pkg/gnolang.(*Machine).RunFiles(...)
        /home/howl/oc/gno/gnovm/pkg/gnolang/machine.go:475

This was discovered while trying to run gno test . on a converted version of https://github.com/notnil/chess . (as a fun fact, yaegi doesn't correctly handle that package's tests either: traefik/yaegi#1585 -- looks like we found ourselves a challenge-package!)

I did some debugging by adding this print statement in gnovm/pkg/gnolang/nodes.go:

func (sb *StaticBlock) Define2(isConst bool, n Name, st Type, tv TypedValue) {
	fmt.Printf(
		"StaticBlock.Define2(%v, %s, %v, %v)\n",
		isConst, n, st, tv)
// ...

The relevant output is in the last lines

StaticBlock.Define2(false, err, <nil>, (undefined))
StaticBlock.Define2(false, err, interface{}, (undefined))
StaticBlock.Define2(false, testing, package{}, (package(testing testing) package{}))
StaticBlock.Define2(false, pair, type{}, (typeval{gno/test.pair (0xc00992c9c0)} type{}))
StaticBlock.Define2(false, TestHello, func()(), (TestHello func()()))
StaticBlock.Define2(false, tests, <nil>, (undefined)) // <=============
StaticBlock.Define2(false, tests, []gno/test.pair, (nil []gno/test.pair))
StaticBlock.Define2(false, t, *testing.T, (nil *testing.T))
StaticBlock.Define2(false, _, int, (0 int))
StaticBlock.Define2(false, p, gno/test.pair, (nil gno/test.pair))
StaticBlock.Define2(false, testing, package{}, (package(testing testing) package{}))
StaticBlock.Define2(false, runtest, func()(), (runtest func()()))
StaticBlock.Define2(false, tests, <nil>, (undefined)) // <=============

I'm not 100% sure on the functioning of Define2, but a rough guess seems to be that it is seen as a re-definition and thus panics.

I suspect this issue is not limited to the tests variable, but this needs more investigation.

(The specific issue disappears changing the name of var tests to something else, like xxtests.)

(Part of a series of issues encountered while developing gnochess)

@thehowl thehowl added 🐞 bug Something isn't working 📦 🤖 gnovm Issues or PRs gnovm related labels Aug 30, 2023
@moul moul moved this to 🏆 Needed for Launch in 🚀 The Launch [DEPRECATED] Sep 5, 2023
@moul moul added this to the 🚀 main.gno.land milestone Sep 6, 2023
thehowl added a commit to thehowl/gno that referenced this issue Nov 19, 2024
r3v4s pushed a commit to gnoswap-labs/gno that referenced this issue Dec 10, 2024
This PR tackles a large amount of improvements in our current internal
testing systems for the gnovm, as well as those for `gno test`. The
primary target is the execution of filetests; however many improvements
have been implemented to remove dead or duplicate code, and bring over
some of the improvements that have been implemented in tests to
filetests, when they come at little added "cost".

The biggest headline concerns the execution of filetests. I wrote the
specific improvements undertaken in a [blog post on "diary of a
gnome"](https://gno.howl.moe/faster-tests/), but here's a side-by-side
comparison of the execution in this PR (left) and the execution on
master (right).


![filetests](https://github.com/user-attachments/assets/049680f2-baeb-4f24-8f0f-60ae5fa4bce5)

- Fixes gnolang#1084
- Fixes gnolang#2826 (by addressing root cause of slowness)
- Fixes gnolang#588, only running `_long` filetests on master and generally
speeding up test execution.

## Impact

- Test context (tests and filetests)
- Tests and filetests now share the same `test.Store` when running.
Coupled with `test.LoadImports`, it allows us to "eagerly load" imports
ahead of the test execution, and save it in the store. This is the
primary performance improvement of this PR, as all pure packages can be
run and preprocessed only once, whereas before it was once per package,
and once per filetest. (More info in the blog post.)
- One of the consequences of this is that package initialization happens
outside of the test; so a filetest can no longer check the output of a
`println` used in package initialization.
- The default user no longer has 200 gnot by default. There are two
mechanisms that make this unnecessary: `// SEND:`, and
`std.TestIssueCoin`. Some test balances had to be updated.
- Filetests
	- Running filetests in `gno test -v` now also prints their output.
- Realm tests are no longer executed in `main.gno`, but in a filename
following the name of the filetest; this changed some `// Realm:`
directives.
- The sytem to read directives and update them in the filetests has been
re-written, and should now be more resilient and with fewer
"exceptions". This means that leading and trailing whitespace in output
now is correctly considered as output, leading to the addition of many
empty `//` in the tests.
- `// Error:` directives are now treated the same as everything else;
and are updated if the `-update-golden-tests` flag is passed.
- Removed the `imports` metric from the runtime metrics, as it's now no
longer straightforward to calculate (given that imports are loaded
"eagerly").
- Removed support for different "modes" of importing stdlibs; further
removing support for gonative (gnolang#1361). The remaining gonative libraries
are `os`, `fmt`, `encoding/json`, `internal/os_test` and `math/big`.
This removes the `-with-native-fallback` flag from `gno test`.
- Consequently, filetests ending with `_native.gno` have largely been
removed, and those ending with `_stdlibs.go` have had their suffix
removed.
- Some files testing `gonative` types and functions which were only used
in a small subset of these tests, have been removed.
- Tests
- `gno test`, for testing packages, created a `main_test.gno` file that
is then called directly from the machine on each test. This creates two
identifiers, `tests` and `runtest`, which could come into conflict with
those defined by the tests themselves. We now call `testing.RunTest`
directly, without an intermediary.
- Exports in the normal tests (ie. defined in `pkg`) are now visible in
the tests of `pkg_test`. This is most useful for some standard library
tests, where there often is an `export_test.go` with the sole scope of
exporting some internal functions and methods for testing in the "real"
testing file.
- `gno lint`, and other occasions where we use `issueFromError` (like
when parsing errors in `gno test`), will now also print the column of
the error if given.

## Summary of internal changes

- pkg/gnolang
	- `TestFiles` is the new function running the filetests.
- `eval_test.go` has been removed, moving some of its improvements over
to `TestFiles`, and reducing the scope of the corresponding tests in
`debugger.go`, to what it's actually supposed to test for.
- As a consequence of removing all of type mappings in `imports.go`, I
removed `SetStrictGo2GnoMapping` in favour of always being "strict", and
not allowing defining native types of defined types any more.
- The tests in `gonative_test` where primarily related to this, so I
removed them. Similarly for `preprocess_test`.
- `TestFunc` / `TestMemPackage` have been removed as redundant,
including some of their features in `cmd/go/test.go` (like supporting
exporting symbols in `_test.gno` files)
- The `Store` no longer has `ClearCache` and `SetStrictGo2GnoMapping`;
`ClearCache` now has a better substitute in `BeginTransaction`.
- the `testing` stdlib no longer caches `matchPat` and `matchString` as
pure packages should not be able to modify their values during
execution, and as a result of other changes this was failing.
- The tests/ directory has been removed / moved to `gnovm/pkg/test`.
This directory should eventually contain the internal code for `gno
test` as well; but for now I wanted to clean `tests` to ensure it is a
directory just for integration tests, rather than code and testing
systems.
- `TestMachine` and `TestStore` have been renamed to Machine and Store,
to avoid redundancy with the `test` package name.
- Removed plenty instructions in `gnovm/Makefile` as now most tests are
just in `pkg/gnolang`.
- I removed `MsgContext.Msg` as unused. It can be re-added later if
necessary.
- In the CI, tests are now run with `-short`, at least until we figure
out how to make the VM efficient enough to run these tests. Aside from
some filetests, this now excludes some stdlibs: `bytes`, `strconv`,
`regexp/syntax`. I accept other proposals that could make us have fast
tests on PR's, while still testing (mostly) everything.

---
[![Open Source
Saturday](https://img.shields.io/badge/%E2%9D%A4%EF%B8%8F-open%20source%20saturday-F64060.svg)](https://lu.ma/open-source-saturday-torino)

---------

Co-authored-by: Marc Vertes <marc.vertes@tendermint.com>
albttx pushed a commit that referenced this issue Jan 10, 2025
This PR tackles a large amount of improvements in our current internal
testing systems for the gnovm, as well as those for `gno test`. The
primary target is the execution of filetests; however many improvements
have been implemented to remove dead or duplicate code, and bring over
some of the improvements that have been implemented in tests to
filetests, when they come at little added "cost".

The biggest headline concerns the execution of filetests. I wrote the
specific improvements undertaken in a [blog post on "diary of a
gnome"](https://gno.howl.moe/faster-tests/), but here's a side-by-side
comparison of the execution in this PR (left) and the execution on
master (right).


![filetests](https://github.com/user-attachments/assets/049680f2-baeb-4f24-8f0f-60ae5fa4bce5)

- Fixes #1084
- Fixes #2826 (by addressing root cause of slowness)
- Fixes #588, only running `_long` filetests on master and generally
speeding up test execution.

## Impact

- Test context (tests and filetests)
- Tests and filetests now share the same `test.Store` when running.
Coupled with `test.LoadImports`, it allows us to "eagerly load" imports
ahead of the test execution, and save it in the store. This is the
primary performance improvement of this PR, as all pure packages can be
run and preprocessed only once, whereas before it was once per package,
and once per filetest. (More info in the blog post.)
- One of the consequences of this is that package initialization happens
outside of the test; so a filetest can no longer check the output of a
`println` used in package initialization.
- The default user no longer has 200 gnot by default. There are two
mechanisms that make this unnecessary: `// SEND:`, and
`std.TestIssueCoin`. Some test balances had to be updated.
- Filetests
	- Running filetests in `gno test -v` now also prints their output.
- Realm tests are no longer executed in `main.gno`, but in a filename
following the name of the filetest; this changed some `// Realm:`
directives.
- The sytem to read directives and update them in the filetests has been
re-written, and should now be more resilient and with fewer
"exceptions". This means that leading and trailing whitespace in output
now is correctly considered as output, leading to the addition of many
empty `//` in the tests.
- `// Error:` directives are now treated the same as everything else;
and are updated if the `-update-golden-tests` flag is passed.
- Removed the `imports` metric from the runtime metrics, as it's now no
longer straightforward to calculate (given that imports are loaded
"eagerly").
- Removed support for different "modes" of importing stdlibs; further
removing support for gonative (#1361). The remaining gonative libraries
are `os`, `fmt`, `encoding/json`, `internal/os_test` and `math/big`.
This removes the `-with-native-fallback` flag from `gno test`.
- Consequently, filetests ending with `_native.gno` have largely been
removed, and those ending with `_stdlibs.go` have had their suffix
removed.
- Some files testing `gonative` types and functions which were only used
in a small subset of these tests, have been removed.
- Tests
- `gno test`, for testing packages, created a `main_test.gno` file that
is then called directly from the machine on each test. This creates two
identifiers, `tests` and `runtest`, which could come into conflict with
those defined by the tests themselves. We now call `testing.RunTest`
directly, without an intermediary.
- Exports in the normal tests (ie. defined in `pkg`) are now visible in
the tests of `pkg_test`. This is most useful for some standard library
tests, where there often is an `export_test.go` with the sole scope of
exporting some internal functions and methods for testing in the "real"
testing file.
- `gno lint`, and other occasions where we use `issueFromError` (like
when parsing errors in `gno test`), will now also print the column of
the error if given.

## Summary of internal changes

- pkg/gnolang
	- `TestFiles` is the new function running the filetests.
- `eval_test.go` has been removed, moving some of its improvements over
to `TestFiles`, and reducing the scope of the corresponding tests in
`debugger.go`, to what it's actually supposed to test for.
- As a consequence of removing all of type mappings in `imports.go`, I
removed `SetStrictGo2GnoMapping` in favour of always being "strict", and
not allowing defining native types of defined types any more.
- The tests in `gonative_test` where primarily related to this, so I
removed them. Similarly for `preprocess_test`.
- `TestFunc` / `TestMemPackage` have been removed as redundant,
including some of their features in `cmd/go/test.go` (like supporting
exporting symbols in `_test.gno` files)
- The `Store` no longer has `ClearCache` and `SetStrictGo2GnoMapping`;
`ClearCache` now has a better substitute in `BeginTransaction`.
- the `testing` stdlib no longer caches `matchPat` and `matchString` as
pure packages should not be able to modify their values during
execution, and as a result of other changes this was failing.
- The tests/ directory has been removed / moved to `gnovm/pkg/test`.
This directory should eventually contain the internal code for `gno
test` as well; but for now I wanted to clean `tests` to ensure it is a
directory just for integration tests, rather than code and testing
systems.
- `TestMachine` and `TestStore` have been renamed to Machine and Store,
to avoid redundancy with the `test` package name.
- Removed plenty instructions in `gnovm/Makefile` as now most tests are
just in `pkg/gnolang`.
- I removed `MsgContext.Msg` as unused. It can be re-added later if
necessary.
- In the CI, tests are now run with `-short`, at least until we figure
out how to make the VM efficient enough to run these tests. Aside from
some filetests, this now excludes some stdlibs: `bytes`, `strconv`,
`regexp/syntax`. I accept other proposals that could make us have fast
tests on PR's, while still testing (mostly) everything.

---
[![Open Source
Saturday](https://img.shields.io/badge/%E2%9D%A4%EF%B8%8F-open%20source%20saturday-F64060.svg)](https://lu.ma/open-source-saturday-torino)

---------

Co-authored-by: Marc Vertes <marc.vertes@tendermint.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐞 bug Something isn't working 📦 🤖 gnovm Issues or PRs gnovm related
Projects
Development

Successfully merging a pull request may close this issue.

4 participants