rdblue commented on code in PR #16408:
URL: https://github.com/apache/iceberg/pull/16408#discussion_r3337143916
##########
core/src/test/java/org/apache/iceberg/TestManifestInfoStruct.java:
##########
@@ -162,82 +193,100 @@ void testJavaSerializationRoundTrip() throws
IOException, ClassNotFoundException
assertThat(deserialized.dvCardinality()).isEqualTo(1L);
}
- @Test
- void testBuilderValidation() {
- assertThatThrownBy(
- () ->
- ManifestInfoStruct.builder()
- .existingFilesCount(0)
- .deletedFilesCount(0)
- .replacedFilesCount(0)
- .addedRowsCount(0L)
- .existingRowsCount(0L)
- .deletedRowsCount(0L)
- .replacedRowsCount(0L)
- .minSequenceNumber(0L)
- .build())
- .isInstanceOf(IllegalArgumentException.class)
- .hasMessage("Invalid added files count: -1 (must be >= 0)");
+ // Keyed by the field name used in the "Missing required value: ..." build()
error so each
+ // case has a single source of truth. No dependency on schema field order.
+ private static final Map<String, Consumer<ManifestInfoStruct.Builder>>
REQUIRED_SETTERS =
+ Map.ofEntries(
+ Map.entry("added files count", b -> b.addedFilesCount(0)),
+ Map.entry("existing files count", b -> b.existingFilesCount(0)),
+ Map.entry("deleted files count", b -> b.deletedFilesCount(0)),
+ Map.entry("replaced files count", b -> b.replacedFilesCount(0)),
+ Map.entry("added rows count", b -> b.addedRowsCount(0L)),
+ Map.entry("existing rows count", b -> b.existingRowsCount(0L)),
+ Map.entry("deleted rows count", b -> b.deletedRowsCount(0L)),
+ Map.entry("replaced rows count", b -> b.replacedRowsCount(0L)),
+ Map.entry("min sequence number", b -> b.minSequenceNumber(0L)));
+
+ private static Stream<String> missingRequiredFieldCases() {
+ return REQUIRED_SETTERS.keySet().stream();
+ }
- assertThatThrownBy(
- () ->
- ManifestInfoStruct.builder()
- .addedFilesCount(0)
- .deletedFilesCount(0)
- .replacedFilesCount(0)
- .addedRowsCount(0L)
- .existingRowsCount(0L)
- .deletedRowsCount(0L)
- .replacedRowsCount(0L)
- .minSequenceNumber(0L)
- .build())
+ @ParameterizedTest
+ @MethodSource("missingRequiredFieldCases")
+ void testBuilderMissingRequiredFields(String missingField) {
+ ManifestInfoStruct.Builder builder = ManifestInfoStruct.builder();
+ REQUIRED_SETTERS.forEach(
+ (name, setter) -> {
+ if (!name.equals(missingField)) {
+ setter.accept(builder);
+ }
+ });
+
+ assertThatThrownBy(builder::build)
.isInstanceOf(IllegalArgumentException.class)
- .hasMessage("Invalid existing files count: -1 (must be >= 0)");
+ .hasMessage("Missing required value: " + missingField);
+ }
- assertThatThrownBy(
- () ->
- ManifestInfoStruct.builder()
- .addedFilesCount(0)
- .existingFilesCount(0)
- .replacedFilesCount(0)
- .addedRowsCount(0L)
- .existingRowsCount(0L)
- .deletedRowsCount(0L)
- .replacedRowsCount(0L)
- .minSequenceNumber(0L)
- .build())
- .isInstanceOf(IllegalArgumentException.class)
- .hasMessage("Invalid deleted files count: -1 (must be >= 0)");
+ private static Stream<Arguments> negativeValueAtSetterCases() {
Review Comment:
This is a lot of extra code and work just to share an `assertThatThrowBy`
function call. It is much more straightforward to just have separate test
cases. For this and the one above. I wouldn't be surprised if you ended up with
a smaller test suite that way, too.
--
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]
