-
Notifications
You must be signed in to change notification settings - Fork 2
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
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,7 +13,6 @@ use super::build_event_server::BuildEventAction; | |
use bazelfe_protos::build_event_stream::NamedSetOfFiles; | ||
use bazelfe_protos::*; | ||
use std::path::PathBuf; | ||
use std::pin::pin; | ||
|
||
pub trait HasFiles { | ||
fn files(&self) -> Vec<build_event_stream::File>; | ||
|
@@ -324,6 +323,24 @@ impl HydratedInfo { | |
}); | ||
next_rx | ||
} | ||
|
||
pub fn label(&self) -> Option<&str> { | ||
match self { | ||
HydratedInfo::BazelAbort(ba) => | ||
// this is a dance to return the ref | ||
{ | ||
match &ba.label { | ||
Some(s) => Some(s.as_str()), | ||
None => None, | ||
} | ||
} | ||
HydratedInfo::ActionFailed(af) => Some(af.label.as_str()), | ||
HydratedInfo::Progress(_) => None, | ||
HydratedInfo::TestResult(tri) => Some(tri.test_summary_event.label.as_str()), | ||
HydratedInfo::ActionSuccess(asucc) => Some(asucc.label.as_str()), | ||
HydratedInfo::TargetComplete(tc) => Some(tc.label.as_str()), | ||
} | ||
} | ||
} | ||
|
||
#[cfg(test)] | ||
|
@@ -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)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
||
tx.send(BuildEventAction::BuildEvent(bazel_event::BazelBuildEvent { | ||
event: bazel_event::Evt::ActionCompleted(bazel_event::ActionCompletedEvt { | ||
|
@@ -363,7 +380,7 @@ mod tests { | |
#[tokio::test] | ||
async fn test_with_files() { | ||
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)); | ||
|
||
tx.send(BuildEventAction::BuildEvent(bazel_event::BazelBuildEvent { | ||
event: bazel_event::Evt::ActionCompleted(bazel_event::ActionCompletedEvt { | ||
|
@@ -413,7 +430,7 @@ mod tests { | |
#[tokio::test] | ||
async fn test_with_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)); | ||
|
||
tx.send(BuildEventAction::BuildEvent(bazel_event::BazelBuildEvent { | ||
event: bazel_event::Evt::TargetConfigured(bazel_event::TargetConfiguredEvt { | ||
|
@@ -472,7 +489,7 @@ mod tests { | |
#[tokio::test] | ||
async fn state_resets_on_new_build() { | ||
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)); | ||
|
||
tx.send(BuildEventAction::BuildEvent(bazel_event::BazelBuildEvent { | ||
event: bazel_event::Evt::TargetConfigured(bazel_event::TargetConfiguredEvt { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,7 @@ | ||
use bazelfe_bazel_wrapper::bep::build_events::build_event_server::BuildEventAction; | ||
use bazelfe_core::bep_junit::{ | ||
emit_backup_error_data, emit_junit_xml_from_aborted_action, emit_junit_xml_from_failed_action, | ||
label_to_junit_relative_path, suites_with_error_from_xml, | ||
label_to_junit_relative_path, | ||
}; | ||
|
||
use bazelfe_bazel_wrapper::bep::build_events::hydrated_stream::{HydratedInfo, HydratorState}; | ||
|
@@ -10,6 +10,7 @@ use clap::Parser; | |
use prost::Message; | ||
use std::collections::VecDeque; | ||
use std::error::Error; | ||
use std::os::unix::fs::MetadataExt; | ||
use std::path::{Path, PathBuf}; | ||
|
||
#[derive(Parser, Debug)] | ||
|
@@ -77,19 +78,26 @@ async fn main() -> Result<(), Box<dyn Error>> { | |
let mut failed_actions = Vec::default(); | ||
let mut aborted_actions = Vec::default(); | ||
let mut failed_tests = Vec::default(); | ||
let mut failed_xml_writes = Vec::default(); | ||
for build_event in hydrated_infos.into_iter() { | ||
match build_event { | ||
match &build_event { | ||
HydratedInfo::BazelAbort(abort_info) => { | ||
emit_junit_xml_from_aborted_action( | ||
aborted_actions.push(abort_info.label.clone()); | ||
match emit_junit_xml_from_aborted_action( | ||
&abort_info, | ||
aborted_actions.len(), | ||
&opt.junit_output_path, | ||
); | ||
aborted_actions.push(abort_info.label); | ||
) { | ||
Ok(_) => (), | ||
Err(e) => failed_xml_writes.push((build_event, e)), | ||
} | ||
} | ||
HydratedInfo::ActionFailed(action_failed) => { | ||
emit_junit_xml_from_failed_action(&action_failed, &opt.junit_output_path); | ||
failed_actions.push(action_failed.label); | ||
failed_actions.push(action_failed.label.clone()); | ||
match emit_junit_xml_from_failed_action(&action_failed, &opt.junit_output_path) { | ||
Ok(_) => (), | ||
Err(e) => failed_xml_writes.push((build_event, e)), | ||
} | ||
} | ||
HydratedInfo::TestResult(r) => { | ||
let is_failure = r.test_summary_event.test_status.didnt_pass(); | ||
|
@@ -99,35 +107,60 @@ async fn main() -> Result<(), Box<dyn Error>> { | |
let output_folder = opt.junit_output_path.join(label_to_junit_relative_path( | ||
r.test_summary_event.label.as_str(), | ||
)); | ||
std::fs::create_dir_all(&output_folder).expect("Make dir failed"); | ||
|
||
let files: Vec<&str> = r | ||
.test_summary_event | ||
.output_files | ||
.iter() | ||
.flat_map(|e| match e { | ||
bazelfe_protos::build_event_stream::file::File::Uri(uri) => { | ||
let p = uri | ||
.strip_prefix("file://") | ||
.unwrap_or_else(|| panic!("Wasn't a local file for {}", uri)); | ||
if p.ends_with("/test.xml") { | ||
Some(p) | ||
} else { | ||
None | ||
|
||
match std::fs::create_dir_all(&output_folder) { | ||
Ok(_) => { | ||
let files: Vec<&str> = r | ||
.test_summary_event | ||
.output_files | ||
.iter() | ||
.flat_map(|e| match e { | ||
bazelfe_protos::build_event_stream::file::File::Uri(uri) => { | ||
let p = uri.strip_prefix("file://").unwrap_or_else(|| { | ||
panic!("Wasn't a local file for {}", uri) | ||
}); | ||
if p.ends_with("/test.xml") { | ||
Some(p) | ||
} else { | ||
None | ||
} | ||
} | ||
bazelfe_protos::build_event_stream::file::File::Contents(_) => None, | ||
}) | ||
.collect(); | ||
|
||
for (idx, f) in files.into_iter().enumerate() { | ||
match std::fs::metadata(f) { | ||
Ok(m) => { | ||
if m.size() > 0 { | ||
let output_file = output_folder.join(format!("test.{}.xml", idx)); | ||
match std::fs::copy(f, output_file) { | ||
Ok(_) => (), | ||
Err(e) => | ||
println!("could not access metadata for test result {} at file {}.\nError {}", | ||
r.test_summary_event.label, | ||
f, | ||
e) | ||
} | ||
} | ||
} | ||
Err(e) => | ||
println!("could not access metadata for test result {} at file {}.\nError {}", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does anything in here bubble back up? Should it? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah I think that's fine for now |
||
r.test_summary_event.label, | ||
f, | ||
e) | ||
} | ||
bazelfe_protos::build_event_stream::file::File::Contents(_) => None, | ||
}) | ||
.collect(); | ||
|
||
if is_failure { | ||
// Some failures don't get to the phase of writing junit output | ||
// this ensures we write something | ||
emit_backup_error_data(&r, &opt.junit_output_path); | ||
} | ||
for (idx, f) in files.into_iter().enumerate() { | ||
let output_file = output_folder.join(format!("test.{}.xml", idx)); | ||
std::fs::copy(f, output_file).unwrap(); | ||
} | ||
if is_failure { | ||
// Some failures don't get to the phase of writing junit output | ||
// this ensures we write something | ||
match emit_backup_error_data(&r, &opt.junit_output_path) { | ||
Ok(_) => (), | ||
Err(err) => failed_xml_writes.push((build_event, err)), | ||
} | ||
} | ||
} | ||
Err(e) => failed_xml_writes.push((build_event, e.into())), | ||
} | ||
} | ||
HydratedInfo::Progress(_) => (), | ||
|
@@ -136,32 +169,53 @@ async fn main() -> Result<(), Box<dyn Error>> { | |
} | ||
} | ||
|
||
if failed_actions.is_empty() && failed_tests.is_empty() && aborted_actions.is_empty() { | ||
println!("Have zero failures, all successful.") | ||
if failed_actions.is_empty() | ||
&& failed_tests.is_empty() | ||
&& aborted_actions.is_empty() | ||
&& failed_xml_writes.is_empty() | ||
{ | ||
println!("Have zero failures, all successful."); | ||
Ok(()) | ||
} else { | ||
if !failed_actions.is_empty() { | ||
println!("Have {} failed actions", failed_actions.len()); | ||
for a in failed_actions.iter() { | ||
for a in failed_actions { | ||
println!(" - {}", a); | ||
} | ||
} | ||
|
||
if !failed_tests.is_empty() { | ||
println!("Have {} failed tests", failed_tests.len()); | ||
for a in failed_tests.iter() { | ||
failed_tests.sort(); | ||
for a in failed_tests { | ||
println!(" - {}", a); | ||
} | ||
} | ||
|
||
if !aborted_actions.is_empty() { | ||
println!("Have {} aborted actions", aborted_actions.len()); | ||
for a in aborted_actions.iter() { | ||
aborted_actions.sort(); | ||
for a in aborted_actions { | ||
println!(" - {}", a.unwrap_or_else(|| "Unknown".to_string())); | ||
} | ||
} | ||
|
||
if !failed_xml_writes.is_empty() { | ||
println!("Got {} xml write failures", failed_xml_writes.len()); | ||
failed_xml_writes.sort_by(|p1, p2| p1.0.label().cmp(&p2.0.label())); | ||
for (r, err) in failed_xml_writes { | ||
println!( | ||
" - {}", | ||
a.to_owned().unwrap_or_else(|| "Unknown".to_string()) | ||
"Target label = {} failed to write: {}", | ||
r.label().unwrap_or("<unknown>"), | ||
err | ||
); | ||
} | ||
} | ||
|
||
let err: Box<dyn Error> = Box::new(std::io::Error::new( | ||
std::io::ErrorKind::Other, | ||
"non-zero error count.", | ||
)); | ||
Err(err) | ||
} | ||
Ok(()) | ||
} |
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.
addresses a warning