Test fix: ensure we don't accidentally generate two identical histograms #113322
Conversation
smalyshev
added
>test
|
Pinging @elastic/es-search-foundations (Team:Search Foundations) |
elasticsearchmachine
added
the
Team:Search Foundations
| @@ -33,7 +33,7 @@ public class CCSTelemetrySnapshotTests extends AbstractWireSerializingTestCase<C | |||
|
|
|||
| private LongMetricValue randomLongMetricValue() { | |||
| LongMetric v = new LongMetric(); | |||
| for (int i = 0; i < randomIntBetween(1, 10); i++) { | |||
| for (int i = 0; i < randomIntBetween(5, 10); i++) { | |||
There was a problem hiding this comment.
It's not immediately obvious what issue we're looking to fix and how this change fixes it.
Can you please expand in the PR description the observed problem?
I suspect the problematic usage of this random long method lies in mutateInstance ? (based on the linked issue) If this is correct I think we should use randomValueOtherThan() to make sure mutateInstance generates a value that's different than the input.
edit: linking an example from other tests https://github.com/elastic/elasticsearch/blob/main/server/src/test/java/org/elasticsearch/cluster/metadata/DataStreamTests.java#L133
There was a problem hiding this comment.
The problem seems to be that if it chooses to have one value, the histogram sometimes ends up the same as the previous one. The problem here is that I can't just generate a long different from previous long, for two reasons: 1) I do not know what the previous long has been (I only have a histogram) and 2) even if I did, a histogram can put two different values into the same bucket, thus producing the same histogram. Making it always generate more than one value significantly reduces a chance for randomly generating two identical histograms.
There was a problem hiding this comment.
@andreidan I changed the code to use randomValueOtherThan
There was a problem hiding this comment.
Sorry if I'm misunderstanding the problem, but wouldn't we get different histograms if we radomize away from the current value?
e.g.
case 3:
took = randomValueOtherThan(took, () -> randomLongMetricValue());
break;
case 4:
tookMrtTrue = randomValueOtherThan(tookMrtTrue, () -> randomLongMetricValue());
break;
case 5:
tookMrtFalse = randomValueOtherThan(tookMrtFalse, () -> randomLongMetricValue());
break;
I believe the current proposed solution only changes the window of flakiness (arguably making it less probable to occur, but does not eliminate it i.e. the creation of test instance my get 5 values in a histogram and the mutation might also get 5)
smalyshev
changed the title
|
@elasticmachine update branch |
💚 Backport successful
|
…ams (elastic#113322) * Test fix: looks like using one value is not random enough
Fixes #113264