Skip to content

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.

"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

#[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.

3 participants