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

Remove panics from xml writing #1075

Merged
merged 4 commits into from
Feb 8, 2024
Merged

Conversation

johnynek
Copy link
Collaborator

@johnynek johnynek commented Feb 7, 2024

occasionally we see panics when trying to generate the junit xml. The junit code has a number of expect calls.

This pr changes the code to pass the Result along, aggregate all the xml writing failures, and list those at the very end.

It should only improve matters.

@@ -334,7 +351,7 @@ mod tests {
#[tokio::test]
async fn test_no_history() {
let (tx, rx) = async_channel::unbounded();
let mut child_rx = pin!(HydratedInfo::build_transformer(rx));
let mut child_rx = std::pin::pin!(HydratedInfo::build_transformer(rx));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

use std::pin::pin gives an unused warning... but if I remove it, then it won't compile.

cfg.description.clone(),
)),
build_event_stream::build_event::Payload::Aborted(cfg) => Some(
match build_event_stream::aborted::AbortReason::try_from(cfg.reason) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

addresses a warning

Copy link

@wrobstory wrobstory left a comment

Choose a reason for hiding this comment

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

One comment that's more curiosity than anything

}
}
Err(e) =>
println!("could not access metadata for test result {} at file {}.\nError {}",

Choose a reason for hiding this comment

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

Does anything in here bubble back up? Should it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So this error means that when you went to access the size of the file, the filesystem operation failed.

It's for an intermediate test result that is reported by the Bazel event protocol log to exist.

We could halt the program here. I doubt this will happen. The cases we have seen look more like we can't write to the disk (maybe it's full, maybe we have some permission issue). Although if it is a permission issue, maybe we can't read.

I would say leave it as is now and we'll see how these spurious errors chance.

What do you think?

Choose a reason for hiding this comment

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

yeah I think that's fine for now

@johnynek johnynek merged commit 5afd72a into main Feb 8, 2024
5 checks passed
@johnynek johnynek deleted the oscar/20240207_more_robust_xml branch February 8, 2024 17:20
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