HonahX commented on code in PR #11947: URL: https://github.com/apache/iceberg/pull/11947#discussion_r1917865349
########## core/src/test/java/org/apache/iceberg/MetadataTestUtils.java: ########## @@ -0,0 +1,336 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.iceberg; + +import static org.apache.iceberg.TableMetadata.INITIAL_SEQUENCE_NUMBER; + +import java.util.List; +import java.util.Map; +import java.util.UUID; +import org.apache.iceberg.relocated.com.google.common.base.Preconditions; +import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList; +import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap; +import org.apache.iceberg.relocated.com.google.common.collect.Lists; +import org.apache.iceberg.relocated.com.google.common.collect.Maps; +import org.apache.iceberg.util.SerializableSupplier; + +public class MetadataTestUtils { + + private MetadataTestUtils() {} + + public static TableMetadataBuilder buildTestTableMetadata(int formatVersion) { + return new TableMetadataBuilder(formatVersion); + } + + public static class TableMetadataBuilder { Review Comment: I think we can approach this from two different perspectives, depending on the purpose of the test. ### Type 1: Comprehensive Tests Covering All or Most Fields These tests ensure that TableMetadata as a whole is correctly processed, typically verifying round-trip serialization, deserialization, and backward compatibility. Examples include: - `testJsonConversion` – Ensures that all fields are correctly handled in serialization and deserialization. - `testBackwardCompatibility` – Verifies that metadata remains compatible across versions. For these tests, I agree that continuing to use the constructor directly makes sense. Since these tests require explicitly setting all fields, any new field will naturally require an update, ensuring that the test remains valid and that new fields are properly incorporated. ### Type 2: Targeted Tests for Specific Features or Edge Cases These tests focus on specific behaviors or individual fields rather than the full metadata structure. For example: - testInvalidMainBranch – Only checks whether currentSnapshotId matches the main branch. - testJsonWithPreviousMetadataLog – Specifically validates how the metadataLog field is handled. For these tests, I think the builder approach provides a readability advantage. It allows us to set only the relevant fields, making it clear what the test is actually focusing on while avoiding unnecessary details. This helps maintain clarity and ensures that unrelated fields don’t introduce noise into the test. In conclusion, introducing the builder can significantly simplify unit tests with a more focused scope, while the constructor remains useful for comprehensive tests. One key benefit of this mixed approach is that comprehensive tests are relatively few, meaning we can still reduce the overall effort required when adding new fields. There are also opportunities to further refine this PR by eliminating unnecessary setters. If we decide to move forward with this approach, I can update the implementation accordingly. Looking forward to your thoughts! -- 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: issues-unsubscr...@iceberg.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For additional commands, e-mail: issues-h...@iceberg.apache.org
