RussellSpitzer commented on code in PR #11947: URL: https://github.com/apache/iceberg/pull/11947#discussion_r1917469252
########## 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: That's actually why I prefer the builder, I believe the tests are much more readable without specifying all of the parameters each time and we would still force all tests to have to deal with new parameters if they all use the builder that defaults to an example fully-non-null metadata. Previously, the worst case scenario of this approach (having to set every single property) was required for every single test. First, several local variables are created and set to arbitrary default values, seldomly these values are relevant to the test, then we call a constructor which has many arguments mostly null or empty. For me this made every test a bit of a hunt and check, which args are being set explicitly because of this test and which just need to be non-null so that the test doesn't break? Previously, I had no idea what some tests were testing or if they were correct without IDE hints on which arg was which. For example, the test for UUID being null sets 3/ 30~ args to null and sets specific values to 13 of the args. Of all of these values only 1 of those nulls actually matters to the test and everything else is essentially noise. If we do want to stick with using the full constructor everywhere I think that's fine but if our rational is that we don't want to force these tests to be able to ignore constructor changes then I think we need to never have secondary constructors for TableMetadata. I know adding more constructors is a a common review comment when we add new args and I think we should just articulate that we won't do that for these key constructors in Metadata and BaseSnapshot. -- 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]
