test: increase Telemetry resource testing coverage #1646
Conversation
rogercoll
changed the title
|
Skip changelog? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1646 +/- ##
=======================================
+ Coverage 68.9% 69.2% +0.3%
=======================================
Files 136 136
Lines 19429 19625 +196
=======================================
+ Hits 13396 13598 +202
+ Misses 6033 6027 -6 ☔ View full report in Codecov by Sentry. |
| }; | ||
|
|
||
| // If users didn't provide a resource and there isn't a env var set. Use default one. | ||
| let default_config_provider = super::LoggerProvider::builder().build(); |
There was a problem hiding this comment.
Let's wrap it with temp_env. Just in case other tests contaminate the env var
There was a problem hiding this comment.
sounds good, added 611a3f3
| }) | ||
| .build(); | ||
|
|
||
| assert_resource(&no_service_name, SERVICE_NAME, None); |
There was a problem hiding this comment.
nit - this test is redundant, and can be removed. As we are checking the length in the next test.
There was a problem hiding this comment.
make sense to me 7947daa
| let user_provided_resource_config_provider = super::LoggerProvider::builder() | ||
| .with_config(Config { | ||
| resource: Cow::Owned(Resource::default().merge(&mut Resource::new(vec![ | ||
| KeyValue::new("my-custom-key", "my-custom-value"), |
There was a problem hiding this comment.
should we also add here a resource attribute not present in the OTEL_RESOURCE_ATTRIBUTES env var, to validate it is merged properly?
There was a problem hiding this comment.
Good point! Added 5a45089
| provider.config().resource.get(TELEMETRY_SDK_VERSION.into()), | ||
| Some(Value::from(env!("CARGO_PKG_VERSION"))) | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Good to investigate (probably as a separate PR) if we can reuse the above validation function - assert_resource() and assert_telemetry_resource() across all the providers. One option could be to move them to testing directory, and they can take resource object instead of provider as an argument.
There was a problem hiding this comment.
would it make sense to add those functions in the resource module and make them available for the whole crate? or the testing directory is preferred?
draft pr moving the functions to the resource module: https://github.com/rogercoll/opentelemetry-rust/pull/1/files
| "test_service", | ||
| )])) | ||
| .build(); | ||
| assert_service_name(custom_meter_provider, Some("test_service")); | ||
| assert_resource(&custom_meter_provider, SERVICE_NAME, Some("test_service")); |
There was a problem hiding this comment.
nit - as earlier, this test is redundant, and can be removed. As we are checking the length in the next test.
There was a problem hiding this comment.
In this case we want to assert the custom defined service_name value ("test_service"), but also that no other resources are defined (like the telemetry ones)
| @@ -310,7 +346,8 @@ mod tests { | |||
| .with_resource(Resource::empty()) | |||
| .build(); | |||
|
|
|||
| assert_service_name(no_service_name, None); | |||
| assert_resource(&no_service_name, SERVICE_NAME, None); | |||
There was a problem hiding this comment.
nit - as earlier, this test is redundant, and can be removed. As we are checking the length in the next test.
| @@ -357,6 +395,7 @@ mod tests { | |||
| }) | |||
| .build(); | |||
|
|
|||
| assert_service_name(no_service_name, None); | |||
| assert_resource(&no_service_name, SERVICE_NAME, None); | |||
There was a problem hiding this comment.
nit - as earlier this test is redundant, and can be removed. As we are checking the length in the next test.
Fixes # #1624 (comment)
Changes
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial, user-facing changes