Copilot commented on code in PR #20305:
URL: https://github.com/apache/druid/pull/20305#discussion_r3965943024
##########
processing/src/test/java/org/apache/druid/segment/MergingRowIteratorTest.java:
##########
@@ -117,13 +134,13 @@ private static void testMerge(List<Long>...
timestampSequences)
expectedTimestamps.add(expectedTimestampIterator.next());
}
for (int markIteration = 0; markIteration < totalLength; markIteration++) {
- testMerge(message, markIteration, expectedTimestamps,
timestampSequences);
+ testMerge(failureMessage, markIteration, expectedTimestamps,
timestampSequences);
Review Comment:
The lazy failure message currently only prints the input sequences, which
makes it harder to diagnose which `markIteration` (or which phase of the loop)
failed. Consider enriching the message supplier used in `testMerge(...)` to
include `markIteration` (and optionally the current output index / expected
timestamp) so failures remain actionable without re-running under a debugger.
##########
processing/src/test/java/org/apache/druid/segment/MergingRowIteratorTest.java:
##########
@@ -104,8 +120,9 @@ private static void populateSequences(
@SafeVarargs
private static void testMerge(List<Long>... timestampSequences)
{
- String message =
Stream.of(timestampSequences).map(List::toString).collect(Collectors.joining("
"));
- int totalLength = Stream.of(timestampSequences).mapToInt(List::size).sum();
+ final Supplier<String> failureMessage
+ = () ->
Stream.of(timestampSequences).map(List::toString).collect(Collectors.joining("
"));
+ final int totalLength =
Stream.of(timestampSequences).mapToInt(List::size).sum();
Review Comment:
The lazy failure message currently only prints the input sequences, which
makes it harder to diagnose which `markIteration` (or which phase of the loop)
failed. Consider enriching the message supplier used in `testMerge(...)` to
include `markIteration` (and optionally the current output index / expected
timestamp) so failures remain actionable without re-running under a debugger.
##########
processing/src/test/java/org/apache/druid/segment/MergingRowIteratorTest.java:
##########
@@ -156,13 +177,47 @@ private static void testMerge(
}
i++;
}
- Assertions.assertFalse(mergingRowIterator.moveToNext(), message);
+ Assertions.assertFalse(mergingRowIterator.moveToNext(), failureMessage);
if (iterated) {
- Assertions.assertEquals(currentTimestamp,
mergingRowIterator.getPointer().timestampSelector.getLong(), message);
+ Assertions.assertEquals(
+ currentTimestamp,
+ mergingRowIterator.getPointer().timestampSelector.getLong(),
+ failureMessage
+ );
}
}
}
+ @SafeVarargs
+ private static void testMergeOrder(List<Long>... timestampSequences)
+ {
+ final Supplier<String> failureMessage
+ = () ->
Stream.of(timestampSequences).map(List::toString).collect(Collectors.joining("
"));
+ try (MergingRowIterator mergingRowIterator = new MergingRowIterator(
+
Stream.of(timestampSequences).map(TestRowIterator::new).collect(Collectors.toList())
+ )) {
+ final Iterator<Long> expectedTimestamps = Utils.mergeSorted(
+
Stream.of(timestampSequences).map(List::iterator).collect(Collectors.toList()),
+ Comparator.naturalOrder()
+ );
+ while (expectedTimestamps.hasNext()) {
+ Assertions.assertTrue(
+ mergingRowIterator.moveToNext(),
+ failureMessage
+ );
+ Assertions.assertEquals(
+ expectedTimestamps.next(),
Review Comment:
Calling `expectedTimestamps.next()` inside the assertion introduces side
effects in the assertion arguments, which can make failures harder to reason
about and debug. Prefer assigning the expected value to a local variable before
the assertion, then asserting on that variable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
