alhudz commented on code in PR #1808:
URL: https://github.com/apache/commons-lang/pull/1808#discussion_r4153477016
##########
src/test/java/org/apache/commons/lang3/RandomStringUtilsTest.java:
##########
@@ -746,6 +746,42 @@ void testRandomAscii(final RandomStringUtils rsu) {
}
}
+ /**
+ * The POSIX {@code [:graph:]} class runs up to and including {@code '~'}
(0x7E), so {@link RandomStringUtils#nextGraph(int)} must be able to
+ * generate it. This test will fail randomly with negligible probability
{@code (1 - (1 - (93/94)^10))^1000}.
Review Comment:
@garydgregory Reviewed both of them.
The low one is fair, so the formula is gone: the Javadoc now refers to
`LOOP_COUNT` and `BOUNDARY_SAMPLE_LENGTH` rather than hard-coding `1000`.
One thing about the version in 51eb3eb though: a pure range check passes
with `end` at `126` or `127`, so it no longer catches the off-by-one, and
`testRandomGraphRange`/`testRandomPrintRange` already cover
`\p{Graph}`/`\p{Print}` membership. I've folded both halves into one helper so
each test asserts the whole documented contract: every character within the
inclusive range, and both ends of it actually generated (`'!'` and `'~'` for
`[:graph:]`, space and `'~'` for `[:print:]`). That mirrors `testRandomAscii`,
which does the same for 0x20 and 0x7E on the sibling method.
`expected:` `'~'` reachable, nothing above 0x7E
`actual, end 126:` 6/6 fail with `character not generated in 1000 attempts:
126`
`actual, end 127:` `RandomStringUtilsTest` 94 tests, 0 failures; full `mvn
test` 89369 tests, 0 failures; `checkstyle:check` clean
I also checked the Commons Text side as you've asked on earlier PRs:
`RandomStringGenerator` takes its bounds from the caller as inclusive and does
`maxInclusive - minInclusive + 1`, so there's no equivalent hard-coded boundary
to be off by one there.
##########
src/test/java/org/apache/commons/lang3/RandomStringUtilsTest.java:
##########
@@ -746,6 +746,42 @@ void testRandomAscii(final RandomStringUtils rsu) {
}
}
+ /**
+ * The POSIX {@code [:graph:]} class runs up to and including {@code '~'}
(0x7E), so {@link RandomStringUtils#nextGraph(int)} must be able to
+ * generate it. This test will fail randomly with negligible probability
{@code (1 - (1 - (93/94)^10))^1000}.
+ *
+ * @param rsu The instance to test
+ */
+ @ParameterizedTest
+ @MethodSource("randomProvider")
+ void testRandomGraphIncludesTilde(final RandomStringUtils rsu) {
+ boolean found = false;
+ for (int i = 0; i < LOOP_COUNT && !found; i++) {
+ if (rsu.nextGraph(10).indexOf('~') >= 0) {
+ found = true;
+ }
+ }
+ assertTrue(found, "'~' (0x7E) not generated by nextGraph in " +
LOOP_COUNT + " attempts -- repeated failures indicate a problem");
+ }
+
+ /**
+ * The POSIX {@code [:print:]} class runs up to and including {@code '~'}
(0x7E), so {@link RandomStringUtils#nextPrint(int)} must be able to
+ * generate it. This test will fail randomly with negligible probability
{@code (1 - (1 - (94/95)^10))^1000}.
+ *
+ * @param rsu The instance to test
+ */
+ @ParameterizedTest
+ @MethodSource("randomProvider")
+ void testRandomPrintIncludesTilde(final RandomStringUtils rsu) {
+ boolean found = false;
+ for (int i = 0; i < LOOP_COUNT && !found; i++) {
+ if (rsu.nextPrint(10).indexOf('~') >= 0) {
+ found = true;
+ }
+ }
+ assertTrue(found, "'~' (0x7E) not generated by nextPrint in " +
LOOP_COUNT + " attempts -- repeated failures indicate a problem");
Review Comment:
On the determinism point: the RNG behind `nextGraph`/`nextPrint` can't be
pinned from a test, as both `RandomStringUtils(Supplier<RandomUtils>)` and
`RandomUtils(Supplier<Random>)` are private and neither instance method takes a
`Random`. Where that injection does exist the suite already uses it, see
`testCharOverflow` passing a fixed `Random` to the `next(count, start, end,
..., Random)` overload.
So the boundary half is sampled: `LOOP_COUNT` strings of 100 characters,
i.e. 100k draws from a 94-character alphabet per instance, which puts the miss
probability for a correct generator around `e^-1069`. The in-range half of the
assertion is deterministic.
--
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]
