XComp commented on a change in pull request #17159:
URL: https://github.com/apache/flink/pull/17159#discussion_r703374564
##########
File path:
flink-connectors/flink-connector-pulsar/src/test/java/org/apache/flink/connector/pulsar/source/enumerator/topic/TopicRangeTest.java
##########
@@ -33,26 +30,22 @@
/** Unit tests for {@link TopicRange}. */
class TopicRangeTest {
- private final Random random = new Random(System.currentTimeMillis());
-
- @RepeatedTest(10)
- @SuppressWarnings("java:S5778")
+ @Test
void rangeCreationHaveALimitedScope() {
- assertThrows(
- IllegalArgumentException.class,
- () -> new TopicRange(-1, random.nextInt(MAX_RANGE)));
- assertThrows(
- IllegalArgumentException.class,
- () -> new TopicRange(1, MAX_RANGE + random.nextInt(10000)));
-
- assertDoesNotThrow(() -> new TopicRange(1, random.nextInt(MAX_RANGE)));
+ // Negative start.
+ assertThrows(IllegalArgumentException.class, () -> new TopicRange(-1,
1));
+ // End below the boundary.
+ assertDoesNotThrow(() -> new TopicRange(1, MAX_RANGE - 1));
+ // End on the boundary.
+ assertDoesNotThrow(() -> new TopicRange(1, MAX_RANGE));
+ // End above the boundary.
+ assertThrows(IllegalArgumentException.class, () -> new TopicRange(1,
MAX_RANGE + 1));
}
@Test
void topicRangeIsSerializable() throws Exception {
- TopicRange range = new TopicRange(10, random.nextInt(MAX_RANGE));
- TopicRange cloneRange = InstantiationUtil.clone(range);
-
+ final TopicRange range = new TopicRange(10, 9);
Review comment:
Here, I'm puzzled. It feels like there should be another invariant
checking `start < end`. But that's probably out of the scope of this ticket.
I'm just wondering whether it's smart to create semantically invalid test data
here on purpose or not? 🤔 ...but maybe, in the end it doesn't matter since it
reflects the current state of the code base. ¯\_(ツ)_/¯ Did you add this case on
purpose?
##########
File path:
flink-connectors/flink-connector-pulsar/src/test/java/org/apache/flink/connector/pulsar/source/enumerator/topic/TopicRangeTest.java
##########
@@ -33,26 +30,22 @@
/** Unit tests for {@link TopicRange}. */
class TopicRangeTest {
- private final Random random = new Random(System.currentTimeMillis());
-
- @RepeatedTest(10)
- @SuppressWarnings("java:S5778")
+ @Test
void rangeCreationHaveALimitedScope() {
- assertThrows(
- IllegalArgumentException.class,
- () -> new TopicRange(-1, random.nextInt(MAX_RANGE)));
- assertThrows(
- IllegalArgumentException.class,
- () -> new TopicRange(1, MAX_RANGE + random.nextInt(10000)));
-
- assertDoesNotThrow(() -> new TopicRange(1, random.nextInt(MAX_RANGE)));
+ // Negative start.
Review comment:
I would prefer splitting this test into individual test methods and
using the comments as method names. This way, the actual issue becomes clearer
right away when running the tests.
--
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]
