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(gnovm): "*_filetest.gno" instructions #1023

Merged
merged 9 commits into from
Aug 12, 2023

Conversation

tbruyelle
Copy link
Contributor

@tbruyelle tbruyelle commented Aug 3, 2023

Add tests for Output, Error and Realm instructions

The change introduces a new test dependency: testscripts, which allows to create tests based on txtar files. This is very handy for testing gno test, because:

  • you can easily creates a set of files that can be used for a command (here the gno test command)
  • you can assert stderr and stdout expectations in a very readable and maintainable way
  • you can compare files using the cmp command, which is very useful to assert of the -update-golden-tests behavior.

The tests can be run with the following command:

$ go test ./gnovm/tests/ -v -run TestRunFileTest

These tests were added to help work on #1015, which will potentially alter the behavior of "*_filetest.gno" instructions updates.

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
  • Added new benchmarks to generated graphs, if any. More info here.

@tbruyelle tbruyelle requested a review from a team as a code owner August 3, 2023 14:52
@github-actions github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label Aug 3, 2023
@@ -0,0 +1,17 @@
# Test Error instruction incorrect

! exec $GNO test -verbose -root-dir=$ROOTDIR .
Copy link
Contributor Author

@tbruyelle tbruyelle Aug 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The leading ! indicates the command is expected to fail (exit code != 0)

Conversely, when there's no ! the command is expected to succeed.

When the expections are wrong, the test fails ofc.

rootDir := path.Dir(string(goModPath))
// Build a fresh gno binary in a temp directory
gnoBin := path.Join(t.TempDir(), "gno")
err = exec.Command("go", "build", "-o", gnoBin, path.Join(rootDir, "gnovm", "cmd", "gno")).Run()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't want to override an existing gno binary, so we compile a new one in a temp directory that will be removed at the end of the test.

}

// Error:
// oups
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Each txtar file (txt is also an accepted extension) is executed in his own directory, so here the only file present for that test is x_filetest.gno.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks great.

What do you think about moving this to gnovm/cmd/gno/testdata, since it’s testing the CLI?

Copy link
Contributor Author

@tbruyelle tbruyelle Aug 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Ideally, I think those tests should be added to gnovm/cmd/gno/test_test.go, and so this whole file should be rewritten using testscripts. Indeed I find the current format hard to read and maintain. The only good part is it fits in a single file, whereas with typescript there would be multiple small txtar files.

If it's OK I can do that in this PR, or an other one if you prefer.

@tbruyelle tbruyelle force-pushed the tbruyelle/test/filetest-instructions branch 2 times, most recently from db84131 to 918c3de Compare August 4, 2023 14:13
gnovm/tests/file_test.go Outdated Show resolved Hide resolved
@harry-hov harry-hov self-requested a review August 7, 2023 15:31
Copy link
Contributor

@harry-hov harry-hov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 💯

Really welcoming testscript package :shipit:

gnovm/tests/file_test.go Outdated Show resolved Hide resolved
gnovm/tests/file_test.go Outdated Show resolved Hide resolved
@tbruyelle tbruyelle mentioned this pull request Aug 9, 2023
7 tasks
@zivkovicmilos zivkovicmilos self-requested a review August 10, 2023 16:35
Copy link
Member

@zivkovicmilos zivkovicmilos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is really cool

Thank you for this addition 🙏

Copy link
Member

@moul moul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you address this before: #1023 (comment), please?

Add tests for Output, Error and Realm instructions

The change introduces a new test dependency
github.com/rogpeppe/go-internal/testscripts, which allows to create
tests bases on txtar files. This is very handy for this kind of tests,
because:
- you can easily creates a set of files that can be used for a command
  (here the `gno test` command)
- you can assert stderr and stdout expectations in a very readable and
  maintainable way
- you can compare files using the `cmp` command, which is very useful to
  assert of the `-update-golden-tests` behavior.

These tests were added to help work on gnolang#1015, which will potentially
alter the behavior of "*filetest.gno" instructions updates.
- rename txt to txtar
- add comment for '!stdout .+' instruction
- use filepath instead of path
- use a custom command for "gno" instead of env var
- rebase on master so we can use "GNOROOT" instead of "-root-dir"
@tbruyelle tbruyelle force-pushed the tbruyelle/test/filetest-instructions branch from 6a2ed63 to 1b1fddf Compare August 11, 2023 09:46
@tbruyelle tbruyelle force-pushed the tbruyelle/test/filetest-instructions branch from c20f39f to 891929d Compare August 11, 2023 09:55
@tbruyelle
Copy link
Contributor Author

Can you address this before: #1023 (comment), please?

@moul the last commits address that :

  • moved gnovm/tests.RunFileTest in gnovm/cmd/gno.TestTest
  • migrate all cases in TestTest so they use testscript.
    Some of them were using files in examples, I replaced those cases by testscript file so we don't take the risk of breaking the tests when changing files in examples.
  • clean gno files in gnovm/tests/integ that aren't used any more (due to previous point)

Regarding testscript setup, I also provided a couple of improvements:

  • replace exec $GNO with a simple gno, which is configured as a testscript command/keyword that targets the gno binary compiled during setup.
  • moved the setup in a method in main_test.go, so when we'll migrate the other gno commands it will be easier.
  • replace -root-dir flag by $GNOROOT, thanks to feat: add GNOROOT environment variables #1014 and a rebase on master.

@moul moul merged commit 902b7d9 into gnolang:master Aug 12, 2023
77 checks passed
@tbruyelle tbruyelle deleted the tbruyelle/test/filetest-instructions branch August 13, 2023 07:51
Doozers pushed a commit to Doozers/gno that referenced this pull request Aug 31, 2023
@moul moul added this to the 🌟 main.gno.land (wanted) milestone Sep 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 🤖 gnovm Issues or PRs gnovm related
Projects
Archived in project
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants