Skip to content

Conversation

joshua-adams-1
Copy link
Contributor

Adds tests for the following methods in ESTestCase: randomSubsetOf, randomNonEmptySubsetOf and shuffledList

Relates #126579

@joshua-adams-1 joshua-adams-1 self-assigned this Jul 23, 2025
@joshua-adams-1 joshua-adams-1 added >non-issue :Distributed Coordination/Distributed A catch all label for anything in the Distributed Coordination area. Please avoid if you can. labels Jul 23, 2025
Adds tests for the following methods in `ESTestCase`: `randomSubsetOf`,
`randomNonEmptySubsetOf` and `shuffledList`
@joshua-adams-1 joshua-adams-1 requested a review from ywangd July 23, 2025 11:16
@joshua-adams-1 joshua-adams-1 marked this pull request as ready for review July 23, 2025 11:16
@elasticsearchmachine elasticsearchmachine added the Team:Distributed Coordination Meta label for Distributed Coordination team label Jul 23, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination)

Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

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

I'd rather use more randomness here rather than hard-coding just these specific values.

Comment on lines 178 to 180
List<Integer> result = ESTestCase.randomSubsetOf(2, 1, 2, 3, 4);
assertEquals(2, result.size());
assertTrue(Arrays.asList(1, 2, 3, 4).containsAll(result));
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can use randomness to make some more general statements here. This should hold for all values of 2 (the subset size), so why not choose it randomly? Likewise the source list could be of any length, it doesn't have to be {1,2,3,4}.

Comment on lines 184 to 185
List<Integer> result = ESTestCase.randomSubsetOf(0, 1, 2, 3);
assertEquals(0, result.size());
Copy link
Contributor

Choose a reason for hiding this comment

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

This should hold for all values of 2

In particular, if we pick 2 randomly from a set including 0 then we'd cover this case at the same time.

}

public void testRandomSubsetOfWithVarargsAndSizeTooLarge() {
assertThrows(IllegalArgumentException.class, () -> ESTestCase.randomSubsetOf(5, 1, 2, 3));
Copy link
Contributor

Choose a reason for hiding this comment

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

Likewise this is true for all values of 5 greater than 3.

Copy link
Member

@ywangd ywangd left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -174,11 +174,77 @@ public void testRandomUniqueNormalUsageAlwayMoreThanOne() {
assertThat(randomUnique(() -> randomAlphaOfLengthBetween(1, 20), 10), hasSize(greaterThan(0)));
}

public void testRandomSubsetOfWithVarargs() {
List<Integer> randomList = randomList(10, () -> randomIntBetween(-100, 100));
Copy link
Member

Choose a reason for hiding this comment

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

Nit: We could also randomize the list length of 10. The same goes for other places where we construct the initial list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The 10 parameter is actually the maximum list size, not the hardcoded list size. The randomList function randomises the length between 0 and this value inclusively. I could have randomised this size too, but I figured hardcoding a max list size was acceptable, because even if I randomised it, I would still have to declare a max value somewhere.

Copy link
Member

Choose a reason for hiding this comment

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

Ah right. Sounds good to me.

@joshua-adams-1 joshua-adams-1 merged commit 7485750 into elastic:main Jul 28, 2025
33 checks passed
@joshua-adams-1 joshua-adams-1 deleted the extend-es-test-case branch July 28, 2025 09:11
szybia added a commit to szybia/elasticsearch that referenced this pull request Jul 28, 2025
…-tracking

* upstream/main:
  Fix MergeWithLowDiskSpaceIT testRelocationWhileForceMerging (elastic#131806)
  [ML] Prevent the trained model deployment memory estimation from double-counting allocations. (elastic#131990)
  ES|QL Assert current thread during query planning and execution (elastic#131807)
  Add ElasticsearchIndexDeletionPolicy and EngineConfig policy wrapper (elastic#130442)
  [TEST] Adds tests for ESTestCase randomSubset methods (elastic#131745)
  Simplify esql session (elastic#131925)
  Simplify EsqlExecution info serialization (elastic#131823)
  Add utility to check for project global block (elastic#131927)
  [DOCS] Update ES|QL applies to's (elastic#131805)
  Handle structured log messages (elastic#131027)
  Mute org.elasticsearch.test.rest.yaml.RcsCcsCommonYamlTestSuiteIT test {p0=search/600_flattened_ignore_above/flattened ignore_above multi-value field} elastic#131967
  Mute org.elasticsearch.xpack.remotecluster.CrossClusterEsqlRCS2EnrichUnavailableRemotesIT testEsqlEnrichWithSkipUnavailable elastic#131965
  Mute org.elasticsearch.xpack.restart.FullClusterRestartIT testWatcherWithApiKey {cluster=UPGRADED} elastic#131964
  [ES|QL] Fix aggregate_metric_double sorting and mv_expand issues (elastic#131658)
  Reduce logging levels for meter usage tests (elastic#131935)
afoucret pushed a commit to afoucret/elasticsearch that referenced this pull request Jul 28, 2025
[TEST] Adds tests for ESTestCase randomSubset methods

Adds tests for the following methods in `ESTestCase`: `randomSubsetOf`,
`randomNonEmptySubsetOf` and `shuffledList`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed Coordination/Distributed A catch all label for anything in the Distributed Coordination area. Please avoid if you can. >non-issue Team:Distributed Coordination Meta label for Distributed Coordination team v9.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants