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: make --gas-report JSON output compatible #9063

Merged
merged 11 commits into from
Oct 10, 2024

Conversation

zerosnacks
Copy link
Member

@zerosnacks zerosnacks commented Oct 8, 2024

Motivation

Closes: #8789

Part of meta-ticket #8794

Solution

Updates existing relevant tests to use Snapbox and adds additional tests for JSON output

Adds an additonal check on --gas-report when the --junit flag is specified as JUnit output is not supported

Adds a silent variable to mute unexpected output, this can be expanded in the future for other outputs that are mutually exclusive with the standard output

Each test suite ran with --json is written to an individual line to allow users to easily consume the output by parsing by line.

JSON formatted output looks like this:

{
   "gas":106715,
   "size":277,
   "functions":{
      "increment":{
         "increment()":{
            "calls":1,
            "min":43404,
            "mean":43404,
            "median":43404,
            "max":43404
         }
      },
      "number":{
         "number()":{
            "calls":257,
            "min":283,
            "mean":283,
            "median":283,
            "max":283
         }
      },
      "setNumber":{
         "setNumber(uint256)":{
            "calls":258,
            "min":23582,
            "mean":43384,
            "median":43542,
            "max":43866
         }
      }
   }
}

Compare to Markdown output:

| src/Counter.sol:Counter contract |                 |       |        |       |         |
|----------------------------------|-----------------|-------|--------|-------|---------|
| Deployment Cost                  | Deployment Size |       |        |       |         |
| 106715                           | 277             |       |        |       |         |
| Function Name                    | min             | avg   | median | max   | # calls |
| increment                        | 43404           | 43404 | 43404  | 43404 | 1       |
| number                           | 283             | 283   | 283    | 283   | 257     |
| setNumber                        | 23582           | 43224 | 43554  | 43866 | 258     |

Copy link
Collaborator

@grandizzy grandizzy left a comment

Choose a reason for hiding this comment

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

lgtm!

crates/forge/tests/cli/cmd.rs Outdated Show resolved Hide resolved
@zerosnacks zerosnacks enabled auto-merge (squash) October 9, 2024 12:30
@zerosnacks
Copy link
Member Author

ping @DaniPopes for final review

@zerosnacks zerosnacks merged commit 0ec018d into master Oct 10, 2024
21 checks passed
@zerosnacks zerosnacks deleted the zerosnacks/fix-test-gas-report-json branch October 10, 2024 17:06
@sakulstra
Copy link
Contributor

sakulstra commented Oct 14, 2024

Hey folks, I just tried running this command and imo there's multiple issues with that implementation:

  1. the emitted json data loses the contract path completely, which is unfortunate as in projects with more than one contracts this information is actually quite important. Especially if multiple contracts have the same signatures it's becoming impossible to distinguish.
  2. the emitted json is not valid json - each line for itself, yes, but the overall generated output is not(as it's missing separators and a bounding array). Imo the goal should be that piping forge test --gas-report --json > some.json, i receive a valid json i can post-process.

@zerosnacks
Copy link
Member Author

Hi @sakulstra thanks for your feedback, I've opened a new ticket here with your points as feature requests: #9111 assigned to me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Ready For Review
Development

Successfully merging this pull request may close these issues.

bug: forge test --gas-report --json yields unexpected data and misses data
4 participants