Skip to content

Commit

Permalink
remove the leaky ArtifactContext
Browse files Browse the repository at this point in the history 
- previously all log and error outputs needed to know where theyre being
  called from; this is an antipattern and leaks impl detail
- move this context to the proper spaces (in StartedTestRun,
  StartedTestStep)
- this commit changes semantics of the `to_artifact` a bit, but this
  will be cleared in the next commits which refactor the emitter

Signed-off-by: mimir-d <[email protected]>
  • Loading branch information
@mimir-d
mimir-d committed Oct 4, 2024
1 parent b8716a0 commit c800560
Show file tree
Hide file tree
Showing 4 changed files with 159 additions and 172 deletions.
158 changes: 65 additions & 93 deletions src/output/error.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
// https://opensource.org/licenses/MIT.

use crate::output as tv;
use tv::{dut, models, run::ArtifactContext};
use tv::{dut, models};

pub struct Error {
symptom: String,
Expand All @@ -19,28 +19,12 @@ impl Error {
ErrorBuilder::new(symptom)
}

pub fn to_artifact(&self, context: ArtifactContext) -> models::OutputArtifactDescendant {
match context {
ArtifactContext::TestRun => {
models::OutputArtifactDescendant::TestRunArtifact(models::TestRunArtifactSpec {
descendant: models::TestRunArtifactDescendant::Error(models::ErrorSpec {
symptom: self.symptom.clone(),
message: self.message.clone(),
software_infos: self.software_infos.clone(),
source_location: self.source_location.clone(),
}),
})
}
ArtifactContext::TestStep => {
models::OutputArtifactDescendant::TestStepArtifact(models::TestStepArtifactSpec {
descendant: models::TestStepArtifactDescendant::Error(models::ErrorSpec {
symptom: self.symptom.clone(),
message: self.message.clone(),
software_infos: self.software_infos.clone(),
source_location: self.source_location.clone(),
}),
})
}
pub fn to_artifact(&self) -> models::ErrorSpec {
models::ErrorSpec {
symptom: self.symptom.clone(),
message: self.message.clone(),
software_infos: self.software_infos.clone(),
source_location: self.source_location.clone(),
}
}
}
Expand Down Expand Up @@ -112,17 +96,15 @@ mod tests {
.source("", 1)
.build();

let artifact = error.to_artifact(ArtifactContext::TestRun);
let artifact = error.to_artifact();
assert_eq!(
artifact,
models::OutputArtifactDescendant::TestRunArtifact(models::TestRunArtifactSpec {
descendant: models::TestRunArtifactDescendant::Error(models::ErrorSpec {
symptom: error.symptom.clone(),
message: error.message.clone(),
software_infos: error.software_infos.clone(),
source_location: error.source_location.clone(),
}),
})
models::ErrorSpec {
symptom: error.symptom.clone(),
message: error.message.clone(),
software_infos: error.software_infos.clone(),
source_location: error.source_location.clone(),
}
);

Ok(())
Expand All @@ -136,17 +118,15 @@ mod tests {
.source("", 1)
.build();

let artifact = error.to_artifact(ArtifactContext::TestStep);
let artifact = error.to_artifact();
assert_eq!(
artifact,
models::OutputArtifactDescendant::TestStepArtifact(models::TestStepArtifactSpec {
descendant: models::TestStepArtifactDescendant::Error(models::ErrorSpec {
symptom: error.symptom.clone(),
message: error.message.clone(),
software_infos: error.software_infos.clone(),
source_location: error.source_location.clone(),
}),
})
models::ErrorSpec {
symptom: error.symptom.clone(),
message: error.message.clone(),
software_infos: error.software_infos.clone(),
source_location: error.source_location.clone(),
}
);

Ok(())
Expand All @@ -155,60 +135,52 @@ mod tests {
#[test]
fn test_error() -> Result<()> {
let expected_run = serde_json::json!({
"testRunArtifact": {
"error": {
"message": "message",
"softwareInfoIds": [
{
"computerSystem": null,
"name": "name",
"revision": null,
"softwareInfoId":
"software_id",
"softwareType": null,
"version": null
},
{
"computerSystem": null,
"name": "name",
"revision": null,
"softwareInfoId":
"software_id",
"softwareType": null,
"version": null
}
],
"sourceLocation": {"file": "file.rs", "line": 1},
"symptom": "symptom"
"message": "message",
"softwareInfoIds": [
{
"computerSystem": null,
"name": "name",
"revision": null,
"softwareInfoId":
"software_id",
"softwareType": null,
"version": null
},
{
"computerSystem": null,
"name": "name",
"revision": null,
"softwareInfoId":
"software_id",
"softwareType": null,
"version": null
}
}
],
"sourceLocation": {"file": "file.rs", "line": 1},
"symptom": "symptom"
});
let expected_step = serde_json::json!({
"testStepArtifact": {
"error": {
"message": "message",
"softwareInfoIds": [
{
"computerSystem": null,
"name": "name",
"revision": null,
"softwareInfoId": "software_id",
"softwareType": null,
"version": null
},
{
"computerSystem": null,
"name": "name",
"revision": null,
"softwareInfoId": "software_id",
"softwareType": null,
"version": null
}
],
"sourceLocation": {"file":"file.rs","line":1},
"symptom":"symptom"
"message": "message",
"softwareInfoIds": [
{
"computerSystem": null,
"name": "name",
"revision": null,
"softwareInfoId": "software_id",
"softwareType": null,
"version": null
},
{
"computerSystem": null,
"name": "name",
"revision": null,
"softwareInfoId": "software_id",
"softwareType": null,
"version": null
}
}
],
"sourceLocation": {"file":"file.rs","line":1},
"symptom":"symptom"
});

let software = dut::SoftwareInfo::builder("software_id", "name").build();
Expand All @@ -219,11 +191,11 @@ mod tests {
.add_software_info(&software)
.build();

let spec_error = error.to_artifact(ArtifactContext::TestRun);
let spec_error = error.to_artifact();
let actual = serde_json::json!(spec_error);
assert_json_include!(actual: actual, expected: &expected_run);

let spec_error = error.to_artifact(ArtifactContext::TestStep);
let spec_error = error.to_artifact();
let actual = serde_json::json!(spec_error);
assert_json_include!(actual: actual, expected: &expected_step);

Expand Down
55 changes: 18 additions & 37 deletions src/output/log.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
// https://opensource.org/licenses/MIT.

use crate::output as tv;
use tv::{models, run::ArtifactContext};
use tv::models;

pub struct Log {
severity: models::LogSeverity,
Expand All @@ -18,26 +18,11 @@ impl Log {
LogBuilder::new(message)
}

pub fn to_artifact(&self, context: ArtifactContext) -> models::OutputArtifactDescendant {
match context {
ArtifactContext::TestRun => {
models::OutputArtifactDescendant::TestRunArtifact(models::TestRunArtifactSpec {
descendant: models::TestRunArtifactDescendant::Log(models::LogSpec {
severity: self.severity.clone(),
message: self.message.clone(),
source_location: self.source_location.clone(),
}),
})
}
ArtifactContext::TestStep => {
models::OutputArtifactDescendant::TestStepArtifact(models::TestStepArtifactSpec {
descendant: models::TestStepArtifactDescendant::Log(models::LogSpec {
severity: self.severity.clone(),
message: self.message.clone(),
source_location: self.source_location.clone(),
}),
})
}
pub fn to_artifact(&self) -> models::LogSpec {
models::LogSpec {
severity: self.severity.clone(),
message: self.message.clone(),
source_location: self.source_location.clone(),
}
}
}
Expand Down Expand Up @@ -92,16 +77,14 @@ mod tests {
.severity(models::LogSeverity::Info)
.build();

let artifact = log.to_artifact(ArtifactContext::TestRun);
let artifact = log.to_artifact();
assert_eq!(
artifact,
models::OutputArtifactDescendant::TestRunArtifact(models::TestRunArtifactSpec {
descendant: models::TestRunArtifactDescendant::Log(models::LogSpec {
severity: log.severity.clone(),
message: log.message.clone(),
source_location: log.source_location.clone(),
}),
})
models::LogSpec {
severity: log.severity.clone(),
message: log.message.clone(),
source_location: log.source_location.clone(),
},
);

Ok(())
Expand All @@ -113,16 +96,14 @@ mod tests {
.severity(models::LogSeverity::Info)
.build();

let artifact = log.to_artifact(ArtifactContext::TestStep);
let artifact = log.to_artifact();
assert_eq!(
artifact,
models::OutputArtifactDescendant::TestStepArtifact(models::TestStepArtifactSpec {
descendant: models::TestStepArtifactDescendant::Log(models::LogSpec {
severity: log.severity.clone(),
message: log.message.clone(),
source_location: log.source_location.clone(),
}),
})
models::LogSpec {
severity: log.severity.clone(),
message: log.message.clone(),
source_location: log.source_location.clone(),
}
);

Ok(())
Expand Down
36 changes: 25 additions & 11 deletions src/output/run.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -348,9 +348,13 @@ impl StartedTestRun {

let emitter = &self.run.state.lock().await.emitter;

let artifact = models::TestRunArtifactSpec {
descendant: models::TestRunArtifactDescendant::Log(log.to_artifact()),
};
emitter
.emit(&log.to_artifact(ArtifactContext::TestRun))
.emit(&models::OutputArtifactDescendant::TestRunArtifact(artifact))
.await?;

Check warning on line 356 in src/output/run.rs

View check run for this annotation

Codecov / codecov/patch

src/output/run.rs#L356 

Added line #L356 was not covered by tests

Ok(())
}

Expand Down Expand Up @@ -380,9 +384,13 @@ impl StartedTestRun {
pub async fn log_with_details(&self, log: &log::Log) -> Result<(), emitters::WriterError> {
let emitter = &self.run.state.lock().await.emitter;

let artifact = models::TestRunArtifactSpec {
descendant: models::TestRunArtifactDescendant::Log(log.to_artifact()),
};
emitter
.emit(&log.to_artifact(ArtifactContext::TestRun))
.emit(&models::OutputArtifactDescendant::TestRunArtifact(artifact))
.await?;

Check warning on line 392 in src/output/run.rs

View check run for this annotation

Codecov / codecov/patch

src/output/run.rs#L392 

Added line #L392 was not covered by tests

Ok(())
}

Expand All @@ -408,9 +416,13 @@ impl StartedTestRun {
let error = error::Error::builder(symptom).build();
let emitter = &self.run.state.lock().await.emitter;

let artifact = models::TestRunArtifactSpec {
descendant: models::TestRunArtifactDescendant::Error(error.to_artifact()),
};
emitter
.emit(&error.to_artifact(ArtifactContext::TestRun))
.emit(&models::OutputArtifactDescendant::TestRunArtifact(artifact))
.await?;

Check warning on line 424 in src/output/run.rs

View check run for this annotation

Codecov / codecov/patch

src/output/run.rs#L424 

Added line #L424 was not covered by tests

Ok(())
}

Expand Down Expand Up @@ -441,9 +453,13 @@ impl StartedTestRun {
let error = error::Error::builder(symptom).message(msg).build();
let emitter = &self.run.state.lock().await.emitter;

let artifact = models::TestRunArtifactSpec {
descendant: models::TestRunArtifactDescendant::Error(error.to_artifact()),
};
emitter
.emit(&error.to_artifact(ArtifactContext::TestRun))
.emit(&models::OutputArtifactDescendant::TestRunArtifact(artifact))
.await?;

Ok(())
}

Expand Down Expand Up @@ -477,9 +493,13 @@ impl StartedTestRun {
) -> Result<(), emitters::WriterError> {
let emitter = &self.run.state.lock().await.emitter;

let artifact = models::TestRunArtifactSpec {
descendant: models::TestRunArtifactDescendant::Error(error.to_artifact()),
};
emitter
.emit(&error.to_artifact(ArtifactContext::TestRun))
.emit(&models::OutputArtifactDescendant::TestRunArtifact(artifact))
.await?;

Check warning on line 501 in src/output/run.rs

View check run for this annotation

Codecov / codecov/patch

src/output/run.rs#L501 

Added line #L501 was not covered by tests

Ok(())
}

Expand Down Expand Up @@ -628,12 +648,6 @@ impl TestRunEndBuilder {
}
}

// TODO: move this away from here
pub enum ArtifactContext {
TestRun,
TestStep,
}

// TODO: this likely will go into the emitter since it's not the run's job to emit the schema version
pub struct SchemaVersion {
major: i8,
Expand Down
Loading

0 comments on commit c800560

Please sign in to comment.