Skip to content

Conversation

@mkocher
Copy link
Member

@mkocher mkocher commented Jun 14, 2022

This commit changes the Store creation logic in the tests so that the
tests will never have multiple Stores accessing the same Metrics
Registry at the same time.

Prior to this commit, some tests (but most notably the "truncates older
envelopes when max size is reached" test) will -when waiting to data
values out of the Metrics Registry- will sometimes get values for the
wrong Registry.

Signed-off-by: Matthew Kocher [email protected]

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Testing performed?

  • Unit tests
  • Integration tests
  • Acceptance tests

Checklist:

  • This PR is being made against the main branch, or relevant version branch
  • I have made corresponding changes to the documentation
  • I have added testing for my changes

This commit changes the Store creation logic in the tests so that the
tests will never have multiple Stores accessing the same Metrics
Registry at the same time.

Prior to this commit, some tests (but most notably the "truncates older
envelopes when max size is reached" test) will -when waiting to data
values out of the Metrics Registry- will sometimes get values for the
wrong Registry.

Signed-off-by: Matthew Kocher <[email protected]>
Copy link
Member

@ctlong ctlong left a comment

Choose a reason for hiding this comment

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

LGTM thanks!

I ran this suite with go run github.com/onsi/ginkgo/ginkgo -p -untilItFails and saw 32 continuous passes.

@ctlong ctlong merged commit 18e7202 into main Jun 15, 2022
@ctlong ctlong deleted the test-concurrency-error-fix branch June 15, 2022 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants