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

Add a CompactReport reporter to raclette #19

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vcm-at-dfinity
Copy link
Contributor

No description provided.

Comment on lines +241 to +242
self.writer.with_color(term::color::BRIGHT_BLACK, |out| {
writeln!(out, "(in {:?})", dur).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure it's a good default, especially for dark backgrounds. I couldn't see the time on the screenshot you've sent me at all. I knew it's there, but couldn't really read it. Maybe use gray instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, that's fine.

});
}

let (lbl, color) = if self.stats.ok() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let (lbl, color) = if self.stats.ok() {
let (label, color) = if self.stats.ok() {

Saving 2 symbols is probably not worth it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will fix


write!(self.writer, "\nSummary: ").unwrap();
self.writer
.with_color(color, |out| write!(out, "{} ", lbl).unwrap());
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.with_color(color, |out| write!(out, "{} ", lbl).unwrap());
.with_color(color, |out| write!(out, "{} ", label).unwrap());


fn done(&mut self) {
for (name, (dur, res)) in self.results.iter() {
let (lbl, color) = match res {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let (lbl, color) = match res {
let (label, color) = match res {

};

self.writer.with_color(color, |out| {
write!(out, "{} ", lbl).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
write!(out, "{} ", lbl).unwrap();
write!(out, "{} ", label).unwrap();


fn report(&mut self, result: &CompletedTask) {
self.stats.update(result);
self.results.insert(
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it really that important to have test labels sorted? I assume most users will prefer interactive output to waiting until the very last test completes just in order to see the test labels sorted...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But when using --format compact --nocapture you'll see the interactive output. I really want a list of all executed tests at the end. Precisely so its easy to see what happened after a long test run.

If the users prefer, they can stick with --format libtest for a more familiar report format.

Copy link
Contributor

Choose a reason for hiding this comment

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

But then the test logs will form one huge blob and it will be hard to find where one test begins and the other test ends. Also note that libtest format explicitly enumerates all the failed test in the summary, which is what people should probably care about the most.

Status::Success => ("pass", GREEN),
Status::Failure(_) => ("fail", RED),
Status::Signaled(_) => ("sign", YELLOW),
Status::Timeout => ("tout", RED),
Copy link
Contributor

Choose a reason for hiding this comment

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

tout

I'd never have guessed what that means :)
Consider using longer labels, padding them with spaces if needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure

("FAIL", BRIGHT_RED)
};

write!(self.writer, "\nSummary: ").unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

So if the test fails, users won't have any idea why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. If the user doesn't run with --nocapture they won't know why.

@@ -125,6 +128,7 @@ fn parse_format(input: &str) -> Result<Format, String> {
"libtest" => Ok(Format::LibTest),
"json" => Ok(Format::Json),
"tap" => Ok(Format::Tap),
"compact" => Ok(Format::Compact),
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we also need to update the help string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, thanks. Will do

@@ -70,7 +70,7 @@ const NUM_IGNORED: usize = 1;

#[test]
fn raclette_sample_main() {
let completed_tasks = default_main(Config::default().format(config::Format::Json), tests());
let completed_tasks = default_main(Config::default().format(config::Format::Compact), tests());
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it just for testing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct

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.

2 participants