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

[GnoVM] Add Event support when testing or running expressions #2007

Open
leohhhn opened this issue Apr 30, 2024 · 7 comments
Open

[GnoVM] Add Event support when testing or running expressions #2007

leohhhn opened this issue Apr 30, 2024 · 7 comments
Assignees
Labels
help wanted Want to contribute? We recommend these issues.

Comments

@leohhhn
Copy link
Contributor

leohhhn commented Apr 30, 2024

Description

With #1653 merged, we should now start working on expanding the feature set. It would be great if gno test & gno run would display events emitted when executing functions that have them, for example with a -events flag in order to not clog up the CLI.

EDIT: another consideration is to be able to catch events within Gno test files and assert their names & arguments.

@dongwon8247
Copy link
Member

Can @notJoon @r3v4s take this effort? we just had a short discussion on this, it seems to be more complex than #1653 but we want to give it a shot. cc @moul @zivkovicmilos @leohhhn

@notJoon
Copy link
Member

notJoon commented May 3, 2024

Can @notJoon @r3v4s take this effort?

I have no idea how long the implementation will take, what I need to touch, or what I don't know. However, I will give it a try. If anyone have any ideas on how to approach this, please let me know.

@r3v4s
Copy link
Contributor

r3v4s commented May 3, 2024

before doing this, we need to address #2028

@r3v4s
Copy link
Contributor

r3v4s commented May 3, 2024

How does it look? ( single line of printing evt does look good at least for test)

$ go run ./gnovm/cmd/gno test -root-dir ~/gno -v=true ./examples/gno.land/r/demo/event
=== RUN   TestFuncs
{"pkg_path":"gno.land/r/demo/event2","type":"t","func":"inner","attrs":[{"key":"k","value":"v"}]}
{"pkg_path":"gno.land/r/demo/event2","type":"t","func":"Hello","attrs":[{"key":"k","value":"v"}]}
{"pkg_path":"gno.land/r/demo/event","type":"t","func":"inner","attrs":[{"key":"k","value":"v"}]}
{"pkg_path":"gno.land/r/demo/event","type":"t","func":"Hello","attrs":[{"key":"k","value":"v"}]}
{"pkg_path":"gno.land/r/demo/event","type":"other","func":"Other","attrs":[{"key":"k","value":"v"}]}
--- PASS: TestFuncs (0.00s)
ok      ./examples/gno.land/r/demo/event 	0.76s

@leohhhn
Copy link
Contributor Author

leohhhn commented May 3, 2024

Hey, thanks for this - I'm just wondering if events would clutter up the -verbose output, which is why it might be better to either:

  1. Put it under -print-runtime-metrics - which might not be the best place for it (WDYT?)
  2. Make it even more complicated and add another flag, such as -events or -display-events

I think option 1 could be good, but I'd like to hear more opinions.

@r3v4s
Copy link
Contributor

r3v4s commented May 3, 2024

Hey, thanks for this - I'm just wondering if events would clutter up the -verbose output, which is why it might be better to either:

  1. Put it under -print-runtime-metrics - which might not be the best place for it (WDYT?)
  2. Make it even more complicated and add another flag, such as -events or -display-events

I think option 1 could be good, but I'd like to hear more opinions.

Since event is not the type of metrics, I think option 1 might lead developer snowed under unwatned(non metric type) datas.

And for option 2, yes I do agree its more complicated than option 1 but gnoland has similar flags for events. So to make gno's cli close to each other I'd rather go with option 2.

any other ideas?? @notJoon

@notJoon
Copy link
Member

notJoon commented May 3, 2024

And for option 2, yes I do agree its more complicated than option 1 but gnoland has similar flags for events. So to make gno's cli close to each other I'd rather go with option 2.

Yes I agree with this. Because it describes the printing runtime metrics outputs like gas or memory, I think there seems to be room for cofusion when events are involved in that flag.

gno/gnovm/cmd/gno/test.go

Lines 155 to 160 in 0fc011a

fs.BoolVar(
&c.printRuntimeMetrics,
"print-runtime-metrics",
false,
"print runtime metrics (gas, memory, cpu cycles)",
)

Also, there may be cases where someone only want to output events (e.g. testing purpose), so it seems be better to have assign separate event flag to follow for flexible selection. In this case, I prefer the -events flag BTW.

Of course, I also agree this change might get a bit more complicated, but there is more to gain than lose.

@zivkovicmilos zivkovicmilos added ⭐️ could have ⭐️ help wanted Want to contribute? We recommend these issues. labels Sep 11, 2024
@Kouteki Kouteki added this to the ⏭️Next after mainnet milestone Oct 16, 2024
moul added a commit that referenced this issue Oct 23, 2024
…ests (#2975)

- [x] add `gno test -print-events` flag for unit tests.
	Props to @r3v4s for his work on #2071
- [x] add `// Events:` support in `_filetests.gno`.
- [x] cleanup `gno.Machine` between unit tests (\o/) .

Fixes #1982 
Closes #2071 
Addresses #2007

---------

Signed-off-by: moul <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Want to contribute? We recommend these issues.
Projects
Status: In Progress
Status: In Progress
Development

No branches or pull requests

6 participants