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.

Sign up for GitHub

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

Jump to bottom

[chore][pkg/stanza] - Fix Flaky test TestMatcher #36640

Merged
merged 1 commit into from
Dec 3, 2024
Merged

[chore][pkg/stanza] - Fix Flaky test TestMatcher #36640

merged 1 commit into from
Dec 3, 2024

Conversation

VihasMakwana
Copy link
Contributor

Fixes #36623

We're using a map to store groups and we then do a for .. range over keys.

opentelemetry-collector-contrib/pkg/stanza/fileconsumer/matcher/matcher.go

Lines 200 to 206 in 6119d51

for _, groupedFiles := range groups {
groupResult, err := filter.Filter(groupedFiles, m.regex, m.filterOpts...)
if len(groupResult) == 0 {
return groupResult, errors.Join(err, errs)
}
result = append(result, groupResult...)
}

For maps, the ordering of keys is not guaranteed. Hence, the check fails.
We should instead check with assert.ElementsMatch (which was previously the case, but #36518 changed it to assert.Equal)

@VihasMakwana VihasMakwana requested review from djaglowski and a team as code owners December 3, 2024 07:45
@VihasMakwana VihasMakwana added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Dec 3, 2024
@VihasMakwana VihasMakwana mentioned this pull request Dec 3, 2024
Closed
evan-bradley
evan-bradley approved these changes Dec 3, 2024
View reviewed changes
Copy link
Contributor

@evan-bradley evan-bradley left a comment

Choose a reason for hiding this comment

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

Nice catch, thanks.

@VihasMakwana VihasMakwana requested a review from odubajDT December 3, 2024 16:07
@VihasMakwana
Copy link
Contributor Author

cc: @odubajDT can you take a look? this is related to your recent work on file groupings.

dmitryax
dmitryax approved these changes Dec 3, 2024
View reviewed changes
Copy link
Member

@dmitryax dmitryax left a comment

Choose a reason for hiding this comment

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

Thanks @VihasMakwana!

@dmitryax dmitryax merged commit 7abf152 into open-telemetry:main Dec 3, 2024
168 of 169 checks passed
@github-actions github-actions bot added this to the next release milestone Dec 3, 2024
shivanthzen pushed a commit to shivanthzen/opentelemetry-collector-contrib that referenced this pull request Dec 5, 2024
@VihasMakwana @shivanthzen
[chore][pkg/stanza] - Fix Flaky test TestMatcher (open-telemetry#36640)
acaee18 
Fixes
open-telemetry#36623

We're using a map to store groups and we then do a `for .. range` over
keys.

https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/6119d51a5a85565c409bb225027d6d1ab353aecc/pkg/stanza/fileconsumer/matcher/matcher.go#L200-L206

For maps, the ordering of keys is not guaranteed. Hence, the check
fails.
We should instead check with `assert.ElementsMatch` (which was
previously the case, but
open-telemetry#36518
changed it to `assert.Equal`)
ZenoCC-Peng pushed a commit to ZenoCC-Peng/opentelemetry-collector-contrib that referenced this pull request Dec 6, 2024
@VihasMakwana @ZenoCC-Peng
[chore][pkg/stanza] - Fix Flaky test TestMatcher (open-telemetry#36640)
132571d 
Fixes
open-telemetry#36623

We're using a map to store groups and we then do a `for .. range` over
keys.

https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/6119d51a5a85565c409bb225027d6d1ab353aecc/pkg/stanza/fileconsumer/matcher/matcher.go#L200-L206

For maps, the ordering of keys is not guaranteed. Hence, the check
fails.
We should instead check with `assert.ElementsMatch` (which was
previously the case, but
open-telemetry#36518
changed it to `assert.Equal`)
sbylica-splunk pushed a commit to sbylica-splunk/opentelemetry-collector-contrib that referenced this pull request Dec 17, 2024
@VihasMakwana @sbylica-splunk
[chore][pkg/stanza] - Fix Flaky test TestMatcher (open-telemetry#36640)
049d0e0 
Fixes
open-telemetry#36623

We're using a map to store groups and we then do a `for .. range` over
keys.

https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/6119d51a5a85565c409bb225027d6d1ab353aecc/pkg/stanza/fileconsumer/matcher/matcher.go#L200-L206

For maps, the ordering of keys is not guaranteed. Hence, the check
fails.
We should instead check with `assert.ElementsMatch` (which was
previously the case, but
open-telemetry#36518
changed it to `assert.Equal`)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adrielp adrielp adrielp approved these changes

@evan-bradley evan-bradley evan-bradley approved these changes

@dmitryax dmitryax dmitryax approved these changes

@djaglowski djaglowski Awaiting requested review from djaglowski djaglowski is a code owner

@odubajDT odubajDT Awaiting requested review from odubajDT

Assignees

@evan-bradley evan-bradley

Labels
pkg/stanza Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Milestone
v0.115.0
Development

Successfully merging this pull request may close these issues.

[pkg/stanza] Flaky test TestMatcher/Numeric_Sorting_with_grouping
4 participants
@VihasMakwana @dmitryax @evan-bradley @adrielp