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(world-modules): add gas reports to ERC721Module #1966

Merged
merged 2 commits into from
Nov 30, 2023

Conversation

R-Morpheus
Copy link
Contributor

closes #1854

Copy link

changeset-bot bot commented Nov 28, 2023

⚠️ No Changeset found

Latest commit: 9f14a4b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@holic
Copy link
Member

holic commented Nov 28, 2023

Thank you! Be sure to run pnpm gas-report from the repo root to regenerate the gas report file.

@R-Morpheus
Copy link
Contributor Author

Thank you! Be sure to run pnpm gas-report from the repo root to regenerate the gas report file.

yes, of course, that's what I wanted to do, but for some reason my commands are not executed. At the same time, it is clearly indicated that the process is completed and no errors have occurred.

I executed commands both in the root directory and in the world-modules folder, and deleted them as well.a json file, but all attempts are in vain. Maybe you have any thoughts what am I doing wrong?

Снимок экрана 2023-11-28 в 12 50 14

@holic
Copy link
Member

holic commented Nov 28, 2023

ohh I see what's going on! these are all fuzz tests, so logs are suppressed (because each one runs hundreds if not thousands of times) and therefore gas report logs are not showing up

I assume we'll need to add a separate test suite specifically for these gas reports that runs each test once

@R-Morpheus
Copy link
Contributor Author

ohh I see what's going on! these are all fuzz tests, so logs are suppressed (because each one runs hundreds if not thousands of times) and therefore gas report logs are not showing up

I assume we'll need to add a separate test suite specifically for these gas reports that runs each test once

Oh, I get it, is there anything I can help you with?

@holic
Copy link
Member

holic commented Nov 28, 2023

Oh, I get it, is there anything I can help you with?

Yeah! If you leave the gas tests where they are and a set of separate test methods that call out to the fuzzed ones with hardcoded arguments, that should give you logs/gas reports for each.

For example:

function testMintGas() public {
  testMint(1, vm.addr("minter"));
}

Copy link
Member

@holic holic left a comment

Choose a reason for hiding this comment

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

Thank you!

@holic holic merged commit 72df3d3 into latticexyz:main Nov 30, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add gas reports to ERC721Module
2 participants