Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[chore][exporter/signalfx] fix tests #31522

Merged
merged 5 commits into from
Mar 1, 2024
Merged

Conversation

jinja2
Copy link
Contributor

@jinja2 jinja2 commented Mar 1, 2024

Description:

Update test cases to ignore the order of resource/scope metrics since the returned histograms-only pmetrics can have different ordering for resource and scope metrics

Link to tracking Issue: Fixes #31493

Testing:

Documentation:

Comment on lines +328 to +330
if tt.wantMetricCount == 0 {
assert.Equal(t, tt.wantMetrics(), gotMetrics)
} else {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say this part is redundant. pmetrictest.CompareMetrics should do the job even if wantMetricCount == 0, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with no histograms, we return uninitialized pmetric.Metrics{} and CompareMetrics does not check for nil

@jinja2
Copy link
Contributor Author

jinja2 commented Mar 1, 2024

ci failure due to #31443

@crobert-1
Copy link
Member

@jinja2 Can you update the description to say Fixes or Resolves before the bug number? This will allow the bug to be automatically closed after this is merged so we don't miss it 👍

Copy link
Member

@crobert-1 crobert-1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing, really appreciate the quick resolution here!

@jinja2 jinja2 marked this pull request as ready for review March 1, 2024 17:14
@jinja2 jinja2 requested a review from a team March 1, 2024 17:14
@songy23 songy23 added the ready to merge Code review completed; ready to merge by maintainers label Mar 1, 2024
@codeboten codeboten merged commit 3401d8a into open-telemetry:main Mar 1, 2024
142 checks passed
@github-actions github-actions bot added this to the next release milestone Mar 1, 2024
XinRanZhAWS pushed a commit to XinRanZhAWS/opentelemetry-collector-contrib that referenced this pull request Mar 13, 2024
Update test cases to ignore the order of resource/scope metrics since
the returned histograms-only pmetrics can have different ordering for
resource and scope metrics

Fixes
open-telemetry#31493
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exporter/signalfx ready to merge Code review completed; ready to merge by maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Flaky Test][exporter/signalfx] internal/utils TestHistogramsAreRetrieved: Actual metrics don't match expected
7 participants