♻️ Add `--summary` flag to print test summary table #5961

PraneshASP · 2023-09-30T20:00:35Z

Motivation

For projects with large number of testcases, the current output format is not friendly enough to quickly glance through the test suites. Scrolling through the output in the terminal is not quite user-friendly especially if the output is huge. It would be nice to have the test command to accept a flag and print the tests summary in a tabular format.

Solution

This pull request introduces a --summary flag for the test command. When this flag is passed, the test runner will output a summary of test results in a neatly formatted table. This tabular format will include columns for the test suite name, number of tests passed, failed, and skipped.

Example:

forge test --summary

Sample Output (Seaport codebase):

This can be further developed into something as sophisticated as hardhat-gas-report table in the future if required.

Evalir

hey! thanks for the contribution. Supportive—albeit I'd like some discussion around this first cc @mattsse @mds1 i think we had talked about this exact thing a week earlier

some nits in the meanwhile!

crates/forge/bin/cmd/test/mod.rs

Evalir · 2023-10-02T02:23:48Z

crates/forge/bin/cmd/test/mod.rs

            format_aggregated_summary(num_test_suites, total_passed, total_failed, total_skipped)
        );
+
+        if summary {


wondering, can we move all the work for building and displaying the table to this if as it's only necessary if it's actually going to be displayed?

This would require storing all the test suite name and results to a separate object inside the loop and then later read inside the if block? I wanted to avoid adding new variable/storage.

We have to do the same to display the contract names in sorted order as well I guess. What do you guys think? Do you suggest storing the test results? @mds1 @Evalir @mattsse

Yep, I think in the end if we have to sort the contracts we have to do it—so feel free to go ahead!

mattsse

I think this is a reasonable addition, and because this is opt-in, I don't see why we shouldn't include it.

pending nits

also I think we should sort the contracts by name

mds1 · 2023-10-02T16:12:48Z

crates/forge/bin/cmd/test/mod.rs

+        Cell::new("Failed")
+            .set_alignment(CellAlignment::Center)
+            .add_attribute(Attribute::Bold)
+            .fg(Color::Red),


I know we already showed red, but I'd avoid showing any red unless we actually have a failure. If there is a failure, the header name can be red, and the cells where there were failures, but the rest should be the default color

Good idea. Should we do the same for all 3 columns or only for failures?

Hmm good question. What do other frameworks do? I don't want to overcomplicate it, mainly just don't like that we currently always show red—it looks bad at a glance since everyone associates red=bad

imo, I think we could go for something simple: Unless the number is 0, show the corresponding color. I htink this works well because:

If it's 0, no need to look at it.

Ideally all your passed suites should look green :)

If you have failed or skipped tests, you probably want something to grab your attention so you can double check or at least acknowledge it.

mds1 · 2023-10-02T16:14:56Z

crates/forge/bin/cmd/test/mod.rs

+fn create_test_summary_table_header(summary_table: &mut Table) -> Row {
+    summary_table.apply_modifier(UTF8_ROUND_CORNERS);
+    Row::from(vec![
+        Cell::new("Test Suites")


Consider adding a verbose version of the summary that splits this into two "File Path" and "Contract Name" columns, since users may have the same contract name in multiple files, so that helps disambiguate

you mean adding another flag like --summary-detailed? Can also include the test file path by default if it makes sense.

I'd maybe do this as a --detailed flag which requires --summary to be enabled

mds1 · 2023-10-02T16:16:50Z

crates/forge/bin/cmd/test/mod.rs


+        if summary {
+            let row = create_test_summary_table_row(
+                contract_name,


Like @mattsse said, alphabetical sorting here would be nice, it's unclear what the current ordering is

mds1 · 2023-10-02T16:17:10Z

This is nice overall, thanks @PraneshASP! Just left a few nits

Evalir

looking good! this is only pending the few nits the others have left.

Evalir · 2023-10-06T02:42:40Z

friendly ping @PraneshASP

PraneshASP · 2023-10-06T05:55:22Z

Thanks for the suggestions! Will work on the refactor over the weekend.

PraneshASP · 2023-10-07T19:35:57Z

Made all the changes as suggested.

Added --detailed param with error handling to print file path and duration of the test suite (see screenshot)
Results are alphabetically sorted now (based on Test Suite name)
Cells are colored only if the count is > 0

Screenshots for reference:

Command:

forge test --summary

Output:

Command:

forge test --summary -detailed

Output

mattsse

nice,

I moved some things around

feat: test summary table

fd60738

Evalir requested changes Oct 2, 2023

View reviewed changes

mattsse reviewed Oct 2, 2023

View reviewed changes

mds1 reviewed Oct 2, 2023

View reviewed changes

chore: update comment format

0f85877

PraneshASP requested a review from Evalir October 2, 2023 17:29

Evalir requested changes Oct 3, 2023

View reviewed changes

PraneshASP and others added 11 commits October 7, 2023 22:35

refactor: add test summary reporter struct

f3447e9

chore: remove old code

d0bcd66

refactor: add is_detailed option to struct

128d4a8

chore: add help_heading

10779ce

feat: add conditional coloring to cells

d25e719

feat: sort based on test suite name

02a8c4a

Merge branch 'master' into feat/test-summary

445682f

chore: fix fmt and clippy warns

3428bb1

pull latest changes

eea945a

chore: fmt

3952c74

chore: update column name

b6f5ee7

PraneshASP requested review from Evalir, mattsse and mds1 October 7, 2023 19:37

some touchups

f45b2ec

mattsse approved these changes Oct 8, 2023

View reviewed changes

mattsse merged commit fb51f2a into foundry-rs:master Oct 8, 2023

♻️ Add --summary flag to print test summary table #5961

♻️ Add --summary flag to print test summary table #5961

Uh oh!

Conversation

PraneshASP commented Sep 30, 2023

Motivation

Solution

Uh oh!

Evalir left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattsse left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mds1 Oct 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mds1 commented Oct 2, 2023

Uh oh!

Evalir left a comment

Choose a reason for hiding this comment

Uh oh!

Evalir commented Oct 6, 2023

Uh oh!

PraneshASP commented Oct 6, 2023

Uh oh!

PraneshASP commented Oct 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Screenshots for reference:

Command:

Output:

Command:

Output

Uh oh!

mattsse left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

♻️ Add `--summary` flag to print test summary table #5961

♻️ Add `--summary` flag to print test summary table #5961

mattsse left a comment •

edited

Loading

mds1 Oct 2, 2023 •

edited

Loading

PraneshASP commented Oct 7, 2023 •

edited

Loading