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

feat(gnovm): add 'gno test -print-events' + cleanup machine between tests #2975

Merged
merged 8 commits into from
Oct 23, 2024

Conversation

moul
Copy link
Member

@moul moul commented Oct 17, 2024

Fixes #1982
Closes #2071
Addresses #2007

@moul moul self-assigned this Oct 17, 2024
@github-actions github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label Oct 17, 2024
Copy link

codecov bot commented Oct 17, 2024

Codecov Report

Attention: Patch coverage is 59.42029% with 28 lines in your changes missing coverage. Please review.

Project coverage is 63.40%. Comparing base (d961785) to head (3409b96).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
gnovm/tests/file.go 46.93% 23 Missing and 3 partials ⚠️
gnovm/cmd/gno/test.go 90.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2975      +/-   ##
==========================================
+ Coverage   63.39%   63.40%   +0.01%     
==========================================
  Files         565      565              
  Lines       79390    79454      +64     
==========================================
+ Hits        50326    50377      +51     
- Misses      25674    25682       +8     
- Partials     3390     3395       +5     
Flag Coverage Δ
contribs/gnodev 60.57% <ø> (ø)
contribs/gnofaucet 14.82% <ø> (ø)
gno.land 67.37% <ø> (ø)
gnovm 67.87% <59.42%> (+0.01%) ⬆️
misc/genstd 79.72% <ø> (ø)
tm2 62.47% <ø> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@moul moul force-pushed the dev/moul/test-events branch from dd4fb02 to 68ca01d Compare October 18, 2024 03:37
Signed-off-by: moul <[email protected]>
@github-actions github-actions bot added the 🧾 package/realm Tag used for new Realms or Packages. label Oct 18, 2024
@moul moul marked this pull request as ready for review October 18, 2024 15:53
@moul moul mentioned this pull request Oct 18, 2024
@leohhhn
Copy link
Contributor

leohhhn commented Oct 18, 2024

There's a PR for this, although it hasn't been merged for some time.

#2071


// No events yet
events = nil
Copy link
Member

Choose a reason for hiding this comment

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

This is no longer required, no?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about this. What if we had something like testing.Events() to get the list of events which were emitted, which then can be printed by the test?

This way, we also have a way to programmatically verify the tests, rather than having to pass -print-events.

Copy link
Contributor

@leohhhn leohhhn Oct 23, 2024

Choose a reason for hiding this comment

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

See consideration at bottom of #2007

This would definitely be something needed, it's standard in other blockchain ecosystems. Ideally, we could actually do type checking on event objects down the line, but for now I guess testing.Events() would return a string of events or similar.

Copy link
Member Author

Choose a reason for hiding this comment

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

testing.Events appears to be a valuable enhancement, particularly for unit testing. However, print-events allows for event printing in existing flows, regardless of whether they have been tested or modified. In my case, I wanted to understand what was happening in some multiphase test flows. At another time, I aimed to identify all the tests that triggered a specific std.Emit, including those from realms that import the emitter indirectly.

I believe it makes sense to keep both options. The one in this PR functions like a specialized logging engine. Additionally, events serve as an alternative stdout in some ways.

Once we have the testing.Events() helper, I'm definitely open to removing the -print-events flag for the sake of simplification. Let's discuss when it will take place.

gnovm/tests/file.go Show resolved Hide resolved
Comment on lines +5 to +8
! stdout .+
stderr 'EVENTS: \[{\"type\":\"EventA\",\"attrs\":\[\],\"pkg_path\":\"gno.land/r/.*\",\"func\":\"TestA\"}\]'
stderr 'EVENTS: \[{\"type\":\"EventB\",\"attrs\":\[{\"key\":\"keyA\",\"value\":\"valA\"}\],\"pkg_path\":\"gno.land/r/.*\",\"func\":\"TestB\"},{\"type\":\"EventC\",\"attrs\":\[{\"key\":\"keyD\",\"value\":\"valD\"}\],\"pkg_path\":\"gno.land/r/.*\",\"func\":\"TestB\"}\]'
stderr 'ok \. \d\.\d\ds'
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if maybe it would be nicer to have a more human-readable format when printing. 🤷

Copy link
Member Author

Choose a reason for hiding this comment

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

The current mode is to provide the same JSON output on the WebSocket, during the maketx call, and in unit tests. Essentially, events are to be intercepted by machines, so I believe it is beneficial to have the exact same representation, which is also copy-paste friendly.

We can reconsider.

@moul moul enabled auto-merge (squash) October 23, 2024 17:28
@moul moul disabled auto-merge October 23, 2024 17:55
@moul moul merged commit f4c4204 into gnolang:master Oct 23, 2024
119 checks passed
@moul moul deleted the dev/moul/test-events branch October 23, 2024 17:55
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 🧾 package/realm Tag used for new Realms or Packages.
Projects
Status: Done
Status: ✅ Done
Status: Done
Development

Successfully merging this pull request may close these issues.

[GnoVM] Reset machine context & realm state after each Test function
3 participants