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

Fix failing StatsSummaryTests.testEqualsAndHashCode tests. #115919

Conversation

afoucret
Copy link
Contributor

@afoucret afoucret commented Oct 30, 2024

Closes #112439

Closes #112511

Description

The failure was due to a high probability (approximately every 400 runs) of adding 0 new items to both accumulators in the following code:

randomDoubles(randomIntBetween(0, 20)).forEach(stats1);
randomDoubles(randomIntBetween(0, 20)).forEach(stats2);

This led to both accumulators being equal when they were not expected to be.

@afoucret afoucret added >test-failure Triaged test failures from CI auto-backport Automatically create backport pull requests when merged Team:Search Relevance Meta label for the Search Relevance team in Elasticsearch v9.0.0 v8.17.0 labels Oct 30, 2024
@afoucret afoucret requested a review from a team October 30, 2024 09:49
@elasticsearchmachine elasticsearchmachine added needs:triage Requires assignment of a team area label needs:risk Requires assignment of a risk label (low, medium, blocker) and removed Team:Search Relevance Meta label for the Search Relevance team in Elasticsearch labels Oct 30, 2024
@afoucret afoucret added :Search/Search Search-related issues that do not fall into other categories Team:Search Relevance Meta label for the Search Relevance team in Elasticsearch labels Oct 30, 2024
@elasticsearchmachine elasticsearchmachine added Team:Search Meta label for search team and removed needs:triage Requires assignment of a team area label Team:Search Relevance Meta label for the Search Relevance team in Elasticsearch labels Oct 30, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-search (Team:Search)

randomDoubles(randomIntBetween(0, 20)).forEach(stats1);
randomDoubles(randomIntBetween(0, 20)).forEach(stats2);
randomDoubles(randomIntBetween(1, 1000)).forEach(stats1);
randomDoubles(randomIntBetween(1, 1000)).forEach(stats2);
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to even keep the random ranges non-overlapping in this case? Or is this overkill because picking the same amount of doubles and the same values is so unlikely?

Copy link
Contributor Author

@afoucret afoucret Oct 30, 2024

Choose a reason for hiding this comment

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

My assumption was indeed that the probability of having an equality with such a combination is lower than winning to the lottery twice the same month ....

Also, I did run the tests with -D tests.iters=1000000 to check the randomization

@afoucret afoucret enabled auto-merge (squash) October 30, 2024 10:04
@afoucret
Copy link
Contributor Author

Closed in favor of #115922

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Automatically create backport pull requests when merged needs:risk Requires assignment of a risk label (low, medium, blocker) :Search/Search Search-related issues that do not fall into other categories Team:Search Meta label for search team >test-failure Triaged test failures from CI
Projects
None yet
4 participants