ctubbsii commented on code in PR #3228: URL: https://github.com/apache/accumulo/pull/3228#discussion_r1329167022
########## test/src/main/java/org/apache/accumulo/test/ExportTableMakeMultipleDistcp.java: ########## @@ -0,0 +1,148 @@ +/* + * 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 + * + * https://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.accumulo.test; + +import static java.nio.charset.StandardCharsets.UTF_8; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.io.BufferedWriter; +import java.io.OutputStreamWriter; +import java.util.HashMap; +import java.util.Map; +import java.util.Set; +import java.util.TreeSet; + +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.fs.FileSystem; +import org.apache.hadoop.fs.Path; +import org.junit.Test; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +public class ExportTableMakeMultipleDistcp { Review Comment: I'm not sure what this class is doing. It looks like a unit test class, but does not follow the naming convention of unit test class. It also has a mix of JUnit4 and JUnit5 imports. It doesn't seem to be unit testing the implementation, but seems to have a separate implementation that it's using. My suggestion last week was to have a unit test that tests the appropriate method in WriteExportFiles, and that if necessary, WriteExportFiles might need to be refactored to make it more easy to write a unit test against the file grouping function. This looks like of like that, except it doesn't call any WriteExportFiles methods to test that class' implementation. It just provides a new implementation here, so it's not clear what is being tested. For example, if WriteExportFiles had a method like: ```java public Map<String, Set<String>> groupDataFilesByOutputFile(Set<String> uniqueFiles); ``` Then, it would take a set of input files and group them by output filename. You could write a unit test with a particular set of uniqueFiles, and verify that the output mapping was correct. Alternatively, if it had a mapper function, like: ```java Function<String,String> outputFileMapper; ``` Then, you could write a unit test that verified that for any given input file name, it properly mapped to the expected output file name. (as in, for each uniqueFiles, `assertEquals(expectedOutputFile, outputFileMapper.apply(inputFile));`) A grouping method could use a function like this, and you could either write a unit test against the grouping method or the mapping function, or both. -- 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]
