feat(gnovm): `gno test` now has gas used information #2571

thinhnx-var · 2024-07-10T16:50:27Z

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.

Why do we have this PR?

After discussion in #2467 and #2149 that the gno test now has gas used information instead of execution time.

Target:

gno test has total gas used information
gno test has gas used for each xxx_filetest.gno

I will add realm storage diff size #2468 in another PR.

codecov · 2024-07-10T16:55:41Z

Codecov Report

Attention: Patch coverage is 84.00000% with 4 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
gnovm/tests/file.go	72.72%	3 Missing ⚠️
gnovm/cmd/gno/test.go	92.85%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

linhpn99 · 2024-07-10T16:58:58Z

If the gas used for each test case is displayed, that would be great :)

linhpn99 · 2024-07-10T17:00:26Z

If the gas used for each test case is displayed, that would be great :)

And it would be even better if we displayed both the execution time and the gas used :)

deelawn

Nice idea. Some style comments and also -- are also those lines supposed to commented out?

gnovm/cmd/gno/test.go

gnovm/tests/file_test.go

gnovm/tests/file.go

deelawn · 2024-07-23T01:04:26Z

gnovm/cmd/gno/test.go

@@ -205,7 +206,7 @@ func execTest(cfg *testCfg, args []string, io commands.IO) error {
 			io.ErrPrintfln("FAIL")
 			testErrCount++
 		} else {
-			io.ErrPrintfln("ok      %s \t%s", pkg.Dir, dstr)
+			io.ErrPrintfln("ok      %s \ttotal gas used: %d", pkg.Dir, gasUsed)


Why can't we have both gas used and duration? Both seem useful.

I see that people are not often concern about that time so much.

I'll ask in another way -- how do we benefit from removing it? If someone doesn't care about it then they don't have to use it. If someone does care about it then they have it. It's only a few more characters being printed.

@deelawn
I am fine with keeping the duration to be displayed.
But I have another view, should we move duration to -print-runtime-metrics which currently displaying runtime: cycle=%s imports=%d allocs=%s?

Before these changes, only subpackage level test execution displayed duration. The runtime metrics are something different. I'm only suggesting that we don't remove existing functionality if there is no good reason to do so. I don't think duration needs to be added anywhere else it doesn't currently exist.

You are right 👍 . I re-added the executing duration back again.
The result now looks like this:

gnovm/cmd/gno/test.go

gnovm/stdlibs/testing/testing.gno

thinhnx-var · 2024-07-23T22:37:58Z

Thanks for reviewing with so much details.
About displaying gasUsed instead of duration, I think people seems to not concern about the duration so much. I am not sure about this :)
Do you have any idea about removing or keeping this duration ?
cc to @moul @deelawn

moul · 2024-12-28T21:00:58Z

This PR could be useful as we are currently dogfooding more and adding new data structures, such as btree, ulist, and AVL alternatives.

@thehowl @ltzmaxwell, could you review this PR and share your thoughts on whether we should continue with it or consider a different implementation? After that, we can check if the original author is willing to update it or if we should find someone else to continue the work. Thank you.

Gno2D2 · 2024-12-28T21:02:49Z

🛠 PR Checks Summary

🔴 Maintainers must be able to edit this pull request (more info)

Manual Checks (for Reviewers):

IGNORE the bot requirements for this PR (force green CI check)
The pull request description provides enough details

✅ Automated Checks (for Contributors):

🔴 Maintainers must be able to edit this pull request (more info)

☑️ Contributor Actions:

Fix any issues flagged by automated checks.
Follow the Contributor Checklist to ensure your PR is ready for review.
- Add new tests, or document why they are unnecessary.
- Provide clear examples/screenshots, if necessary.
- Update documentation, if required.
- Ensure no breaking changes, or include BREAKING CHANGE notes.
- Link related issues/PRs, where applicable.

☑️ Reviewer Actions:

Complete manual checks for the PR, including the guidelines and additional checks if applicable.

📚 Resources:

Debug

Automated Checks
Maintainers must be able to edit this pull request (more info)
If
🟢 Condition met
└── 🟢 The pull request was created from a fork (head branch repo: VAR-META-Tech/gno)
Then
🔴 Requirement not satisfied
└── 🔴 Maintainer can modify this pull request
Manual Checks
**IGNORE** the bot requirements for this PR (force green CI check)
If
🟢 Condition met
└── 🟢 On every pull request
Can be checked by

Any user with comment edit permission
The pull request description provides enough details
If
🟢 Condition met
└── 🟢 Not (🔴 Pull request author is a member of the team: core-contributors)
Can be checked by

team core-contributors

thinhnx-var added 2 commits July 10, 2024 23:28

