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

Surface native runner information in the presence of errors, and unconditionally write to reporters. #76

Merged
merged 5 commits into from
Sep 27, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions crates/abq_cli/src/report.rs
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,7 @@ fn process_results(

let completed_summary = &CompletedSummary {
native_runner_info: Some(native_runner_info),
other_error_messages: vec![],
};

for reporter in reporters {
Expand Down
2 changes: 1 addition & 1 deletion crates/abq_cli/src/reporting.rs
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,7 @@ impl Reporter for RwxV1JsonReporter {
.map(|runner| &runner.specification);

self.collector
.write_json(fd, opt_specification)
.write_json(fd, opt_specification, summary.other_error_messages.clone())
.map_err(|_| ReportingError::FailedToFormat)?;

Ok(())
Expand Down
8 changes: 6 additions & 2 deletions crates/abq_cli/src/workers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ async fn do_shutdown(
..
} = worker_pool.shutdown().await;

tracing::debug!("Workers shutdown");
tracing::debug!(?status, "Workers shutdown");

let finalized_reporters = reporting_handle.join().await;

Expand All @@ -174,7 +174,11 @@ async fn do_shutdown(
}

let native_runner_info = native_runner_info.clone();
let completed_summary = CompletedSummary { native_runner_info };
let other_error_messages = status.error_messages().cloned().unwrap_or_default();
let completed_summary = CompletedSummary {
native_runner_info,
other_error_messages,
};

let (suite_result, errors) = finalized_reporters.finish(&completed_summary);

Expand Down
176 changes: 174 additions & 2 deletions crates/abq_cli/tests/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4717,11 +4717,17 @@ fn warn_on_different_runner_command() {
args
};

let instance1 = Abq::new(format!("{name}_inst1")).args(test_args(0)).run();
let instance1 = Abq::new(format!("{name}_inst1"))
.args(test_args(0))
.always_capture_stderr(true)
.run();

assert!(instance1.exit_status.success());

let instance2 = Abq::new(format!("{name}_inst2")).args(test_args(1)).run();
let instance2 = Abq::new(format!("{name}_inst2"))
.args(test_args(1))
.always_capture_stderr(true)
.run();

assert!(instance2.exit_status.success());

Expand All @@ -4735,3 +4741,169 @@ fn warn_on_different_runner_command() {

term(queue_proc);
}

#[test]
#[with_protocol_version]
#[serial]
fn write_partial_rwx_v1_json_results_on_early_runner_termination() {
let name = "warn_on_different_runner_command";
let conf = CSConfigOptions {
use_auth_token: true,
tls: true,
};

let (_queue_proc, queue_addr) = setup_queue!(name, conf);

let proto = AbqProtocolVersion::V0_2.get_supported_witness().unwrap();

let manifest = vec![
TestOrGroup::test(Test::new(
proto,
"test1".to_string(),
[],
Default::default(),
)),
TestOrGroup::test(Test::new(
proto,
"test2".to_string(),
[],
Default::default(),
)),
];

let manifest = ManifestMessage::new(Manifest::new(manifest, Default::default()));

let simulation = [
Connect,
//
// Write spawn message
OpaqueWrite(pack(legal_spawned_message(proto))),
//
// Write the manifest if we need to.
// Otherwise handle the one test.
IfGenerateManifest {
then_do: vec![OpaqueWrite(pack(&manifest))],
else_do: vec![
//
// Read init context message + write ACK
OpaqueRead,
OpaqueWrite(pack(InitSuccessMessage::new(proto))),
// Read first test, write okay
OpaqueRead,
OpaqueWrite(pack(RawTestResultMessage::fake(proto))),
// Read second test, bail
OpaqueRead,
Exit(1),
],
},
//
// Finish
Exit(0),
];

let packed = pack_msgs_to_disk(simulation);
let rwx_v1_json = tempfile::NamedTempFile::new().unwrap();

let test_args = {
let simulator = native_runner_simulation_bin();
let simfile_path = packed.path.display().to_string();
let rwx_v1_json_path = rwx_v1_json.path().display().to_string();
let args = vec![
format!("test"),
format!("--worker=0"),
format!("--queue-addr={queue_addr}"),
format!("--run-id=test-run-id"),
format!("--reporter=rwx-v1-json={rwx_v1_json_path}"),
];
let mut args = conf.extend_args_for_client(args);
args.extend([s!("--"), simulator, simfile_path]);
args
};

let CmdOutput {
stdout,
stderr,
exit_status,
} = Abq::new(format!("{name}_worker0"))
.args(test_args)
.always_capture_stderr(true)
.run();

assert!(!exit_status.success());
assert_eq!(exit_status.code(), Some(ExitCode::ABQ_ERROR.get()));

assert!(
stdout.contains("2 tests, 1 failures"),
"STDOUT:\n{stdout}\nSTDERR:\n{stderr}"
);
assert!(
stderr.contains("ABQ had an error communicating with the native runner"),
"STDOUT:\n{stdout}\nSTDERR:\n{stderr}"
);

let rwx_v1_json = std::fs::read_to_string(rwx_v1_json.path()).unwrap();
let rwx_v1_json: serde_json::Value = serde_json::from_str(&rwx_v1_json).unwrap();
insta::assert_json_snapshot!(rwx_v1_json, {
}, @r###"
{
"$schema": "https://raw.githubusercontent.com/rwx-research/test-results-schema/main/v1.json",
"framework": {
"kind": "RSpec",
"language": "Ruby"
},
"otherErrors": [
{
"message": "ABQ had an error communicating with the native runner: early eof at crates/abq_runners/generic_test_runner/src/lib.rs@1035:87"
}
],
"summary": {
"canceled": 0,
"failed": 0,
"otherErrors": 2,
Copy link
Member

Choose a reason for hiding this comment

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

🤔 should this be 1?

Copy link
Member

Choose a reason for hiding this comment

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

or differently stated, we only have one otherError in the list of other errors and I the summary is supposed to reflect those lists (other errors and the tests)

"pended": 0,
"quarantined": 0,
"retries": 0,
"skipped": 0,
"status": {
"kind": "failed"
},
"successful": 1,
"tests": 1,
"timedOut": 0,
"todo": 0
},
"tests": [
{
"attempt": {
"durationInNanoseconds": 0,
"finishedAt": "1994-11-05T13:17:30Z",
"meta": {
"abq_metadata": {
"runner": 1,
"worker": 0
}
},
"startedAt": "1994-11-05T13:15:30Z",
"status": {
"kind": "successful"
},
"stderr": "",
"stdout": ""
},
"id": "zzz-faux",
"lineage": [
"TopLevel",
"SubModule",
"Test"
],
"location": {
"column": 15,
"file": "a/b/x.file",
"line": 10
},
"name": "zzz-faux"
}
]
}
"###);
}
1 change: 1 addition & 0 deletions crates/abq_reporting/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ pub struct CompletedSummary {
/// The native test runner that was in use for this test run.
/// Can be [None] if the returned worker never ran any tests.
pub native_runner_info: Option<NativeRunnerInfo>,
pub other_error_messages: Vec<String>,
}

#[derive(Debug, Error)]
Expand Down
36 changes: 25 additions & 11 deletions crates/abq_runners/generic_test_runner/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,8 @@ impl RunnerConnection {
}
}

#[derive(Debug)]
pub struct NativeRunnerInfo {
#[derive(Debug, Clone)]
pub struct NativeRunnerConfiguration {
protocol: ProtocolWitness,
specification: NativeRunnerSpecification,
}
Expand All @@ -103,7 +103,7 @@ pub struct NativeRunnerInfo {
pub async fn open_native_runner_connection(
listener: &mut TcpListener,
timeout: Duration,
) -> Result<(NativeRunnerInfo, RunnerConnection), GenericRunnerErrorKind> {
) -> Result<(NativeRunnerConfiguration, RunnerConnection), GenericRunnerErrorKind> {
let start = Instant::now();
let (
NativeRunnerSpawnedMessage {
Expand Down Expand Up @@ -134,7 +134,7 @@ pub async fn open_native_runner_connection(
match protocol_version.get_supported_witness() {
None => Err(ProtocolVersionError::NotCompatible.into()),
Some(witness) => {
let runner_info = NativeRunnerInfo {
let runner_info = NativeRunnerConfiguration {
protocol: witness,
specification: runner_specification,
};
Expand All @@ -148,6 +148,7 @@ macro_rules! try_setup {
$err.located(here!()).map_err(|e| GenericRunnerError {
error: e,
output: StdioOutput::empty(),
native_runner_info: None,
})?
};
}
Expand Down Expand Up @@ -177,12 +178,14 @@ async fn retrieve_manifest<'a>(
return Err(GenericRunnerError {
error,
output: final_stdio_output.into(),
native_runner_info: Some(native_runner.get_native_runner_info()),
});
}
Err(error) => {
return Err(GenericRunnerError {
error,
output: final_stdio_output.into(),
native_runner_info: Some(native_runner.get_native_runner_info()),
});
}
}
Expand Down Expand Up @@ -227,14 +230,15 @@ pub enum GenericRunnerErrorKind {
Io(#[from] io::Error),
#[error("{0}")]
ProtocolVersion(#[from] ProtocolVersionError),
#[error("{0}")]
#[error("ABQ had an error communicating with the native runner: {0}")]
NativeRunner(#[from] NativeTestRunnerError),
}

#[derive(Debug, Error)]
pub struct GenericRunnerError {
pub error: LocatedError,
pub output: StdioOutput,
pub native_runner_info: Option<queue::NativeRunnerInfo>,
}

impl std::fmt::Display for GenericRunnerError {
Expand All @@ -244,10 +248,14 @@ impl std::fmt::Display for GenericRunnerError {
}

impl GenericRunnerError {
pub fn no_captures(kind: LocatedError) -> Self {
pub fn no_captures(
kind: LocatedError,
native_runner_info: Option<queue::NativeRunnerInfo>,
) -> Self {
Self {
error: kind,
output: StdioOutput::empty(),
native_runner_info,
}
}
}
Expand Down Expand Up @@ -348,7 +356,7 @@ struct NativeRunnerState {
// Needed to keep the port alive.
_tcp_listener: TcpListener,
conn: RunnerConnection,
runner_info: NativeRunnerInfo,
runner_info: NativeRunnerConfiguration,
runner_meta: RunnerMeta,
for_manifest_generation: bool,
}
Expand Down Expand Up @@ -508,7 +516,11 @@ impl<'a> NativeRunnerHandle<'a> {
protocol_version_timeout,
)
.await;
return Err(GenericRunnerError { error, output });
return Err(GenericRunnerError {
error,
output,
native_runner_info: None,
});
}
};

Expand Down Expand Up @@ -665,10 +677,10 @@ impl<'a> NativeRunnerHandle<'a> {
Ok(())
}

fn get_native_runner_info(self) -> queue::NativeRunnerInfo {
fn get_native_runner_info(&self) -> queue::NativeRunnerInfo {
queue::NativeRunnerInfo {
protocol_version: self.state.runner_info.protocol.get_version(),
specification: self.state.runner_info.specification,
specification: self.state.runner_info.specification.clone(),
}
}
}
Expand Down Expand Up @@ -858,6 +870,7 @@ async fn run_help(
Err(err) => Err(GenericRunnerError {
error: err,
output: StdioOutput { stderr, stdout },
native_runner_info: Some(native_runner_handle.get_native_runner_info()),
}),
}
}
Expand Down Expand Up @@ -899,7 +912,7 @@ async fn execute_all_tests<'a>(
runner_meta: RunnerMeta,
test_timeout: Duration,
) -> Result<ExitCode, LocatedError> {
let NativeRunnerInfo {
let NativeRunnerConfiguration {
protocol: _,
specification: runner_spec,
} = &native_runner_handle.runner_info;
Expand Down Expand Up @@ -1531,6 +1544,7 @@ pub fn execute_wrapped_runner(
return Err(GenericRunnerError {
error: io::Error::new(io::ErrorKind::InvalidInput, error).located(here!()),
output: StdioOutput::empty(),
native_runner_info: None,
});
}

Expand Down
11 changes: 10 additions & 1 deletion crates/abq_runners/generic_test_runner/tests/simulation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -633,7 +633,16 @@ fn native_runner_fails_while_executing_tests() {
let (error, mut results, _notified_all_run) =
run_simulated_runner_to_error(simulation, None, Box::new(get_next_tests));

let GenericRunnerError { error: _, output } = error;
let GenericRunnerError {
error: _,
output,
native_runner_info,
} = error;

assert!(
native_runner_info.is_some(),
"Native runner info should be defined because some tests ran"
);

check_bytes!(&output.stdout, b"I failed catastrophically");
check_bytes!(
Expand Down
Loading
Loading