-
Notifications
You must be signed in to change notification settings - Fork 707
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
subsystem-bench: Prepare CI output #3158
Conversation
AndreiEres
commented
Jan 31, 2024
•
edited
Loading
edited
- Benchmark results are collected in a single struct.
- The output of the results is prettified.
- The result struct used to save the output as a yaml and store it in artifacts in a CI job.
|
||
if let Some(agent_running) = agent_running { | ||
let agent_ready = agent_running.stop()?; | ||
agent_ready.shutdown(); | ||
} | ||
|
||
let output = if self.ci { serde_yaml::to_string(&vec![usage])? } else { usage.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.
Nit: This logic is duplicated you can move it in a small funciton.
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.
You're right. But we're going to remove the last branch and keep only run with test files. So this duplication is going to be removed as well.
@@ -99,6 +99,10 @@ struct BenchCli { | |||
/// Enable Cache Misses Profiling with Valgrind. Linux only, Valgrind must be in the PATH | |||
pub cache_misses: bool, | |||
|
|||
#[clap(long, default_value_t = false)] | |||
/// Shows the output in YAML format | |||
pub ci: bool, |
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.
Probably we can find a better name here like:
--output-format=yaml
--output-format=json
or even simpler
--yaml-output
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 think we can start with more obvious --yaml-output
, because I have no plans for additional json one :-)
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.
LGTM! I also suggest to include the test objective in the output.
I think we add it as a title here, or you mean something else?
|
Yes, currently you print just the seq file name. |
Co-authored-by: Andrei Sandu <[email protected]>