[chore][receiver/hostmetricsreceiver] Improve cpuscraper test coverage from 61.5% to 92.3% #45225
Conversation
This commit adds a test for system.cpu.physical.count in hostmetricsreceiver cpuscraper
osullivandonal
requested review from
a team,
braydonk and
dmitryax
as code owners
|
|
||
| assert.Equal(t, test.expectedMetricCount, md.MetricCount()) | ||
|
|
||
| if md.MetricCount() > 0 { |
There was a problem hiding this comment.
The test will pass if this is 0, this should change to a require check.
There was a problem hiding this comment.
I have refactored this test and removed this line, now we use require.
| md, err := scraper.scrape(t.Context()) | ||
| require.NoError(t, err, "Failed to scrape metrics: %v", err) | ||
|
|
||
| assert.Equal(t, test.expectedMetricCount, md.MetricCount()) |
There was a problem hiding this comment.
We should not mix assert and require, especially in the same test. We tend to default to require throughout the receiver so let's continue that trend here.
There was a problem hiding this comment.
Good spot, I have removed all instances of assert and replaced them with require.
| pcommon.NewValueStr(metadata.AttributeStateWait.String())) | ||
| } | ||
|
|
||
| func assertCPUBothPhysicalAndLogicalCountMetricsValid(t *testing.T, metrics pmetric.MetricSlice) { |
There was a problem hiding this comment.
This would pass if you gave it any 2 metrics, as it doesn't check that both system.cpu.physical.count and system.cpu.logical.count are present and none others.
There was a problem hiding this comment.
I removed this function as we now check in the test function for present metrics and that no others are present.
receiver/hostmetricsreceiver/internal/scraper/cpuscraper/cpu_scraper_test.go
Show resolved
Hide resolved
When running in CI we cannot get the frequency value on Linux due to the CPU being virtual
We now use only require instead of assert, refactored test function to make it more clear
braydonk
left a comment
braydonk
left a comment
There was a problem hiding this comment.
Addressed comments from last review all look good, one small comment on the new diff.
| } | ||
|
|
||
| metrics := md.ResourceMetrics().At(0).ScopeMetrics().At(0).Metrics() | ||
| reportedMetrics := make(map[string]int) |
There was a problem hiding this comment.
This section below would make more sense if we didn't already check above that there's only one metric. This is doing a lot of extra ceremony for something that would have already failed the test on line 60. I think this can be simplified with that in mind. I understand it's meant to match the other test in the multiplatform test file but it'd probably be more valuable for this test to be simpler than to look the same.
There was a problem hiding this comment.
Agreed, I have removed the extra validation, I think its simpler now!
|
Thank you for your contribution @osullivandonal! 🎉 We would like to hear from you about your experience contributing to OpenTelemetry by taking a few minutes to fill out this survey. If you are getting started contributing, you can also join the CNCF Slack channel #opentelemetry-new-contributors to ask for guidance and get help. |
4 participants
Description
Improve unit-test code coverage for the
cpuscraperinreceiver/hostmetricsreceiver.Previously the results were:
61.5%make test-with-cover | grep cpuscraper ✓ internal/scraper/cpuscraper (1.058s) (coverage: 61.5% of statements) ✓ internal/scraper/cpuscraper/ucal (1.067s) (coverage: 100.0% of statements) ✓ internal/scraper/cpuscraper/internal/metadata (1.08s) (coverage: 88.2% of statements)Results now are:
92.3%make test-with-cover | grep cpuscraper ✓ internal/scraper/cpuscraper (1.058s) (coverage: 92.3% of statements) ✓ internal/scraper/cpuscraper/ucal (1.071s) (coverage: 100.0% of statements) ✓ internal/scraper/cpuscraper/internal/metadata (1.072s) (coverage: 88.2% of statements)Link to tracking issue
Issue -> #44936
This PR shouldn't close the issue, another PR is need to finish the documentation.
Testing
Tests added include:
TestScrape_CpuCountinreceiver/hostmetricsreciever/internal/scraper/cpuscraper/cpu_scraper_test.goPhysical CPUVirtual CPUTestScrape_CpuFrequencyinreceiver/hostmetricsreceiver/internal/scraper/cpuscraper/cpu_scraper_linux_test.goFrequency, this is only available on Linux.