-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
⚡ Improve test summary formatting #5731
⚡ Improve test summary formatting #5731
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i like this! some comments
crates/forge/bin/cmd/test/mod.rs
Outdated
let successes = self.successes().count(); | ||
println!( | ||
"Encountered a total of {} failing tests, {} tests succeeded", | ||
Paint::red(failures.to_string()), | ||
Paint::green(successes.to_string()) | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm why remove this? don't think we should
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already display the aggregated test summary for every runs, which was added in #5266.
Ran 4 test suites: 52 tests passed, 2 failed, 0 skipped (54 total tests)
Failing tests:
....
Encountered a total of 2 failing tests, 52 tests succeeded
The output is redundant in-case of test failures, as the total failed and passed test counts are displayed once again at the end. See attached screenshots above for reference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I understand your point—but I lean on keeping this for the sake of not making a breaking change on the tool's output. Let's keep it for now as it's something ppl expect to be there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good
crates/forge/bin/cmd/test/mod.rs
Outdated
) | ||
} | ||
|
||
/// Lists all matching tests | ||
/// Lists all matching tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extra whitespace
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed: 9cfeaa2
crates/forge/bin/cmd/test/mod.rs
Outdated
|
||
let successes = self.successes().count(); | ||
println!( | ||
"Encountered a total of {} failing tests, {} tests succeeded", | ||
Paint::red(failures.to_string()), | ||
Paint::green(successes.to_string()) | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's re-add this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added here: e409e27
@Evalir Should I change the skipped tests count color to yellow, as the tool already uses it to denote foundry/crates/forge/bin/cmd/test/mod.rs Line 434 in bff4ed9
|
yeh i was debating if we should keep yellow or switch to blue here. Let's change to yellow for now @PraneshASP |
b2ff8d5
to
278e2bf
Compare
* chore: add colors to the test result * chore: remove redundant summary * chore: remove whitespace * chore: change skipped test count color to yellow * refactor: add test summary * chore: update fixtures * chore: update --------- Co-authored-by: Enrique Ortiz <hi@enriqueortiz.dev>
Motivation
Currently, the test result summary doesn't differentiate between the failed, passed and skipped tests visually. So it would be nice to have colored test summary similar to Hardhat.
Also redundant summary is printed if any of the tests fail. It is no longer required as we already print aggregated test summary by default (Issue #5195).
Solution
Used red, green and blue colors in test summary for the no.of failed, passed and skipped tests respectively.
Also removed the redundant test summary output.
Before:
After: