rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1958670627
##########
api/src/test/java/org/apache/iceberg/types/TestReadabilityChecks.java:
##########
@@ -112,6 +114,34 @@ private void testDisallowPrimitiveToStruct(PrimitiveType
from, Schema fromSchema
.contains("cannot be read as a struct");
}
+ @Test
+ public void testVariantType() {
+ Schema fromSchema = new Schema(required(1, "from_field",
Types.VariantType.get()));
+ List<String> errors =
+ CheckCompatibility.writeCompatibilityErrors(
+ new Schema(required(1, "to_field", Types.VariantType.get())),
fromSchema);
+ assertThat(errors).as("Should produce 0 error messages").isEmpty();
Review Comment:
This combines two different test cases that I think should be separate. The
first is this check that variant is compatible with itself. The second is the
case of all other types being written to or read as variant.
There are two problems with combining separate test cases in one method.
* For one, things that are tested first can hide other issues that are
tested later. That can lead to frustrating development were you keep squashing
test bugs, but there are more (different) errors coming from the same method.
It's better to split these so that all errors are visible at the same time.
That way when tests are failing you can tackle the simplest cases first and
usually fix problems before getting to the complicated cases. (A good example
of this is the set of cases in `DataTest`, where tests get more complex:
`testArray`, `testArrayOfStructs`, `testMap`, `testNumericMapKey`, ...
`testMixedTypes`.)
* The second issue is that packing cases into one method can't take
advantage of newer test framework features, like `@ParameterizedTest` in JUnit5
that would work well for the incompatible types here.
--
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]