feat:gno gno test now has gasused instead of time

3daba99

update

e886543

thinhnx-var requested review from moul, thehowl, jaekwon, deelawn and mvertes as code owners July 10, 2024 16:50

github-actions bot assigned thinhnx-var Jul 10, 2024

github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label Jul 10, 2024

thinhnx-var added 2 commits July 10, 2024 23:52

update

01c8eef

remove typing err

b65b347

Merge branch 'master' into dev-thinhnx/feat_gno_test_has_gasused

85572a0

thinhnx-var mentioned this pull request Jul 11, 2024

feat(Gnovm/std): GasUsed function for std #2149

Closed

7 tasks

thinhnx-var and others added 3 commits July 13, 2024 15:47

Merge branch 'master' into dev-thinhnx/feat_gno_test_has_gasused

8e2888d

Merge branch 'gnolang:master' into dev-thinhnx/feat_gno_test_has_gasused

370d3ee

gofumpted

6c525fc

deelawn suggested changes Jul 23, 2024

View reviewed changes

refact coding style

ca995ff

michelleellen mentioned this pull request Jul 31, 2024

Minutes: Public Dev Calls [every Wednesday] gnolang/meetings#37

Open

thinhnx-var and others added 3 commits August 10, 2024 16:18

Merge branch 'master' into dev-thinhnx/feat_gno_test_has_gasused

21cde3f

fix linter

73bf52d

fix current testcase expectation with gasUsed information

0cc91ce

thinhnx-var force-pushed the dev-thinhnx/feat_gno_test_has_gasused branch from 5a85bec to 0cc91ce Compare August 11, 2024 03:46

thinhnx-var and others added 2 commits August 13, 2024 09:17

re-adding duration of test executing duration

6e31307

Merge branch 'master' into dev-thinhnx/feat_gno_test_has_gasused

e90b0d8

Kouteki added the review/triage-pending PRs opened by external contributors that are waiting for the 1st review label Oct 3, 2024

Kouteki removed the review/triage-pending PRs opened by external contributors that are waiting for the 1st review label Oct 4, 2024

moul requested a review from ltzmaxwell December 28, 2024 20:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(gnovm): `gno test` now has gas used information #2571

feat(gnovm): `gno test` now has gas used information #2571

thinhnx-var commented Jul 10, 2024 •

edited

Loading

codecov bot commented Jul 10, 2024 •

edited

Loading

linhpn99 commented Jul 10, 2024

linhpn99 commented Jul 10, 2024

deelawn left a comment

deelawn Jul 23, 2024

thinhnx-var Jul 23, 2024

deelawn Jul 23, 2024 •

edited

Loading

thinhnx-var Jul 29, 2024

deelawn Aug 12, 2024

thinhnx-var Aug 13, 2024

thinhnx-var commented Jul 23, 2024

moul commented Dec 28, 2024

Gno2D2 commented Dec 28, 2024

✅ Automated Checks (for Contributors):

☑️ Contributor Actions:

☑️ Reviewer Actions:

📚 Resources:

If

Then

If

Can be checked by

If

Can be checked by

feat(gnovm): gno test now has gas used information #2571

Are you sure you want to change the base?

feat(gnovm): gno test now has gas used information #2571

Conversation

thinhnx-var commented Jul 10, 2024 • edited Loading

Why do we have this PR?

Target:

codecov bot commented Jul 10, 2024 • edited Loading

Codecov Report

linhpn99 commented Jul 10, 2024

linhpn99 commented Jul 10, 2024

deelawn left a comment

Choose a reason for hiding this comment

deelawn Jul 23, 2024

Choose a reason for hiding this comment

thinhnx-var Jul 23, 2024

Choose a reason for hiding this comment

deelawn Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

thinhnx-var Jul 29, 2024

Choose a reason for hiding this comment

deelawn Aug 12, 2024

Choose a reason for hiding this comment

thinhnx-var Aug 13, 2024

Choose a reason for hiding this comment

thinhnx-var commented Jul 23, 2024

moul commented Dec 28, 2024

Gno2D2 commented Dec 28, 2024

🛠 PR Checks Summary

Manual Checks (for Reviewers):

✅ Automated Checks (for Contributors):

☑️ Contributor Actions:

☑️ Reviewer Actions:

📚 Resources:

If

Then

If

Can be checked by

If

Can be checked by

feat(gnovm): `gno test` now has gas used information #2571

feat(gnovm): `gno test` now has gas used information #2571

thinhnx-var commented Jul 10, 2024 •

edited

Loading

codecov bot commented Jul 10, 2024 •

edited

Loading

deelawn Jul 23, 2024 •

edited

Loading