anoopj commented on code in PR #16408:
URL: https://github.com/apache/iceberg/pull/16408#discussion_r3337904549
##########
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:
I started with separate tests, then refactored to reduce the duplicate code.
Honestly it is a wash, given that we are only doing a small amount of
validation. Fixed, to have separate test case and it's a bit more readable.
--
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]
