(Feat): Upload streamlined BEP file #226
Conversation
|
😎 Merged successfully - details. |
| bep_events_file.flush()?; | ||
| bep_events_file.seek(std::io::SeekFrom::Start(0))?; |
There was a problem hiding this comment.
This seemed to be necessary in order for the tests to work on Mac.
There was a problem hiding this comment.
Yeah, I think it's necessary when you write then immediately read again within the same scope
| Result::Ok(build_event) => { | ||
| if let Some(Payload::TestResult(test_result)) = build_event.payload { | ||
| let xml_files = test_result | ||
| if let Some(Payload::TestResult(test_result)) = &build_event.payload { |
There was a problem hiding this comment.
I will also be uploading TestSummary events in a follow-up. I considered the started events too, but that can include some env vars and bazel config that we probably shouldn't collect
TylerJang27
requested review from
dfrankland,
max-trunk,
pv72895 and
tenesttang-trunk
bundle/src/bundler.rs
Outdated
|
|
||
| if let Some(bep_parser) = self.bep_parser.as_ref() { | ||
| let mut bep_events_file = tempfile::tempfile()?; | ||
| bep_parser.bep_test_events().iter().for_each(|event| { |
There was a problem hiding this comment.
This assumes that bep_parser.parse() was called somewhere upstream (and I do see that it was called in the upload flow before being passed to the BundlerUtil here). Curious if you've considered having the BazelBepParser output a single struct with all of the relevant parsed data that we can pass around rather than the parser itself?
There was a problem hiding this comment.
Agreed—might be better to have a struct which represents an already parsed state
There was a problem hiding this comment.
Good point. I'm refactoring to handle that
| use std::path::PathBuf; | ||
| use std::{ | ||
| fs::File, | ||
| io::{Seek, Write}, | ||
| }; |
There was a problem hiding this comment.
would be awesome if we could combine these std imports and stick them in their own group above these normal crate imports
There was a problem hiding this comment.
👍 Need a linter for this 🙄
bundle/src/bundler.rs
Outdated
|
|
||
| if let Some(bep_parser) = self.bep_parser.as_ref() { | ||
| let mut bep_events_file = tempfile::tempfile()?; | ||
| bep_parser.bep_test_events().iter().for_each(|event| { |
There was a problem hiding this comment.
Agreed—might be better to have a struct which represents an already parsed state
| bep_events_file.flush()?; | ||
| bep_events_file.seek(std::io::SeekFrom::Start(0))?; |
There was a problem hiding this comment.
Yeah, I think it's necessary when you write then immediately read again within the same scope
cli-tests/src/upload.rs
Outdated
| let mut bazel_bep_parser = BazelBepParser::new( | ||
| tar_extract_directory | ||
| .join("bazel_bep.json") | ||
| .to_string_lossy() | ||
| .to_string(), | ||
| ); |
There was a problem hiding this comment.
If we update BazelBepParser you won't have to do all of this Path <-> String conversion:
#[derive(Debug, Clone, Default)]
pub struct BazelBepParser {
bazel_bep_path: PathBuf,
test_results: Vec<TestResult>,
bep_test_events: Vec<BuildEvent>,
errors: Vec<String>,
}
impl BazelBepParser {
pub fn new<T: Into<PathBuf>>(bazel_bep_path: T) -> Self {
Self {
bazel_bep_path: bazel_bep_path.into(),
..Default::default()
}
}
// ... other code ...
}There was a problem hiding this comment.
Everytime I'm sitting here thinking "there has to be a better way," Dylan's got me
Follow-up to #215 (incomplete, just timestamps) and #226 (more BEP debugging). Parses bazel [TestSummary](https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/buildeventstream/proto/build_event_stream.proto#L781) events in order to override the failure status of a given junit. This allows us to properly return a zero exit code if bazel marks a test as flaky, rather than waiting for a quarantine status to properly return. This should hopefully also allow us to better inform the rules engine for test runner-identified flakiness from bazel Notes: - I have attempted to make this as generic as possible without entangling too much logic with bazel parsing - Unfortunately this logic does have to (and already does) live in the client, since we need to parse the junits in order to properly arrive at a quarantine result - I have added unit and integration tests here, and also validated this against the repros identified in [thread](https://trunk-io.slack.com/archives/C044VBASZ0D/p1734028034948189).
3 participants
Include a subset of the BEP file in the bundle tar. This strips it to just
testResultevents, which removes sensitive information that may be in other eventsThis is mainly for our debugging purposes and for evaluating the success of this feature