Copilot commented on code in PR #716:
URL: https://github.com/apache/fesod/pull/716#discussion_r2596131106


##########
fesod/src/test/java/org/apache/fesod/sheet/testkit/util/TestFileUtil.java:
##########
@@ -0,0 +1,126 @@
+/*
+ * 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.fesod.sheet.testkit.util;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import org.apache.fesod.sheet.testkit.enums.ExcelFormat;
+
+/**
+ * Enhanced test file utilities with format-aware methods.
+ * Extends the existing TestFileUtil functionality.
+ */
+public class TestFileUtil {
+
+    /**
+     * Creates a new temporary file with the given name and format extension.
+     */
+    public static File createTempFile(String name, ExcelFormat format) {
+        String fileName = name + format.getExtension();
+        File tempFile = new File(System.getProperty("java.io.tmpdir"), 
fileName);
+        // Make sure file doesn't exist
+        if (tempFile.exists()) {
+            tempFile.delete();
+        }
+        return tempFile;
+    }
+
+    /**
+     * Creates a new temporary file with unique name for the specified format.
+     */
+    public static File createUniqueTempFile(ExcelFormat format) {
+        return createTempFile(
+                "test_" + System.currentTimeMillis() + "_"
+                        + Thread.currentThread().getId(),
+                format);
+    }
+
+    /**
+     * Creates a new temporary file with unique name and specified base name 
for the format.
+     */
+    public static File createUniqueTempFile(String baseName, ExcelFormat 
format) {
+        return createTempFile(
+                baseName + "_" + System.currentTimeMillis() + "_"
+                        + Thread.currentThread().getId(),
+                format);
+    }
+
+    /**
+     * Checks if a file exists and is readable.
+     */
+    public static boolean isFileReadable(File file) {
+        return file != null && file.exists() && file.canRead() && 
file.length() > 0;
+    }
+
+    /**
+     * Gets the format of a file based on its extension.
+     */
+    public static ExcelFormat getFileFormat(File file) {
+        String fileName = file.getName().toLowerCase();
+        if (fileName.endsWith(".xlsx")) {
+            return ExcelFormat.XLSX;
+        } else if (fileName.endsWith(".xls")) {
+            return ExcelFormat.XLS;
+        } else if (fileName.endsWith(".csv")) {
+            return ExcelFormat.CSV;
+        } else {
+            throw new IllegalArgumentException("Unknown file format: " + 
fileName);
+        }
+    }
+
+    /**
+     * Copies a file resource to a temporary file.
+     */
+    public static File copyResourceToFile(String resourceName, File 
destination) throws IOException {
+        try (InputStream inputStream = 
TestFileUtil.class.getClassLoader().getResourceAsStream(resourceName)) {
+            if (inputStream == null) {
+                throw new IOException("Resource not found: " + resourceName);
+            }
+
+            java.nio.file.Files.copy(inputStream, destination.toPath());
+            return destination;
+        }
+    }

Review Comment:
   The method `copyResourceToFile` does not replace the destination file if it 
already exists. This can cause test failures or unexpected behavior when the 
same test is run multiple times. 
   
   Consider adding:
   ```java
   if (destination.exists()) {
       destination.delete();
   }
   ```
   before line 99, or use `Files.copy` with 
`StandardCopyOption.REPLACE_EXISTING`.



##########
fesod/src/test/java/org/apache/fesod/sheet/testkit/util/TestFileUtil.java:
##########
@@ -0,0 +1,126 @@
+/*
+ * 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.fesod.sheet.testkit.util;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import org.apache.fesod.sheet.testkit.enums.ExcelFormat;
+
+/**
+ * Enhanced test file utilities with format-aware methods.
+ * Extends the existing TestFileUtil functionality.
+ */
+public class TestFileUtil {
+
+    /**
+     * Creates a new temporary file with the given name and format extension.
+     */
+    public static File createTempFile(String name, ExcelFormat format) {
+        String fileName = name + format.getExtension();
+        File tempFile = new File(System.getProperty("java.io.tmpdir"), 
fileName);
+        // Make sure file doesn't exist
+        if (tempFile.exists()) {
+            tempFile.delete();
+        }
+        return tempFile;
+    }
+
+    /**
+     * Creates a new temporary file with unique name for the specified format.
+     */
+    public static File createUniqueTempFile(ExcelFormat format) {
+        return createTempFile(
+                "test_" + System.currentTimeMillis() + "_"
+                        + Thread.currentThread().getId(),
+                format);
+    }
+
+    /**
+     * Creates a new temporary file with unique name and specified base name 
for the format.
+     */
+    public static File createUniqueTempFile(String baseName, ExcelFormat 
format) {
+        return createTempFile(
+                baseName + "_" + System.currentTimeMillis() + "_"
+                        + Thread.currentThread().getId(),
+                format);
+    }
+
+    /**
+     * Checks if a file exists and is readable.
+     */
+    public static boolean isFileReadable(File file) {
+        return file != null && file.exists() && file.canRead() && 
file.length() > 0;
+    }
+
+    /**
+     * Gets the format of a file based on its extension.
+     */
+    public static ExcelFormat getFileFormat(File file) {
+        String fileName = file.getName().toLowerCase();
+        if (fileName.endsWith(".xlsx")) {
+            return ExcelFormat.XLSX;
+        } else if (fileName.endsWith(".xls")) {
+            return ExcelFormat.XLS;
+        } else if (fileName.endsWith(".csv")) {
+            return ExcelFormat.CSV;
+        } else {
+            throw new IllegalArgumentException("Unknown file format: " + 
fileName);
+        }
+    }
+
+    /**
+     * Copies a file resource to a temporary file.
+     */
+    public static File copyResourceToFile(String resourceName, File 
destination) throws IOException {
+        try (InputStream inputStream = 
TestFileUtil.class.getClassLoader().getResourceAsStream(resourceName)) {
+            if (inputStream == null) {
+                throw new IOException("Resource not found: " + resourceName);
+            }
+
+            java.nio.file.Files.copy(inputStream, destination.toPath());
+            return destination;
+        }
+    }
+
+    /**
+     * Cleans up temporary files created during testing.
+     */
+    public static void cleanupTempFile(File file) {
+        if (file != null && file.exists()) {
+            file.delete();

Review Comment:
   [nitpick] The `cleanupTempFile` method silently ignores deletion failures. 
If file deletion fails (e.g., due to file locks or permission issues), the test 
may leave behind temporary files. Consider logging a warning or at least 
checking the return value:
   
   ```java
   public static void cleanupTempFile(File file) {
       if (file != null && file.exists()) {
           boolean deleted = file.delete();
           if (!deleted) {
               // Log warning or throw exception
           }
       }
   }
   ```
   
   This is particularly important for cleanup in `@AfterEach` methods where 
silent failures can accumulate temp files over time.
   ```suggestion
               boolean deleted = file.delete();
               if (!deleted) {
                   System.err.println("Warning: Failed to delete temporary 
file: " + file.getAbsolutePath());
               }
   ```



##########
fesod/src/test/java/org/apache/fesod/sheet/testkit/data/RandomDataGenerator.java:
##########
@@ -0,0 +1,136 @@
+/*
+ * 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.fesod.sheet.testkit.data;
+
+import java.math.BigDecimal;
+import java.math.BigInteger;
+import java.time.LocalDate;
+import java.time.LocalDateTime;
+import java.util.Date;
+import java.util.Random;
+import org.apache.fesod.sheet.model.ComplexData;
+import org.apache.fesod.sheet.model.SimpleData;
+
+/**
+ * Generates random test data for fuzz testing and edge case validation.
+ * Helps identify issues with unexpected input values.
+ */
+public class RandomDataGenerator {
+
+    private static final Random RANDOM = new Random();

Review Comment:
   [nitpick] The `Random` instance is shared across all calls without seeding, 
which can make tests non-reproducible. For test infrastructure, it's a best 
practice to either:
   1. Allow setting a seed for reproducible random data in tests
   2. Use `ThreadLocalRandom` instead of a shared `Random` instance
   
   Consider adding:
   ```java
   private static final Random RANDOM = new Random(/* configurable seed */);
   ```
   or providing a method to set the seed for test reproducibility.
   ```suggestion
       private static Random RANDOM = new Random();
       /**
        * Sets the seed for the random number generator to make tests 
reproducible.
        */
       public static void setSeed(long seed) {
           RANDOM = new Random(seed);
       }
   ```



##########
fesod/src/test/java/org/apache/fesod/sheet/model/ConverterData.java:
##########
@@ -0,0 +1,125 @@
+/*
+ * 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.fesod.sheet.model;
+
+import java.math.BigDecimal;
+import java.math.BigInteger;
+import java.time.LocalDate;
+import java.time.LocalDateTime;
+import java.util.Date;
+import lombok.EqualsAndHashCode;
+import lombok.Getter;
+import lombok.Setter;
+import lombok.ToString;
+import org.apache.fesod.sheet.annotation.ExcelProperty;
+import org.apache.fesod.sheet.metadata.data.CellData;
+import org.apache.fesod.sheet.metadata.data.ReadCellData;
+import org.apache.fesod.sheet.metadata.data.WriteCellData;
+import org.apache.fesod.sheet.util.TestUtil;
+
+/**
+ * Unified data model for converter tests.
+ * Consolidates the previous ConverterReadData and ConverterWriteData classes.
+ *
+ * <p>This class uses a generic CellData type to support both read and write 
operations.
+ * For write operations, use {@link WriteCellData}; for read operations, the 
framework
+ * will populate {@link ReadCellData}.</p>
+ *
+ * @see org.apache.fesod.sheet.converter.ConverterReadData (deprecated)
+ * @see org.apache.fesod.sheet.converter.ConverterWriteData (deprecated)
+ */
+@Getter
+@Setter
+@EqualsAndHashCode(exclude = {"cellData"}) // Exclude cellData to avoid 
StackOverflow in hashCode

Review Comment:
   The `EqualsAndHashCode` annotation excludes the `cellData` field with the 
reason "Exclude cellData to avoid StackOverflow in hashCode". This indicates a 
design issue with circular references in the `CellData` type. While the 
exclusion is documented, it means that two `ConverterData` objects with 
different `cellData` values will be considered equal.
   
   This could lead to unexpected behavior in collections (e.g., `Set`, `Map`) 
or when comparing test results. Consider:
   1. Documenting this limitation more prominently in the class-level Javadoc
   2. Adding a warning in test methods that rely on equality checks
   3. Investigating if `CellData` can be redesigned to avoid circular references



##########
fesod/src/test/java/org/apache/fesod/sheet/model/ImageData.java:
##########
@@ -0,0 +1,70 @@
+/*
+ * 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.fesod.sheet.model;
+
+import java.io.File;
+import java.io.InputStream;
+import lombok.EqualsAndHashCode;
+import lombok.Getter;
+import lombok.Setter;
+import org.apache.fesod.sheet.annotation.ExcelProperty;
+import org.apache.fesod.sheet.annotation.write.style.ColumnWidth;
+import org.apache.fesod.sheet.annotation.write.style.ContentRowHeight;
+import org.apache.fesod.sheet.converters.string.StringImageConverter;
+
+/**
+ * Data model for image write tests.
+ * Images can be specified as File, InputStream, String (path), or byte array.
+ *
+ * <p>Note: Image data can only be written to Excel formats (XLSX, XLS),
+ * not CSV. The InputStream should be closed after writing.</p>
+ *
+ * @see org.apache.fesod.sheet.converter.ImageData (deprecated)
+ */
+@Getter
+@Setter
+@EqualsAndHashCode(exclude = {"inputStream"}) // Exclude InputStream to avoid 
hashCode issues
+@ContentRowHeight(500)
+@ColumnWidth(500 / 8)
+public class ImageData {
+
+    /**
+     * Image as a File reference.
+     */
+    private File file;
+
+    /**
+     * Image as an InputStream.
+     * Note: The caller is responsible for closing this stream.
+     */
+    private InputStream inputStream;
+

Review Comment:
   The `EqualsAndHashCode` annotation excludes `inputStream` from equality 
checks, but the Javadoc states "The caller is responsible for closing this 
stream." This creates a potential resource leak because there's no indication 
in the class design that the stream should be closed. Consider:
   
   1. Adding a `@PreDestroy` or cleanup method
   2. Making the field transient if it's not meant to be persisted
   3. Documenting more prominently that users must close the stream after 
writing
   
   The current design could lead to resource leaks if users don't realize they 
need to close the stream.



##########
fesod/src/test/java/org/apache/fesod/sheet/testkit/util/TestFileUtil.java:
##########
@@ -0,0 +1,126 @@
+/*
+ * 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.fesod.sheet.testkit.util;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import org.apache.fesod.sheet.testkit.enums.ExcelFormat;
+
+/**
+ * Enhanced test file utilities with format-aware methods.
+ * Extends the existing TestFileUtil functionality.
+ */
+public class TestFileUtil {
+
+    /**
+     * Creates a new temporary file with the given name and format extension.
+     */
+    public static File createTempFile(String name, ExcelFormat format) {
+        String fileName = name + format.getExtension();
+        File tempFile = new File(System.getProperty("java.io.tmpdir"), 
fileName);
+        // Make sure file doesn't exist
+        if (tempFile.exists()) {
+            tempFile.delete();
+        }
+        return tempFile;
+    }
+
+    /**
+     * Creates a new temporary file with unique name for the specified format.
+     */
+    public static File createUniqueTempFile(ExcelFormat format) {
+        return createTempFile(
+                "test_" + System.currentTimeMillis() + "_"
+                        + Thread.currentThread().getId(),
+                format);
+    }
+
+    /**
+     * Creates a new temporary file with unique name and specified base name 
for the format.
+     */
+    public static File createUniqueTempFile(String baseName, ExcelFormat 
format) {
+        return createTempFile(
+                baseName + "_" + System.currentTimeMillis() + "_"
+                        + Thread.currentThread().getId(),
+                format);
+    }
+
+    /**
+     * Checks if a file exists and is readable.
+     */
+    public static boolean isFileReadable(File file) {
+        return file != null && file.exists() && file.canRead() && 
file.length() > 0;
+    }
+
+    /**
+     * Gets the format of a file based on its extension.
+     */
+    public static ExcelFormat getFileFormat(File file) {
+        String fileName = file.getName().toLowerCase();
+        if (fileName.endsWith(".xlsx")) {
+            return ExcelFormat.XLSX;
+        } else if (fileName.endsWith(".xls")) {
+            return ExcelFormat.XLS;
+        } else if (fileName.endsWith(".csv")) {
+            return ExcelFormat.CSV;
+        } else {
+            throw new IllegalArgumentException("Unknown file format: " + 
fileName);
+        }
+    }
+
+    /**
+     * Copies a file resource to a temporary file.
+     */
+    public static File copyResourceToFile(String resourceName, File 
destination) throws IOException {
+        try (InputStream inputStream = 
TestFileUtil.class.getClassLoader().getResourceAsStream(resourceName)) {
+            if (inputStream == null) {
+                throw new IOException("Resource not found: " + resourceName);
+            }
+
+            java.nio.file.Files.copy(inputStream, destination.toPath());
+            return destination;
+        }
+    }
+
+    /**
+     * Cleans up temporary files created during testing.
+     */
+    public static void cleanupTempFile(File file) {
+        if (file != null && file.exists()) {
+            file.delete();
+        }
+    }
+
+    /**
+     * Reads file contents as a byte array.
+     */
+    public static byte[] readFileAsBytes(File file) throws IOException {
+        try (FileInputStream fis = new FileInputStream(file)) {
+            byte[] bytes = new byte[(int) file.length()];
+            int read = fis.read(bytes);
+            if (read != bytes.length) {
+                throw new IOException("Could not read complete file");
+            }
+            return bytes;
+        }

Review Comment:
   The `readFileAsBytes` method does not handle the case where 
`fis.read(bytes)` might not read the entire file in one call. While this is 
rare for files smaller than the buffer, it can happen with certain filesystems 
or network-mounted drives.
   
   Consider using a more robust approach:
   ```java
   return Files.readAllBytes(file.toPath());
   ```
   This is simpler and handles partial reads correctly.
   ```suggestion
           return java.nio.file.Files.readAllBytes(file.toPath());
   ```



##########
fesod/src/test/java/org/apache/fesod/sheet/testkit/util/TestFileUtil.java:
##########
@@ -0,0 +1,126 @@
+/*
+ * 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.fesod.sheet.testkit.util;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import org.apache.fesod.sheet.testkit.enums.ExcelFormat;
+
+/**
+ * Enhanced test file utilities with format-aware methods.
+ * Extends the existing TestFileUtil functionality.
+ */
+public class TestFileUtil {
+
+    /**
+     * Creates a new temporary file with the given name and format extension.
+     */
+    public static File createTempFile(String name, ExcelFormat format) {
+        String fileName = name + format.getExtension();
+        File tempFile = new File(System.getProperty("java.io.tmpdir"), 
fileName);
+        // Make sure file doesn't exist
+        if (tempFile.exists()) {
+            tempFile.delete();

Review Comment:
   The `createTempFile` method deletes an existing file without checking if the 
deletion succeeded. If deletion fails, the method returns a File object that 
points to an existing file, which could cause test failures or unexpected 
behavior.
   
   Consider checking the deletion result:
   ```java
   if (tempFile.exists()) {
       boolean deleted = tempFile.delete();
       if (!deleted) {
           throw new IOException("Failed to delete existing temp file: " + 
tempFile);
       }
   }
   ```
   ```suggestion
               boolean deleted = tempFile.delete();
               if (!deleted) {
                   throw new IOException("Failed to delete existing temp file: 
" + tempFile);
               }
   ```



##########
fesod/src/test/java/org/apache/fesod/sheet/integration/read/SimpleReadWriteTest.java:
##########
@@ -0,0 +1,281 @@
+/*
+ * 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.fesod.sheet.integration.read;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileOutputStream;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Map;
+import org.apache.fesod.sheet.FesodSheet;
+import org.apache.fesod.sheet.read.listener.PageReadListener;
+import org.apache.fesod.sheet.simple.SimpleData;
+import org.apache.fesod.sheet.testkit.base.AbstractExcelTest;
+import org.apache.fesod.sheet.testkit.enums.ExcelFormat;
+import org.apache.fesod.sheet.testkit.listeners.CollectingReadListener;
+import org.apache.fesod.sheet.util.TestFileUtil;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.Nested;
+import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.MethodSource;
+
+/**
+ * Tests for simple read/write operations across all Excel formats.
+ *
+ * <p>This test class validates basic Excel read/write functionality 
including:</p>
+ * <ul>
+ *   <li>File-based read/write operations</li>
+ *   <li>Stream-based read/write operations</li>
+ *   <li>Synchronous reading</li>
+ *   <li>Sheet name/number navigation</li>
+ *   <li>Paginated reading</li>
+ * </ul>
+ *
+ * <p>Refactored from legacy {@code SimpleDataTest} to use parameterized tests
+ * with format providers from {@link AbstractExcelTest}.</p>
+ *
+ * @see SimpleData
+ * @see AbstractExcelTest
+ */
+@Tag("unit")

Review Comment:
   [nitpick] The test is tagged with `@Tag("unit")` but it performs I/O 
operations (file read/write), which typically classifies it as an integration 
test. According to common testing conventions:
   - **Unit tests**: Test individual components in isolation, no I/O
   - **Integration tests**: Test components working together, may involve I/O
   
   Consider changing to `@Tag("integration")` to accurately reflect the test 
type. This helps with test categorization and selective test execution in CI/CD 
pipelines.
   ```suggestion
   @Tag("integration")
   ```



##########
fesod/src/test/java/org/apache/fesod/sheet/integration/read/SimpleReadWriteTest.java:
##########
@@ -0,0 +1,281 @@
+/*
+ * 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.fesod.sheet.integration.read;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileOutputStream;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Map;
+import org.apache.fesod.sheet.FesodSheet;
+import org.apache.fesod.sheet.read.listener.PageReadListener;
+import org.apache.fesod.sheet.simple.SimpleData;

Review Comment:
   The import statement references the old `SimpleData` class from the `simple` 
package, but this test should use the new consolidated `SimpleData` from the 
`model` package which has additional fields (`number` and `integer`). The old 
`SimpleData` only has the `name` field.
   
   Change the import to:
   ```java
   import org.apache.fesod.sheet.model.SimpleData;
   ```
   
   This is critical because the test creates `SimpleData` instances with more 
than just a name field, which would fail with the old class.
   ```suggestion
   import org.apache.fesod.sheet.model.SimpleData;
   ```



-- 
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]

Reply via email to