Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
RuboCop found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
|
@xuan-cao-swi any way to easily see the coverage info? Quick check there seem to be GHA integrations available. |
Yeah, I integrated the codecov.io |
cheempz
left a comment
cheempz
left a comment
There was a problem hiding this comment.
Overall i like the improvements on test description and coverage, but from spot-checking what tests are actually doing, there are gaps between what descriptions say are being tested and actual setup/assertions.
| require './lib/solarwinds_apm/api' | ||
|
|
||
| describe 'API::OpenTelemetry#in_span delegation to OpenTelemetry tracer' do | ||
| it 'returns nil and warns when block is nil' do |
There was a problem hiding this comment.
i see the return nil is asserted. how is the "warns when block is nil" tested?
There was a problem hiding this comment.
Added assertion that logger.warn will be called.
test/api/api_test.rb
Outdated
| assert_nil result | ||
| end | ||
|
|
||
| it 'calls OpenTelemetry tracer in_span with a block' do |
There was a problem hiding this comment.
How is the "calls OTel tracer" being checed in this test?
There was a problem hiding this comment.
The name is misleading (and updated). It should just check everything works fine if block is given.
test/api/current_trace_info_test.rb
Outdated
|
|
||
| trace = SolarWindsAPM::API.current_trace_info | ||
| result = trace.for_log | ||
| assert_includes result, 'trace_id=' |
There was a problem hiding this comment.
Would be better to assert that there's something after the = sign for these fields.
| @propagator.inject(carrier, context: context) | ||
|
|
||
| assert carrier.key?('x-trace') | ||
| assert_includes carrier['x-trace'], '00-' |
There was a problem hiding this comment.
Any way to assert the value of x-trace against the context that was provided?
| carrier = {} | ||
| @propagator.inject(carrier, context: context) | ||
|
|
||
| assert carrier.key?('x-trace-options-response') |
There was a problem hiding this comment.
Same question on asserting the value of the header.
|
|
||
| # Second call should skip due to not expired | ||
| params = make_sample_params | ||
| sampler.should_sample?(params) |
There was a problem hiding this comment.
Not sure how this is asserting loop_check is skipped, and similar question for next test.
test/api/transaction_name_test.rb
Outdated
| SolarWindsAPM::OTelConfig.class_variable_get(:@@config)[:metrics_processor] = original_proc | ||
| end | ||
|
|
||
| it 'sets transaction name within a valid span context' do |
There was a problem hiding this comment.
This test implies it checks a transaction name is actually set, but there is no such assertion that i can see. Like the previous tests it's only checking the response from the call to set_transaction_name(). Also, since previous tests don't set up a valid span context, they might not really be testing the scenarios they claim to test?
There was a problem hiding this comment.
Updated both test case that provide invalid span context and valid one as part of test case
2 participants
Description
Test (if applicable)