refactor: clean up execution_spec #1727
Conversation
WalkthroughWalkthroughThe changes involve a significant restructuring of the execution spec in the test suite, introducing modularization for parsing, runtime management, and model handling. This refactor enhances the organization, readability, and functionality of the test execution process, including the addition of a mock HTTP client and updates to annotation syntax. Changes
Assessment against linked issues
Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (5)
Files skipped from review as they are similar to previous changes (3)
Additional Context UsedPath-based Instructions (2)
Additional comments not posted (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
github-actions
bot
added
the
type: chore
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1727 +/- ##
==========================================
- Coverage 86.88% 86.65% -0.24%
==========================================
Files 155 155
Lines 15581 15581
==========================================
- Hits 13538 13502 -36
- Misses 2043 2079 +36 ☔ View full report in Codecov by Sentry. |
|
Please run -
|
tests/executionspec/spec.rs
Outdated
| specs = ExecutionSpec::filter_specs(specs); | ||
|
|
||
| for spec in specs.into_iter() { | ||
| test_spec(spec, &opentelemetry).await; |
There was a problem hiding this comment.
Why are we using test_spec if we are using datatest now?
There was a problem hiding this comment.
that is the existing assert_spec function.
renamed to test_spec.
using datatest-stable not datatest, both are different.
datatest-stable just runs the test for each file right.
test_spec is the main thing that runs the test(perform snapshot assertions) for the given spec.
There was a problem hiding this comment.
specs variable at any time contain only 0 or 1 spec based on skip, only annotations.
There was a problem hiding this comment.
datatest-stable is here to just run a test for each file nothing else.
There was a problem hiding this comment.
and seems like we should remove ExecutionSpec::filter_specs and storing specs as vec anyway since it's not applicable anymore when using datatest.
I've tested that it works fine with standart cargo options https://doc.rust-lang.org/rustc/tests/index.html#filters
|
removed old commits, commited new changes. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
tests/executionspec/spec.rs
Outdated
| .to_sdl(); | ||
|
|
||
| let snapshot_name = format!("{}_merged", spec.safe_name); | ||
|
|
||
| insta::assert_snapshot!(snapshot_name, merged); | ||
| // Resolve all configs | ||
| let mut runtime = test::create_runtime(mock_http_client.clone(), spec.env.clone(), None); | ||
| runtime.file = Arc::new(MockFileSystem::new(spec.clone())); | ||
| let reader = ConfigReader::init(runtime); | ||
|
|
||
| let server: Vec<ConfigModule> = join_all( | ||
| server | ||
| .into_iter() | ||
| .map(|config| reader.resolve(config, spec.path.parent())), | ||
| ) | ||
| .await | ||
| .into_iter() | ||
| .enumerate() | ||
| .map(|(i, result)| { | ||
| result.unwrap_or_else(|e| { | ||
| panic!( | ||
| "Couldn't resolve GraphQL in server definition #{} of {:#?}: {}", | ||
| i + 1, | ||
| spec.path, | ||
| e | ||
| ) | ||
| }) | ||
| }) | ||
| .collect(); | ||
|
|
||
| if server.len() == 1 { | ||
| let config = &server[0]; | ||
|
|
||
| // client: Check if client spec matches snapshot | ||
| let client = print_schema( | ||
| (Blueprint::try_from(config) | ||
| .context(format!("file: {}", spec.path.to_str().unwrap())) | ||
| .unwrap()) | ||
| .to_schema(), | ||
| ); | ||
| let snapshot_name = format!("{}_client", spec.safe_name); | ||
|
|
||
| insta::assert_snapshot!(snapshot_name, client); | ||
| } | ||
|
|
||
| if let Some(test_spec) = spec.test.as_ref() { | ||
| let app_ctx = spec | ||
| .app_context( | ||
| server.first().unwrap(), | ||
| spec.env.clone().unwrap_or_default(), | ||
| mock_http_client.clone(), | ||
| ) | ||
| .await; | ||
|
|
||
| // test: Run test specs | ||
| for (i, test) in test_spec.iter().enumerate() { | ||
| opentelemetry.reset(); | ||
|
|
||
| let response = run_test(app_ctx.clone(), test) | ||
| .await | ||
| .context(spec.path.to_str().unwrap().to_string()) | ||
| .unwrap(); | ||
|
|
||
| let mut headers: BTreeMap<String, String> = BTreeMap::new(); | ||
|
|
||
| for (key, value) in response.headers() { | ||
| headers.insert(key.to_string(), value.to_str().unwrap().to_string()); | ||
| } | ||
|
|
||
| let response: APIResponse = APIResponse { | ||
| status: response.status().clone().as_u16(), | ||
| headers, | ||
| body: serde_json::from_slice( | ||
| &hyper::body::to_bytes(response.into_body()).await.unwrap(), | ||
| ) | ||
| .unwrap_or(serde_json::Value::Null), | ||
| text_body: None, | ||
| }; | ||
|
|
||
| let snapshot_name = format!("{}_assert_{}", spec.safe_name, i); | ||
|
|
||
| insta::assert_json_snapshot!(snapshot_name, response); | ||
|
|
||
| if test.test_traces { | ||
| let snapshot_name = format!("{}_assert_traces_{}", spec.safe_name, i); | ||
| insta::assert_json_snapshot!(snapshot_name, opentelemetry.get_traces().unwrap()); | ||
| } | ||
|
|
||
| if test.test_metrics { | ||
| let snapshot_name = format!("{}_assert_metrics_{}", spec.safe_name, i); | ||
| insta::assert_json_snapshot!( | ||
| snapshot_name, | ||
| opentelemetry.get_metrics().await.unwrap() | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| mock_http_client.test_hits(&spec.path); | ||
| } | ||
|
|
There was a problem hiding this comment.
Consider refactoring test_spec to improve modularity and readability.
The test_spec function is quite lengthy and handles multiple responsibilities, such as handling SDL errors, merging configurations, and running tests based on the spec. It would be beneficial to break this function into smaller, more focused functions. This can enhance readability, maintainability, and ease of testing.
tests/executionspec/spec.rs
Outdated
| if !matches!(source, Source::GraphQL) { | ||
| panic!("Cannot use \"sdl error\" directive with a non-GraphQL server block."); | ||
| } |
There was a problem hiding this comment.
Ensure proper error handling for non-GraphQL server blocks.
- panic!("Cannot use \"sdl error\" directive with a non-GraphQL server block.");
+ return Err(anyhow::anyhow!("Cannot use \"sdl error\" directive with a non-GraphQL server block."));Using panic! for error handling in production code is generally discouraged because it causes the thread to unwind and potentially terminate the program. It's better to return an error and allow the caller to decide how to handle it.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if !matches!(source, Source::GraphQL) { | |
| panic!("Cannot use \"sdl error\" directive with a non-GraphQL server block."); | |
| } | |
| if !matches!(source, Source::GraphQL) { | |
| return Err(anyhow::anyhow!("Cannot use \"sdl error\" directive with a non-GraphQL server block.")); | |
| } |
tests/executionspec/spec.rs
Outdated
| panic!( | ||
| "Couldn't parse GraphQL in server definition #{} of {:#?}: {}", | ||
| i + 1, | ||
| spec.path, | ||
| e | ||
| ) |
There was a problem hiding this comment.
Improve error handling by avoiding panics.
- panic!(
- "Couldn't parse GraphQL in server definition #{} of {:#?}: {}",
- i + 1,
- spec.path,
- e
- )
+ return Err(anyhow::anyhow!("Couldn't parse GraphQL in server definition #{} of {:#?}: {}", i + 1, spec.path, e));Similar to the previous comment, replacing panic! with error handling improves the robustness of the application by preventing abrupt terminations.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| panic!( | |
| "Couldn't parse GraphQL in server definition #{} of {:#?}: {}", | |
| i + 1, | |
| spec.path, | |
| e | |
| ) | |
| return Err(anyhow::anyhow!("Couldn't parse GraphQL in server definition #{} of {:#?}: {}", i + 1, spec.path, e)); |
tests/executionspec/spec.rs
Outdated
| panic!( | ||
| "Spec {:#?} has \"check identity\" enabled, but its config isn't in GraphQL.", | ||
| spec.path | ||
| ); |
There was a problem hiding this comment.
Use error handling instead of panicking for non-GraphQL config in identity checks.
- panic!(
- "Spec {:#?} has \"check identity\" enabled, but its config isn't in GraphQL.",
- spec.path
- );
+ return Err(anyhow::anyhow!("Spec {:#?} has \"check identity\" enabled, but its config isn't in GraphQL.", spec.path));Continuing the theme of improving error handling, replacing panic! with a returned error in this scenario allows for more graceful error management.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| panic!( | |
| "Spec {:#?} has \"check identity\" enabled, but its config isn't in GraphQL.", | |
| spec.path | |
| ); | |
| return Err(anyhow::anyhow!("Spec {:#?} has \"check identity\" enabled, but its config isn't in GraphQL.", spec.path)); |
|
@tusharmath |
webbdays
force-pushed
the
1500
branch
2 times, most recently
from
4bf91c9 to
b79dd30
Compare
|
Hi @tusharmath, [ Done] cargo insta test --delete-unreferenced-snapshots [No conflicts now] Resolve conflicts [Removed test_spec success log] Drop the logs since you are using datatest [ Pending, i am not aware of this. please guide] Fix reader is not registered error on console [Looking into it, it seems the insta by default saving snapshots with prefix as module path in which insta is run.] All snapshots are being renamed, if it can be avoided we should try. And i dont understand why code coverage is failing. Thank you. |
|
@tusharmath |
|
for snapshots filenames i have remove module path prefix. |
|
@meskill can you have a look at this issue. |
I took a look into opentelemetry issue. This is because of the refactor since tests are now ran in parallel and that is causing issues with opentelemetry because we're using global state for telemetry providers and setting it to every test. My suggestion would be to drop opentelemetry from execution_spec for now for following reasons:
|
tests/execution_spec.rs
Outdated
|
|
||
| fn run_execution_spec(path: &Path) -> datatest_stable::Result<()> { | ||
| let _result = tokio_test::block_on(load_and_test_execution_spec(path)); |
There was a problem hiding this comment.
is it fine to ignore error here or there could be potentially useful info?
There was a problem hiding this comment.
from test point of view i think its enough to know failed/success (ie. if we get _result then test passed).
if not datatest_stable will fail.
There was a problem hiding this comment.
but here you're not propagating the error to datatest and in case it was Result::Err it will be ignored here, only panics will work
tests/executionspec/parse.rs
Outdated
| } | ||
|
|
||
| pub async fn from_source(path: &Path, contents: String) -> anyhow::Result<Self> { | ||
| INIT.call_once(|| {}); |
There was a problem hiding this comment.
seems like INIT is not doing anything
There was a problem hiding this comment.
Yes, but its from old implementation. i didnt touched it.
Will clean it.
tests/executionspec/spec.rs
Outdated
| specs = ExecutionSpec::filter_specs(specs); | ||
|
|
||
| for spec in specs.into_iter() { | ||
| test_spec(spec, &opentelemetry).await; |
There was a problem hiding this comment.
and seems like we should remove ExecutionSpec::filter_specs and storing specs as vec anyway since it's not applicable anymore when using datatest.
I've tested that it works fine with standart cargo options https://doc.rust-lang.org/rustc/tests/index.html#filters
tests/executionspec/spec.rs
Outdated
| let spec: ExecutionSpec = ExecutionSpec::from_source(path, contents) | ||
| .await | ||
| .map_err(|err| err.context(path.to_str().unwrap().to_string()))?; |
There was a problem hiding this comment.
| let spec: ExecutionSpec = ExecutionSpec::from_source(path, contents) | |
| .await | |
| .map_err(|err| err.context(path.to_str().unwrap().to_string()))?; | |
| let spec: ExecutionSpec = ExecutionSpec::from_source(path, contents) | |
| .await | |
| .with_context(|| path.display().to_string())?; |
tests/executionspec/spec.rs
Outdated
| #[cfg(test)] | ||
| pub mod test { |
There was a problem hiding this comment.
no need to wrap in separate module and that should be part of runtime.rs
tests/executionspec/spec.rs
Outdated
| } | ||
| } | ||
|
|
||
| async fn test_spec(spec: ExecutionSpec, opentelemetry: &InMemoryTelemetry) { |
There was a problem hiding this comment.
could we split this big function into smaller pieces to make it more readable?
|
Hi @meskill, |
yeah, annotations were used to reimplement something like this, but it doesn't work with datatest as expected anymore since |
|
@meskill |
|
will go cargo test filter way. |
|
@meskill |
tests/execution_spec.rs
Outdated
| fn run_execution_spec(path: &Path) -> datatest_stable::Result<()> { | ||
| let _result = tokio_test::block_on(load_and_test_execution_spec(path)); |
There was a problem hiding this comment.
Consider propagating errors from load_and_test_execution_spec to ensure that all test failures are correctly reported.
- let _result = tokio_test::block_on(load_and_test_execution_spec(path));
+ let result = tokio_test::block_on(load_and_test_execution_spec(path));
+ result.map_err(|e| datatest_stable::Error::new(e.to_string()))Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| fn run_execution_spec(path: &Path) -> datatest_stable::Result<()> { | |
| let _result = tokio_test::block_on(load_and_test_execution_spec(path)); | |
| fn run_execution_spec(path: &Path) -> datatest_stable::Result<()> { | |
| let result = tokio_test::block_on(load_and_test_execution_spec(path)); | |
| result.map_err(|e| datatest_stable::Error::new(e.to_string())) |
yes, please, remove opentelemetry usage from execution_spec |
|
@meskill |
tests/executionspec/runtime.rs
Outdated
| use super::model::ExecutionSpec; | ||
|
|
||
| #[derive(Serialize, Deserialize, Clone, Debug, Eq, PartialEq)] | ||
| pub struct APIRequest { |
tests/executionspec/runtime.rs
Outdated
| response: UpstreamResponse, | ||
| #[serde(default = "default_expected_hits")] | ||
| expected_hits: usize, | ||
| } |
tests/executionspec/spec.rs
Outdated
| Some(Annotation::Skip) => { | ||
| println!("{} ... {}", spec.path.display(), "skipped".blue()); | ||
| } | ||
| Some(Annotation::Only) => {} |
There was a problem hiding this comment.
only doesn't do anything, let's drop it from available annotations
tests/executionspec/spec.rs
Outdated
|
|
||
| match spec.runner { | ||
| Some(Annotation::Skip) => { | ||
| println!("{} ... {}", spec.path.display(), "skipped".blue()); |
There was a problem hiding this comment.
looks a little bit off in the output.
test run_execution_spec::call-multiple-steps-piping.md ... ok
tests/execution/graphql-dataloader-batch-keys.md ... skipped
test run_execution_spec::graphql-dataloader-batch-keys.md ... ok
test run_execution_spec::batching-default.md ... ok
can we improve it?
You can take a look how ok tests are logged in https://github.com/nextest-rs/datatest-stable/blob/main/src/utils.rs#L34 and https://github.com/LukasKalbertodt/libtest-mimic/blob/master/src/printer.rs#L118
There was a problem hiding this comment.
@meskill
I think We need to track the max length of test file_name.
There was a problem hiding this comment.
if it's complicated let's not bother for now
|
We'll need to update docs https://tailcall.run/docs/contributors/tests/#annotation for this and add instructions how to filter tests |
tests/executionspec/runtime.rs
Outdated
| pub test_traces: bool, | ||
| #[serde(default)] | ||
| pub test_metrics: bool, | ||
| } |
|
I have moved all structs to model? is it ok. |
tests/executionspec/env.rs
Outdated
| impl TestEnvIO { | ||
| pub fn init(vars: Option<HashMap<String, String>>) -> Self { | ||
| Self { vars: vars.unwrap_or_default() } | ||
| } | ||
| } |
There was a problem hiding this comment.
The constructor init for TestEnvIO is correctly implemented. Consider adding documentation to explain the use of unwrap_or_default for clarity.
/// Initializes a new `TestEnvIO` with the given environment variables.
/// If `None` is passed, initializes with default values.
pub fn init(vars: Option<HashMap<String, String>>) -> Self {
Self { vars: vars.unwrap_or_default() }
}
tests/executionspec/model.rs
Outdated
| fn default_status() -> u16 { | ||
| 200 | ||
| } | ||
|
|
||
| fn default_expected_hits() -> usize { | ||
| 1 | ||
| } |
There was a problem hiding this comment.
Helper functions default_status and default_expected_hits are correctly implemented. Consider adding inline documentation for these functions.
/// Returns the default HTTP status code for responses.
fn default_status() -> u16 {
200
}
/// Returns the default expected number of hits for mock objects.
fn default_expected_hits() -> usize {
1
}
tests/executionspec/file.rs
Outdated
| #[async_trait::async_trait] | ||
| impl FileIO for MockFileSystem { | ||
| async fn write<'a>(&'a self, _path: &'a str, _content: &'a [u8]) -> anyhow::Result<()> { | ||
| Err(anyhow!("Cannot write to a file in an execution spec")) | ||
| } | ||
|
|
||
| async fn read<'a>(&'a self, path: &'a str) -> anyhow::Result<String> { | ||
| let base = PathBuf::from(path); | ||
| let path = base | ||
| .file_name() | ||
| .context("Invalid file path")? | ||
| .to_str() | ||
| .context("Invalid OsString")?; | ||
| match self.spec.files.get(path) { | ||
| Some(x) => Ok(x.to_owned()), | ||
| None => Err(anyhow!("No such file or directory (os error 2)")), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The implementation of the FileIO trait for MockFileSystem correctly handles read operations using a mock approach. The error handling is appropriate, but consider improving the error messages for better clarity.
Err(anyhow!("Cannot write to a file in an execution spec: write operation not supported"))
tests/executionspec/http.rs
Outdated
| #[async_trait::async_trait] | ||
| impl HttpIO for MockHttpClient { | ||
| async fn execute(&self, req: reqwest::Request) -> anyhow::Result<Response<Bytes>> { | ||
| // Determine if the request is a GRPC request based on PORT | ||
| let is_grpc = req.url().as_str().contains("50051"); | ||
|
|
||
| // Try to find a matching mock for the incoming request. | ||
| let execution_mock = self | ||
| .mocks | ||
| .iter() | ||
| .find(|mock| { | ||
| let mock_req = &mock.mock.request; | ||
| let method_match = req.method() == mock_req.0.method.clone().to_hyper(); | ||
| let url_match = req.url().as_str() == mock_req.0.url.clone().as_str(); | ||
| let req_body = match req.body() { | ||
| Some(body) => { | ||
| if let Some(bytes) = body.as_bytes() { | ||
| if let Ok(body_str) = std::str::from_utf8(bytes) { | ||
| Value::from(body_str) | ||
| } else { | ||
| Value::Null | ||
| } | ||
| } else { | ||
| Value::Null | ||
| } | ||
| } | ||
| None => Value::Null, | ||
| }; | ||
| let body_match = req_body == mock_req.0.body; | ||
| let headers_match = req | ||
| .headers() | ||
| .iter() | ||
| .filter(|(key, _)| *key != "content-type") | ||
| .all(|(key, value)| { | ||
| let header_name = key.to_string(); | ||
|
|
||
| let header_value = value.to_str().unwrap(); | ||
| let mock_header_value = "".to_string(); | ||
| let mock_header_value = mock_req | ||
| .0 | ||
| .headers | ||
| .get(&header_name) | ||
| .unwrap_or(&mock_header_value); | ||
| header_value == mock_header_value | ||
| }); | ||
| method_match && url_match && headers_match && (body_match || is_grpc) | ||
| }) | ||
| .ok_or(anyhow!( | ||
| "No mock found for request: {:?} {} in {}", | ||
| req.method(), | ||
| req.url(), | ||
| self.spec_path | ||
| ))?; | ||
|
|
||
| execution_mock.actual_hits.fetch_add(1, Ordering::Relaxed); | ||
|
|
||
| // Clone the response from the mock to avoid borrowing issues. | ||
| let mock_response = execution_mock.mock.response.clone(); | ||
|
|
||
| // Build the response with the status code from the mock. | ||
| let status_code = reqwest::StatusCode::from_u16(mock_response.0.status)?; | ||
|
|
||
| if status_code.is_client_error() || status_code.is_server_error() { | ||
| return Err(anyhow::format_err!("Status code error")); | ||
| } | ||
|
|
||
| let mut response = Response { status: status_code, ..Default::default() }; | ||
|
|
||
| // Insert headers from the mock into the response. | ||
| for (key, value) in mock_response.0.headers { | ||
| let header_name = HeaderName::from_str(&key)?; | ||
| let header_value = HeaderValue::from_str(&value)?; | ||
| response.headers.insert(header_name, header_value); | ||
| } | ||
|
|
||
| // Special Handling for GRPC | ||
| if let Some(body) = mock_response.0.text_body { | ||
| // Return plaintext body if specified | ||
| let body = string_to_bytes(&body); | ||
| response.body = Bytes::from_iter(body); | ||
| } else if is_grpc { | ||
| // Special Handling for GRPC | ||
| let body = string_to_bytes(mock_response.0.body.as_str().unwrap_or_default()); | ||
| response.body = Bytes::from_iter(body); | ||
| } else { | ||
| let body = serde_json::to_vec(&mock_response.0.body)?; | ||
| response.body = Bytes::from_iter(body); | ||
| } | ||
|
|
||
| Ok(response) | ||
| } | ||
| } |
There was a problem hiding this comment.
The implementation of the HttpIO trait for MockHttpClient contains complex logic for matching and handling HTTP requests. The error handling and response construction are well-implemented. However, consider adding more detailed comments to explain the logic, especially for the GRPC handling and header matching.
// This block handles the special case for GRPC requests.
if is_grpc {
// Special Handling for GRPC
let body = string_to_bytes(mock_response.0.body.as_str().unwrap_or_default());
response.body = Bytes::from_iter(body);
} else {
let body = serde_json::to_vec(&mock_response.0.body)?;
response.body = Bytes::from_iter(body);
}
There was a problem hiding this comment.
Actionable comments posted: 2
Out of diff range and nitpick comments (1)
tests/core/file.rs (1)
20-38: The implementation of theFileIOtrait forMockFileSystemcorrectly handles read operations with comprehensive error handling. However, the write operation is intentionally blocked, which should be clearly documented.Consider adding a comment explaining why the write operation is blocked in
MockFileSystem.
tests/core/runtime.rs
Outdated
| pub fn create_runtime( | ||
| http_client: Arc<MockHttpClient>, | ||
| env: Option<HashMap<String, String>>, | ||
| script: Option<blueprint::Script>, | ||
| ) -> TargetRuntime { | ||
| let http = if let Some(script) = script.clone() { | ||
| javascript::init_http(http_client.clone(), script) | ||
| } else { | ||
| http_client.clone() | ||
| }; | ||
|
|
||
| let http2 = if let Some(script) = script { | ||
| javascript::init_http(http_client.clone(), script) | ||
| } else { | ||
| http_client.clone() | ||
| }; | ||
|
|
||
| let file = TestFileIO::init(); | ||
| let env = TestEnvIO::init(env); | ||
|
|
||
| TargetRuntime { | ||
| http, | ||
| http2_only: http2, | ||
| env: Arc::new(env), | ||
| file: Arc::new(file), | ||
| cache: Arc::new(InMemoryCache::new()), | ||
| extensions: Arc::new(vec![]), | ||
| } | ||
| } |
There was a problem hiding this comment.
The function create_runtime is well-structured and makes effective use of cloning and Arc for thread safety. However, the duplication of the http initialization (lines 85-95) should be addressed to avoid redundancy.
- let http = if let Some(script) = script.clone() {
- javascript::init_http(http_client.clone(), script)
- } else {
- http_client.clone()
- };
- let http2 = if let Some(script) = script {
- javascript::init_http(http_client.clone(), script)
- } else {
- http_client.clone()
- };
+ let http = if let Some(script) = script.clone() {
+ javascript::init_http(http_client.clone(), script)
+ } else {
+ http_client.clone()
+ };
+ let http2 = http.clone();Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| pub fn create_runtime( | |
| http_client: Arc<MockHttpClient>, | |
| env: Option<HashMap<String, String>>, | |
| script: Option<blueprint::Script>, | |
| ) -> TargetRuntime { | |
| let http = if let Some(script) = script.clone() { | |
| javascript::init_http(http_client.clone(), script) | |
| } else { | |
| http_client.clone() | |
| }; | |
| let http2 = if let Some(script) = script { | |
| javascript::init_http(http_client.clone(), script) | |
| } else { | |
| http_client.clone() | |
| }; | |
| let file = TestFileIO::init(); | |
| let env = TestEnvIO::init(env); | |
| TargetRuntime { | |
| http, | |
| http2_only: http2, | |
| env: Arc::new(env), | |
| file: Arc::new(file), | |
| cache: Arc::new(InMemoryCache::new()), | |
| extensions: Arc::new(vec![]), | |
| } | |
| } | |
| pub fn create_runtime( | |
| http_client: Arc<MockHttpClient>, | |
| env: Option<HashMap<String, String>>, | |
| script: Option<blueprint::Script>, | |
| ) -> TargetRuntime { | |
| let http = if let Some(script) = script.clone() { | |
| javascript::init_http(http_client.clone(), script) | |
| } else { | |
| http_client.clone() | |
| }; | |
| let http2 = http.clone(); | |
| let file = TestFileIO::init(); | |
| let env = TestEnvIO::init(env); | |
| TargetRuntime { | |
| http, | |
| http2_only: http2, | |
| env: Arc::new(env), | |
| file: Arc::new(file), | |
| cache: Arc::new(InMemoryCache::new()), | |
| extensions: Arc::new(vec![]), | |
| } | |
| } |
tests/core/http.rs
Outdated
| #[async_trait::async_trait] | ||
| impl HttpIO for MockHttpClient { | ||
| async fn execute(&self, req: reqwest::Request) -> anyhow::Result<Response<Bytes>> { | ||
| // Determine if the request is a GRPC request based on PORT | ||
| let is_grpc = req.url().as_str().contains("50051"); | ||
|
|
||
| // Try to find a matching mock for the incoming request. | ||
| let execution_mock = self | ||
| .mocks | ||
| .iter() | ||
| .find(|mock| { | ||
| let mock_req = &mock.mock.request; | ||
| let method_match = req.method() == mock_req.0.method.clone().to_hyper(); | ||
| let url_match = req.url().as_str() == mock_req.0.url.clone().as_str(); | ||
| let req_body = match req.body() { | ||
| Some(body) => { | ||
| if let Some(bytes) = body.as_bytes() { | ||
| if let Ok(body_str) = std::str::from_utf8(bytes) { | ||
| Value::from(body_str) | ||
| } else { | ||
| Value::Null | ||
| } | ||
| } else { | ||
| Value::Null | ||
| } | ||
| } | ||
| None => Value::Null, | ||
| }; | ||
| let body_match = req_body == mock_req.0.body; | ||
| let headers_match = req | ||
| .headers() | ||
| .iter() | ||
| .filter(|(key, _)| *key != "content-type") | ||
| .all(|(key, value)| { | ||
| let header_name = key.to_string(); | ||
|
|
||
| let header_value = value.to_str().unwrap(); | ||
| let mock_header_value = "".to_string(); | ||
| let mock_header_value = mock_req | ||
| .0 | ||
| .headers | ||
| .get(&header_name) | ||
| .unwrap_or(&mock_header_value); | ||
| header_value == mock_header_value | ||
| }); | ||
| method_match && url_match && headers_match && (body_match || is_grpc) | ||
| }) | ||
| .ok_or(anyhow!( | ||
| "No mock found for request: {:?} {} in {}", | ||
| req.method(), | ||
| req.url(), | ||
| self.spec_path | ||
| ))?; | ||
|
|
||
| execution_mock.actual_hits.fetch_add(1, Ordering::Relaxed); | ||
|
|
||
| // Clone the response from the mock to avoid borrowing issues. | ||
| let mock_response = execution_mock.mock.response.clone(); | ||
|
|
||
| // Build the response with the status code from the mock. | ||
| let status_code = reqwest::StatusCode::from_u16(mock_response.0.status)?; | ||
|
|
||
| if status_code.is_client_error() || status_code.is_server_error() { | ||
| return Err(anyhow::format_err!("Status code error")); | ||
| } | ||
|
|
||
| let mut response = Response { status: status_code, ..Default::default() }; | ||
|
|
||
| // Insert headers from the mock into the response. | ||
| for (key, value) in mock_response.0.headers { | ||
| let header_name = HeaderName::from_str(&key)?; | ||
| let header_value = HeaderValue::from_str(&value)?; | ||
| response.headers.insert(header_name, header_value); | ||
| } | ||
|
|
||
| // Special Handling for GRPC | ||
| if let Some(body) = mock_response.0.text_body { | ||
| // Return plaintext body if specified | ||
| let body = string_to_bytes(&body); | ||
| response.body = Bytes::from_iter(body); | ||
| } else if is_grpc { | ||
| // Special Handling for GRPC | ||
| let body = string_to_bytes(mock_response.0.body.as_str().unwrap_or_default()); | ||
| response.body = Bytes::from_iter(body); | ||
| } else { | ||
| let body = serde_json::to_vec(&mock_response.0.body)?; | ||
| response.body = Bytes::from_iter(body); | ||
| } | ||
|
|
||
| Ok(response) | ||
| } | ||
| } |
There was a problem hiding this comment.
The implementation of the HttpIO trait in MockHttpClient includes comprehensive matching logic for requests and detailed response handling. The handling of GRPC and JSON responses is particularly well-implemented. However, consider simplifying the complex conditional logic to improve readability and maintainability.
Consider refactoring the complex conditional logic in the execute method to improve readability and maintainability.
| let spec_path = spec | ||
| .path | ||
| .strip_prefix(std::env::current_dir().unwrap()) | ||
| .unwrap_or(&spec.path) | ||
| .to_string_lossy() | ||
| .into_owned(); |
There was a problem hiding this comment.
Consider handling potential errors more gracefully in path operations to avoid panics.
- .unwrap()
+ .unwrap_or_else(|e| panic!("Error getting current directory: {}", e))Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| let spec_path = spec | |
| .path | |
| .strip_prefix(std::env::current_dir().unwrap()) | |
| .unwrap_or(&spec.path) | |
| .to_string_lossy() | |
| .into_owned(); | |
| let spec_path = spec | |
| .path | |
| .strip_prefix(std::env::current_dir().unwrap_or_else(|e| panic!("Error getting current directory: {}", e))) | |
| .unwrap_or(&spec.path) | |
| .to_string_lossy() | |
| .into_owned(); |
| fn string_to_bytes(input: &str) -> Vec<u8> { | ||
| let mut bytes = Vec::new(); | ||
| let mut chars = input.chars().peekable(); | ||
|
|
||
| while let Some(c) = chars.next() { | ||
| match c { | ||
| '\\' => match chars.next() { | ||
| Some('0') => bytes.push(0), | ||
| Some('n') => bytes.push(b'\n'), | ||
| Some('t') => bytes.push(b'\t'), | ||
| Some('r') => bytes.push(b'\r'), | ||
| Some('\\') => bytes.push(b'\\'), | ||
| Some('\"') => bytes.push(b'\"'), | ||
| Some('x') => { | ||
| let mut hex = chars.next().unwrap().to_string(); | ||
| hex.push(chars.next().unwrap()); | ||
| let byte = u8::from_str_radix(&hex, 16).unwrap(); | ||
| bytes.push(byte); | ||
| } | ||
| _ => panic!("Unsupported escape sequence"), | ||
| }, | ||
| _ => bytes.push(c as u8), | ||
| } | ||
| } | ||
|
|
||
| bytes | ||
| } |
There was a problem hiding this comment.
Improve error handling in string_to_bytes to avoid panics with unexpected characters.
- .unwrap()
+ .ok_or_else(|| anyhow!("Invalid character in input string"))?Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| fn string_to_bytes(input: &str) -> Vec<u8> { | |
| let mut bytes = Vec::new(); | |
| let mut chars = input.chars().peekable(); | |
| while let Some(c) = chars.next() { | |
| match c { | |
| '\\' => match chars.next() { | |
| Some('0') => bytes.push(0), | |
| Some('n') => bytes.push(b'\n'), | |
| Some('t') => bytes.push(b'\t'), | |
| Some('r') => bytes.push(b'\r'), | |
| Some('\\') => bytes.push(b'\\'), | |
| Some('\"') => bytes.push(b'\"'), | |
| Some('x') => { | |
| let mut hex = chars.next().unwrap().to_string(); | |
| hex.push(chars.next().unwrap()); | |
| let byte = u8::from_str_radix(&hex, 16).unwrap(); | |
| bytes.push(byte); | |
| } | |
| _ => panic!("Unsupported escape sequence"), | |
| }, | |
| _ => bytes.push(c as u8), | |
| } | |
| } | |
| bytes | |
| } | |
| fn string_to_bytes(input: &str) -> Vec<u8> { | |
| let mut bytes = Vec::new(); | |
| let mut chars = input.chars().peekable(); | |
| while let Some(c) = chars.next() { | |
| match c { | |
| '\\' => match chars.next() { | |
| Some('0') => bytes.push(0), | |
| Some('n') => bytes.push(b'\n'), | |
| Some('t') => bytes.push(b'\t'), | |
| Some('r') => bytes.push(b'\r'), | |
| Some('\\') => bytes.push(b'\\'), | |
| Some('\"') => bytes.push(b'\"'), | |
| Some('x') => { | |
| let mut hex = chars.next().ok_or_else(|| anyhow!("Invalid character in input string"))?.to_string(); | |
| hex.push(chars.next().ok_or_else(|| anyhow!("Invalid character in input string"))?); | |
| let byte = u8::from_str_radix(&hex, 16).ok_or_else(|| anyhow!("Invalid character in input string"))?; | |
| bytes.push(byte); | |
| } | |
| _ => panic!("Unsupported escape sequence"), | |
| }, | |
| _ => bytes.push(c as u8), | |
| } | |
| } | |
| bytes | |
| } |
| #[async_trait::async_trait] | ||
| impl HttpIO for Http { | ||
| async fn execute(&self, req: reqwest::Request) -> anyhow::Result<Response<Bytes>> { | ||
| // Determine if the request is a GRPC request based on PORT | ||
| let is_grpc = req.url().as_str().contains("50051"); | ||
|
|
||
| // Try to find a matching mock for the incoming request. | ||
| let execution_mock = self | ||
| .mocks | ||
| .iter() | ||
| .find(|mock| { | ||
| let mock_req = &mock.mock.request; | ||
| let method_match = req.method() == mock_req.0.method.clone().to_hyper(); | ||
| let url_match = req.url().as_str() == mock_req.0.url.clone().as_str(); | ||
| let req_body = match req.body() { | ||
| Some(body) => { | ||
| if let Some(bytes) = body.as_bytes() { | ||
| if let Ok(body_str) = std::str::from_utf8(bytes) { | ||
| Value::from(body_str) | ||
| } else { | ||
| Value::Null | ||
| } | ||
| } else { | ||
| Value::Null | ||
| } | ||
| } | ||
| None => Value::Null, | ||
| }; | ||
| let body_match = req_body == mock_req.0.body; | ||
| let headers_match = req | ||
| .headers() | ||
| .iter() | ||
| .filter(|(key, _)| *key != "content-type") | ||
| .all(|(key, value)| { | ||
| let header_name = key.to_string(); | ||
|
|
||
| let header_value = value.to_str().unwrap(); | ||
| let mock_header_value = "".to_string(); | ||
| let mock_header_value = mock_req | ||
| .0 | ||
| .headers | ||
| .get(&header_name) | ||
| .unwrap_or(&mock_header_value); | ||
| header_value == mock_header_value | ||
| }); | ||
| method_match && url_match && headers_match && (body_match || is_grpc) | ||
| }) | ||
| .ok_or(anyhow!( | ||
| "No mock found for request: {:?} {} in {}", | ||
| req.method(), | ||
| req.url(), | ||
| self.spec_path | ||
| ))?; | ||
|
|
||
| execution_mock.actual_hits.fetch_add(1, Ordering::Relaxed); | ||
|
|
||
| // Clone the response from the mock to avoid borrowing issues. | ||
| let mock_response = execution_mock.mock.response.clone(); | ||
|
|
||
| // Build the response with the status code from the mock. | ||
| let status_code = reqwest::StatusCode::from_u16(mock_response.0.status)?; | ||
|
|
||
| if status_code.is_client_error() || status_code.is_server_error() { | ||
| return Err(anyhow::format_err!("Status code error")); | ||
| } | ||
|
|
||
| let mut response = Response { status: status_code, ..Default::default() }; | ||
|
|
||
| // Insert headers from the mock into the response. | ||
| for (key, value) in mock_response.0.headers { | ||
| let header_name = HeaderName::from_str(&key)?; | ||
| let header_value = HeaderValue::from_str(&value)?; | ||
| response.headers.insert(header_name, header_value); | ||
| } | ||
|
|
||
| // Special Handling for GRPC | ||
| if let Some(body) = mock_response.0.text_body { | ||
| // Return plaintext body if specified | ||
| let body = string_to_bytes(&body); | ||
| response.body = Bytes::from_iter(body); | ||
| } else if is_grpc { | ||
| // Special Handling for GRPC | ||
| let body = string_to_bytes(mock_response.0.body.as_str().unwrap_or_default()); | ||
| response.body = Bytes::from_iter(body); | ||
| } else { | ||
| let body = serde_json::to_vec(&mock_response.0.body)?; | ||
| response.body = Bytes::from_iter(body); | ||
| } | ||
|
|
||
| Ok(response) | ||
| } | ||
| } |
There was a problem hiding this comment.
Refactor execute function for better readability and reduce cloning. Improve error handling for more descriptive messages.
- .clone()
+ // Use references or other strategies to avoid cloning where possible
- "No mock found for request: {:?} {} in {}"
+ "No matching mock found for request method: {}, URL: {} in specification: {}"Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| #[async_trait::async_trait] | |
| impl HttpIO for Http { | |
| async fn execute(&self, req: reqwest::Request) -> anyhow::Result<Response<Bytes>> { | |
| // Determine if the request is a GRPC request based on PORT | |
| let is_grpc = req.url().as_str().contains("50051"); | |
| // Try to find a matching mock for the incoming request. | |
| let execution_mock = self | |
| .mocks | |
| .iter() | |
| .find(|mock| { | |
| let mock_req = &mock.mock.request; | |
| let method_match = req.method() == mock_req.0.method.clone().to_hyper(); | |
| let url_match = req.url().as_str() == mock_req.0.url.clone().as_str(); | |
| let req_body = match req.body() { | |
| Some(body) => { | |
| if let Some(bytes) = body.as_bytes() { | |
| if let Ok(body_str) = std::str::from_utf8(bytes) { | |
| Value::from(body_str) | |
| } else { | |
| Value::Null | |
| } | |
| } else { | |
| Value::Null | |
| } | |
| } | |
| None => Value::Null, | |
| }; | |
| let body_match = req_body == mock_req.0.body; | |
| let headers_match = req | |
| .headers() | |
| .iter() | |
| .filter(|(key, _)| *key != "content-type") | |
| .all(|(key, value)| { | |
| let header_name = key.to_string(); | |
| let header_value = value.to_str().unwrap(); | |
| let mock_header_value = "".to_string(); | |
| let mock_header_value = mock_req | |
| .0 | |
| .headers | |
| .get(&header_name) | |
| .unwrap_or(&mock_header_value); | |
| header_value == mock_header_value | |
| }); | |
| method_match && url_match && headers_match && (body_match || is_grpc) | |
| }) | |
| .ok_or(anyhow!( | |
| "No mock found for request: {:?} {} in {}", | |
| req.method(), | |
| req.url(), | |
| self.spec_path | |
| ))?; | |
| execution_mock.actual_hits.fetch_add(1, Ordering::Relaxed); | |
| // Clone the response from the mock to avoid borrowing issues. | |
| let mock_response = execution_mock.mock.response.clone(); | |
| // Build the response with the status code from the mock. | |
| let status_code = reqwest::StatusCode::from_u16(mock_response.0.status)?; | |
| if status_code.is_client_error() || status_code.is_server_error() { | |
| return Err(anyhow::format_err!("Status code error")); | |
| } | |
| let mut response = Response { status: status_code, ..Default::default() }; | |
| // Insert headers from the mock into the response. | |
| for (key, value) in mock_response.0.headers { | |
| let header_name = HeaderName::from_str(&key)?; | |
| let header_value = HeaderValue::from_str(&value)?; | |
| response.headers.insert(header_name, header_value); | |
| } | |
| // Special Handling for GRPC | |
| if let Some(body) = mock_response.0.text_body { | |
| // Return plaintext body if specified | |
| let body = string_to_bytes(&body); | |
| response.body = Bytes::from_iter(body); | |
| } else if is_grpc { | |
| // Special Handling for GRPC | |
| let body = string_to_bytes(mock_response.0.body.as_str().unwrap_or_default()); | |
| response.body = Bytes::from_iter(body); | |
| } else { | |
| let body = serde_json::to_vec(&mock_response.0.body)?; | |
| response.body = Bytes::from_iter(body); | |
| } | |
| Ok(response) | |
| } | |
| } | |
| #[async_trait::async_trait] | |
| impl HttpIO for Http { | |
| async fn execute(&self, req: reqwest::Request) -> anyhow::Result<Response<Bytes>> { | |
| // Determine if the request is a GRPC request based on PORT | |
| let is_grpc = req.url().as_str().contains("50051"); | |
| // Try to find a matching mock for the incoming request. | |
| let execution_mock = self | |
| .mocks | |
| .iter() | |
| .find(|mock| { | |
| let mock_req = &mock.mock.request; | |
| let method_match = req.method() == mock_req.0.method.clone().to_hyper(); | |
| let url_match = req.url().as_str() == mock_req.0.url.clone().as_str(); | |
| let req_body = match req.body() { | |
| Some(body) => { | |
| if let Some(bytes) = body.as_bytes() { | |
| if let Ok(body_str) = std::str::from_utf8(bytes) { | |
| Value::from(body_str) | |
| } else { | |
| Value::Null | |
| } | |
| } else { | |
| Value::Null | |
| } | |
| } | |
| None => Value::Null, | |
| }; | |
| let body_match = req_body == mock_req.0.body; | |
| let headers_match = req | |
| .headers() | |
| .iter() | |
| .filter(|(key, _)| *key != "content-type") | |
| .all(|(key, value)| { | |
| let header_name = key.to_string(); | |
| let header_value = value.to_str().unwrap(); | |
| let mock_header_value = "".to_string(); | |
| let mock_header_value = mock_req | |
| .0 | |
| .headers | |
| .get(&header_name) | |
| .unwrap_or(&mock_header_value); | |
| header_value == mock_header_value | |
| }); | |
| method_match && url_match && headers_match && (body_match || is_grpc) | |
| }) | |
| .ok_or(anyhow!( | |
| "No matching mock found for request method: {}, URL: {} in specification: {}", | |
| req.method(), | |
| req.url(), | |
| self.spec_path | |
| ))?; | |
| execution_mock.actual_hits.fetch_add(1, Ordering::Relaxed); | |
| // Clone the response from the mock to avoid borrowing issues. | |
| let mock_response = execution_mock.mock.response.clone(); | |
| // Build the response with the status code from the mock. | |
| let status_code = reqwest::StatusCode::from_u16(mock_response.0.status)?; | |
| if status_code.is_client_error() || status_code.is_server_error() { | |
| return Err(anyhow::format_err!("Status code error")); | |
| } | |
| let mut response = Response { status: status_code, ..Default::default() }; | |
| // Insert headers from the mock into the response. | |
| for (key, value) in mock_response.0.headers { | |
| let header_name = HeaderName::from_str(&key)?; | |
| let header_value = HeaderValue::from_str(&value)?; | |
| response.headers.insert(header_name, header_value); | |
| } | |
| // Special Handling for GRPC | |
| if let Some(body) = mock_response.0.text_body { | |
| // Return plaintext body if specified | |
| let body = string_to_bytes(&body); | |
| response.body = Bytes::from_iter(body); | |
| } else if is_grpc { | |
| // Special Handling for GRPC | |
| let body = string_to_bytes(mock_response.0.body.as_str().unwrap_or_default()); | |
| response.body = Bytes::from_iter(body); | |
| } else { | |
| let body = serde_json::to_vec(&mock_response.0.body)?; | |
| response.body = Bytes::from_iter(body); | |
| } | |
| Ok(response) | |
| } | |
| } |
| }?; | ||
|
|
||
| let source = Source::from_str(&lang)?; | ||
|
|
||
| match name { | ||
| "server" => { | ||
| // Server configs are only parsed if the test isn't skipped. | ||
| server.push((source, content)); | ||
| } | ||
| "mock" => { | ||
| if mock.is_none() { | ||
| mock = match source { | ||
| Source::Json => Ok(serde_json::from_str(&content)?), | ||
| Source::Yml => Ok(serde_yaml::from_str(&content)?), | ||
| _ => Err(anyhow!("Unexpected language in mock block in {:?} (only JSON and YAML are supported)", path)), | ||
| }?; | ||
| } else { | ||
| return Err(anyhow!("Unexpected number of mock blocks in {:?} (only one is allowed)", path)); | ||
| } | ||
| } | ||
| "env" => { | ||
| if env.is_none() { | ||
| env = match source { | ||
| Source::Json => Ok(serde_json::from_str(&content)?), | ||
| Source::Yml => Ok(serde_yaml::from_str(&content)?), | ||
| _ => Err(anyhow!("Unexpected language in env block in {:?} (only JSON and YAML are supported)", path)), | ||
| }?; | ||
| } else { | ||
| return Err(anyhow!("Unexpected number of env blocks in {:?} (only one is allowed)", path)); | ||
| } | ||
| } | ||
| "test" => { | ||
| if test.is_none() { | ||
| test = match source { | ||
| Source::Json => Ok(serde_json::from_str(&content)?), | ||
| Source::Yml => Ok(serde_yaml::from_str(&content)?), | ||
| _ => Err(anyhow!("Unexpected language in test block in {:?} (only JSON and YAML are supported)", path)), | ||
| }?; | ||
| } else { | ||
| return Err(anyhow!("Unexpected number of test blocks in {:?} (only one is allowed)", path)); | ||
| } | ||
| } | ||
| _ => { | ||
| return Err(anyhow!( | ||
| "Unexpected component {:?} in {:?}: {:#?}", | ||
| name, | ||
| path, | ||
| meta | ||
| )); | ||
| } | ||
| } | ||
| } | ||
| } else { | ||
| return Err(anyhow!( | ||
| "Unexpected content of code in {:?}: {:#?}", | ||
| path, | ||
| meta | ||
| )); | ||
| } | ||
| } | ||
| Node::Definition(d) => { | ||
| if let Some(title) = &d.title { | ||
| tracing::info!("Comment found in: {:?} with title: {}", path, title); | ||
| } | ||
| } | ||
| Node::ThematicBreak(_) => { | ||
| // skip this for and put execute logic in heading.depth | ||
| // above to escape ThematicBreaks like | ||
| // `---`, `***` or `___` | ||
| } | ||
| _ => return Err(anyhow!("Unexpected node in {:?}: {:#?}", path, node)), | ||
| } | ||
| } | ||
|
|
||
| if server.is_empty() { | ||
| return Err(anyhow!( | ||
| "Unexpected blocks in {:?}: You must define a GraphQL Config in an execution test.", | ||
| path, | ||
| )); | ||
| } | ||
|
|
||
| let spec = ExecutionSpec { | ||
| path: path.to_owned(), | ||
| name: name.unwrap_or_else(|| path.file_name().unwrap().to_str().unwrap().to_string()), | ||
| safe_name: path.file_name().unwrap().to_str().unwrap().to_string(), | ||
|
|
||
| server, | ||
| mock, | ||
| env, | ||
| test, | ||
| files, | ||
|
|
||
| runner, | ||
|
|
||
| check_identity, | ||
| sdl_error, | ||
| }; | ||
|
|
||
| anyhow::Ok(spec) | ||
| } |
There was a problem hiding this comment.
Refactor from_source function for better structure and readability. Consider breaking it down into smaller helper functions for handling different markdown nodes.
| pub async fn app_context( | ||
| &self, | ||
| config: &ConfigModule, | ||
| env: HashMap<String, String>, | ||
| http_client: Arc<Http>, | ||
| ) -> Arc<AppContext> { | ||
| let blueprint = Blueprint::try_from(config).unwrap(); | ||
| let http = if let Some(script) = blueprint.server.script.clone() { | ||
| javascript::init_http(http_client, script) | ||
| } else { | ||
| http_client | ||
| }; | ||
|
|
||
| let http2_only = http.clone(); | ||
|
|
||
| let runtime = TargetRuntime { | ||
| http, | ||
| http2_only, | ||
| file: Arc::new(File::new(self.clone())), | ||
| env: Arc::new(Env::init(env)), | ||
| cache: Arc::new(InMemoryCache::new()), | ||
| extensions: Arc::new(vec![]), | ||
| }; | ||
|
|
||
| let endpoints = config | ||
| .extensions | ||
| .endpoint_set | ||
| .clone() | ||
| .into_checked(&blueprint, runtime.clone()) | ||
| .await | ||
| .unwrap(); | ||
|
|
||
| Arc::new(AppContext::new(blueprint, runtime, endpoints)) | ||
| } |
There was a problem hiding this comment.
Optimize cloning and unwrapping in app_context function. Consider refactoring to initialize components more cleanly.
- .clone()
+ // Use references or other strategies to avoid cloning where possible
- .unwrap()
+ .unwrap_or_else(|e| panic!("Error initializing blueprint: {}", e))Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| pub async fn app_context( | |
| &self, | |
| config: &ConfigModule, | |
| env: HashMap<String, String>, | |
| http_client: Arc<Http>, | |
| ) -> Arc<AppContext> { | |
| let blueprint = Blueprint::try_from(config).unwrap(); | |
| let http = if let Some(script) = blueprint.server.script.clone() { | |
| javascript::init_http(http_client, script) | |
| } else { | |
| http_client | |
| }; | |
| let http2_only = http.clone(); | |
| let runtime = TargetRuntime { | |
| http, | |
| http2_only, | |
| file: Arc::new(File::new(self.clone())), | |
| env: Arc::new(Env::init(env)), | |
| cache: Arc::new(InMemoryCache::new()), | |
| extensions: Arc::new(vec![]), | |
| }; | |
| let endpoints = config | |
| .extensions | |
| .endpoint_set | |
| .clone() | |
| .into_checked(&blueprint, runtime.clone()) | |
| .await | |
| .unwrap(); | |
| Arc::new(AppContext::new(blueprint, runtime, endpoints)) | |
| } | |
| pub async fn app_context( | |
| &self, | |
| config: &ConfigModule, | |
| env: HashMap<String, String>, | |
| http_client: Arc<Http>, | |
| ) -> Arc<AppContext> { | |
| let blueprint = Blueprint::try_from(config).unwrap_or_else(|e| panic!("Error initializing blueprint: {}", e)); | |
| let http = if let Some(script) = blueprint.server.script // Use references or other strategies to avoid cloning where possible | |
| { | |
| javascript::init_http(http_client, script) | |
| } else { | |
| http_client | |
| }; | |
| let http2_only = http.clone(); | |
| let runtime = TargetRuntime { | |
| http, | |
| http2_only, | |
| file: Arc::new(File::new(self.clone())), | |
| env: Arc::new(Env::init(env)), | |
| cache: Arc::new(InMemoryCache::new()), | |
| extensions: Arc::new(vec![]), | |
| }; | |
| let endpoints = config | |
| .extensions | |
| .endpoint_set | |
| // Use references or other strategies to avoid cloning where possible | |
| .into_checked(&blueprint, runtime.clone()) | |
| .await | |
| .unwrap_or_else(|e| panic!("Error initializing endpoints: {}", e)); | |
| Arc::new(AppContext::new(blueprint, runtime, endpoints)) | |
| } |
|
@tusharmath |
tusharmath
changed the title
Co-authored-by: Tushar Mathur <[email protected]>
Summary:
Refactor execution_spec : spec.rs, runtime.rs, parse.rs was created in executionspec module.
we can use executionspec module inside execution_spec.rs to run the tests for each test file written in "tests/execution/".
changed @Assert to @test
moved and renamed snaps accordingly.
removed old snapshots location.
i have performed --delete-unreferenced-snapshots .
removed success log in test_spec function.
Issue Reference(s):
Fixes #1500
/claim #1500
Build & Testing:
[No] I ran
cargo testsuccessfully.All tests passed.
But there was an error with open telemetry for some tests when execution_spec tests are run.
"OpenTelemetry metrics error occurred. Metrics error: reader is not registered"
I have run
./lint.sh --mode=fixto fix all linting issues raised by./lint.sh --mode=check.Checklist:
<type>(<optional scope>): <title>Summary by CodeRabbit