[Test] Add testkit infrastructure and refactor simple/converter tests

[GOODBOY008]

Summary

Add shared test infrastructure (testkit) and migrate simple/ and converter/ test packages to use modern JUnit 5 patterns.

Changes

New Testkit Infrastructure

• AbstractExcelTest - Base class with format providers and temp file management
• ExcelFormat - Enum with XLSX/XLS/CSV capabilities metadata
• CollectingReadListener - Reusable data collector (no assertions in listener)
• ExcelAssertions - Fluent API for Excel file validations

Migrated Tests

• simple/ → SimpleReadWriteTest (20 parameterized tests)
• converter/ → ConverterIntegrationTest (14 tests), ConverterUnitTest (11 tests)

New Dependencies

• AssertJ 3.24.2 for fluent assertions

Related

• Proposal Issue: [Proposal] Refactor Unit Tests with Modern Testing Patterns #717

[bengbengbalabalabeng]

Hi @GOODBOY008 .
I really like the Fluent API design of ExcelAssertions—it makes the test code much cleaner.

Building upon your work, I've added a few enhancements:

1. Added overloads for assertThat(Sheet), assertThat(Row), and assertThat(Cell). This supports starting assertions from any level (not just from the Workbook).
2. Added an and() method to CellAssert, RowAssert, and SheetAssert. This allows navigating back up the chain to verify multiple nodes in a single fluent statement.
3. Added a dedicated RowAssert class to verify properties like row height and cell counts.
4. Added a satisfies(Consumer<T>) method. This allows users to perform custom assertions on the underlying POI objects directly.

Example:

ExcelAssertions.assertThat(file)
                .sheet(1)
                .satisfies(sheet -> {
                    ExcelAssertions.assertThat(sheet).hasRowCount(10);
                })
                .row(1)
                    .cell(0).hasStringValue("Name").and()
                    .cell(1)
                    .satisfies(cell -> {
                        Assertions.assertNotNull(cell.getCellComment(), "Cell should have a comment");
                    }).and()
                .and()
                .row(2)
                    .satisfies(row -> {
                        Assertions.assertTrue(row.getZeroHeight(), "Row 2 should be hidden");
                    }).and()
                .and()
                .hasSheetCount(2);

full code view

import java.io.File;
import java.io.IOException;
import java.util.function.Consumer;

import org.apache.poi.ss.usermodel.*;
import org.junit.jupiter.api.Assertions;

/**
 * Main entry point for Excel-specific assertions.
 * Provides a fluent API for testing Excel file contents.
 *
 * <p>Usage:</p>
 * <pre>{@code
 * ExcelAssertions.assertThat(file)
 *     .sheet(0)
 *     .hasRowCount(10)
 *     .cell(0, 0)
 *     .hasStringValue("Header");
 * }</pre>
 */
public class ExcelAssertions {

    /**
     * Entry point for workbook assertions from a file.
     */
    public static WorkbookAssert assertThat(File file) {
        try {
            Workbook workbook = WorkbookFactory.create(file);
            return new WorkbookAssert(workbook, true);
        } catch (IOException e) {
            throw new AssertionError("Failed to open workbook: " + file, e);
        }
    }

    /**
     * Entry point for workbook assertions from a Workbook object.
     */
    public static WorkbookAssert assertThat(Workbook workbook) {
        return new WorkbookAssert(workbook, false);
    }

    public static SheetAssert assertThat(Sheet sheet) {
        return new SheetAssert(sheet, sheet.getWorkbook());
    }

    public static RowAssert assertThat(Row row) {
        return new RowAssert(row, row.getSheet());
    }

    public static CellAssert assertThat(Cell cell) {
        return new CellAssert(cell, cell.getSheet());
    }

    /**
     * Workbook-level assertions.
     */
    public static class WorkbookAssert implements AutoCloseable {
        protected final Workbook workbook;
        private final boolean shouldClose;

        public WorkbookAssert(Workbook workbook, boolean shouldClose) {
            this.workbook = workbook;
            this.shouldClose = shouldClose;
        }

        public WorkbookAssert hasSheetCount(int expected) {
            Assertions.assertEquals(
                    expected,
                    workbook.getNumberOfSheets(),
                    "Expected sheet count " + expected + " but was " + workbook.getNumberOfSheets());
            return this;
        }

        public WorkbookAssert hasSheetNamed(String name) {
            Sheet sheet = workbook.getSheet(name);
            Assertions.assertNotNull(sheet, "Sheet named '" + name + "' not found");
            return this;
        }

        public SheetAssert sheet(int index) {
            Sheet sheet = workbook.getSheetAt(index);
            Assertions.assertNotNull(sheet, "Sheet at index " + index + " is null");
            return new SheetAssert(sheet, workbook);
        }

        public SheetAssert sheet(String name) {
            Sheet sheet = workbook.getSheet(name);
            Assertions.assertNotNull(sheet, "Sheet named '" + name + "' not found");
            return new SheetAssert(sheet, workbook);
        }

        @Override
        public void close() throws Exception {
            if (shouldClose && workbook != null) {
                workbook.close();
            }
        }
    }

    /**
     * Sheet-level assertions.
     */
    public static class SheetAssert {
        protected final Sheet sheet;
        protected final Workbook workbook;

        public SheetAssert(Sheet sheet, Workbook workbook) {
            this.sheet = sheet;
            this.workbook = workbook;
        }

        public WorkbookAssert and() {
            return new WorkbookAssert(workbook, false);
        }

        public SheetAssert satisfies(Consumer<Sheet> consumer) {
            consumer.accept(this.sheet);
            return this;
        }

        public SheetAssert hasRowCount(int expected) {
            int actual = sheet.getPhysicalNumberOfRows();
            Assertions.assertEquals(expected, actual, "Expected row count " + expected + " but was " + actual);
            return this;
        }

        public RowAssert row(int rowIndex) {
            Row row = sheet.getRow(rowIndex);
            Assertions.assertNotNull(row, "Row at index " + rowIndex + " does not exist");
            return new RowAssert(row, sheet);
        }

        public SheetAssert hasColumnWidth(int col, int expectedWidth) {
            int actualWidth = sheet.getColumnWidth(col);
            Assertions.assertEquals(
                    expectedWidth,
                    actualWidth,
                    "Expected column width " + expectedWidth + " for column " + col + " but was " + actualWidth);
            return this;
        }

        public CellAssert cell(int row, int col) {
            return row(row).cell(col);
        }

        public SheetAssert hasCellAt(int row, int col) {
            cell(row, col);
            return this;
        }
    }

    /**
     * Row-level assertions.
     */
    public static class RowAssert {
        private final Row row;
        private final Sheet sheet;

        public RowAssert(Row row, Sheet sheet) {
            this.row = row;
            this.sheet = sheet;
        }

        public SheetAssert and() {
            return new SheetAssert(sheet, sheet.getWorkbook());
        }

        public RowAssert satisfies(Consumer<Row> consumer) {
            consumer.accept(this.row);
            return this;
        }

        public RowAssert hasCellCount(int expected) {
            int actual = row.getPhysicalNumberOfCells();
            Assertions.assertEquals(expected, actual,
                    "Expected cell count " + expected + " at row " + row.getRowNum() + " but was " + actual);
            return this;
        }

        public RowAssert hasHeight(short expected) {
            Assertions.assertEquals(expected, row.getHeight(),
                    "Expected row height " + expected + " but was " + row.getHeight());
            return this;
        }

        public CellAssert cell(int col) {
            Cell cell = row.getCell(col);
            Assertions.assertNotNull(cell, "Cell at row " + row.getRowNum() + ", column " + col + " does not exist");
            return new CellAssert(cell, sheet);
        }
    }

    /**
     * Cell-level assertions.
     */
    public static class CellAssert {
        private final Cell cell;
        private final Sheet sheet;

        public CellAssert(Cell cell, Sheet sheet) {
            this.cell = cell;
            this.sheet = sheet;
        }

        public RowAssert and() {
            return new RowAssert(cell.getRow(), sheet);
        }

        public CellAssert satisfies(Consumer<Cell> consumer) {
            consumer.accept(this.cell);
            return this;
        }

        public CellAssert hasStringValue(String expected) {
            String actual = getCellValueAsString();
            Assertions.assertEquals(
                    expected, actual, "Expected cell value '" + expected + "' but was '" + actual + "'");
            return this;
        }

        public CellAssert hasNumericValue(double expected) {
            Assertions.assertEquals(
                    CellType.NUMERIC, cell.getCellType(), "Cell is not numeric, got: " + cell.getCellType());
            double actual = cell.getNumericCellValue();
            Assertions.assertEquals(
                    expected, actual, 0.001, "Expected numeric value " + expected + " but was " + actual);
            return this;
        }

        public CellAssert hasBooleanValue(boolean expected) {
            Assertions.assertEquals(
                    CellType.BOOLEAN, cell.getCellType(), "Cell is not boolean, got: " + cell.getCellType());
            boolean actual = cell.getBooleanCellValue();
            Assertions.assertEquals(expected, actual, "Expected boolean value " + expected + " but was " + actual);
            return this;
        }

        public CellAssert hasFormulaValue() {
            Assertions.assertEquals(
                    CellType.FORMULA, cell.getCellType(), "Cell is not a formula, got: " + cell.getCellType());
            return this;
        }

        public CellAssert isEmpty() {
            CellType type = cell.getCellType();
            Assertions.assertTrue(
                    type == CellType.BLANK
                            || (type == CellType.STRING
                                    && cell.getStringCellValue().trim().isEmpty()),
                    "Expected cell to be empty, but was: " + getCellValueAsString());
            return this;
        }

        public CellAssert hasType(CellType expected) {
            CellType actual = cell.getCellType();
            Assertions.assertEquals(expected, actual, "Expected cell type " + expected + " but was " + actual);
            return this;
        }

        public CellAssert hasFontColor(short expectedColorIndex) {
            Font font = sheet.getWorkbook().getFontAt(cell.getCellStyle().getFontIndex());
            Assertions.assertEquals(
                    expectedColorIndex,
                    font.getColor(),
                    "Expected font color index " + expectedColorIndex + " but was " + font.getColor());
            return this;
        }

        public CellAssert hasFillColor(short expectedColorIndex) {
            Assertions.assertEquals(
                    expectedColorIndex,
                    cell.getCellStyle().getFillForegroundColor(),
                    "Expected fill color index " + expectedColorIndex + " but was "
                            + cell.getCellStyle().getFillForegroundColor());
            return this;
        }

        private String getCellValueAsString() {
            switch (cell.getCellType()) {
                case STRING:
                    return cell.getStringCellValue();
                case NUMERIC:
                    if (DateUtil.isCellDateFormatted(cell)) {
                        return cell.getDateCellValue().toString();
                    } else {
                        double value = cell.getNumericCellValue();
                        // Convert to integer if it's a whole number
                        if (value == Math.floor(value)) {
                            return String.valueOf((long) value);
                        } else {
                            return String.valueOf(value);
                        }
                    }
                case BOOLEAN:
                    return String.valueOf(cell.getBooleanCellValue());
                case FORMULA:
                    return cell.getCellFormula();
                case BLANK:
                    return "";
                default:
                    return cell.getStringCellValue();
            }
        }
    }
}

What do you think of these changes?

[Copilot]

Pull request overview

This PR adds a comprehensive test infrastructure (testkit) and refactors existing tests to use modern JUnit 5 patterns with parameterized testing across multiple Excel formats (XLSX, XLS, CSV).

Key changes:

• New testkit infrastructure with base classes, fluent assertions, and reusable utilities
• Migration of simple/ and converter/ test packages to parameterized JUnit 5 tests
• Consolidation of test data models into the model/ package with enhanced functionality

Reviewed changes

Copilot reviewed 30 out of 30 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
testkit/base/AbstractExcelTest.java	Base class providing temp directory management and format-aware parameterized test providers
testkit/enums/ExcelFormat.java	Enum with format capabilities metadata (XLSX/XLS/CSV) for parameterized testing
testkit/listeners/CollectingReadListener.java	Reusable listener following Single Responsibility Principle (data collection only, no assertions)
testkit/assertions/ExcelAssertions.java	Fluent API for Excel workbook/sheet/cell assertions with AutoCloseable support
testkit/util/TestFileUtil.java	Format-aware file utilities for temp file management and resource handling
testkit/util/ExcelTestHelper.java	Helper methods for inspecting Excel workbook structure and content
testkit/util/AssertionHelper.java	Common assertion patterns for SimpleData and exception validation
testkit/data/TestDataFactory.java	Static factory methods for creating consistent test data
testkit/data/TestDataBuilder.java	Builder pattern for fluent test data construction
testkit/data/RandomDataGenerator.java	Random data generation for fuzz testing
model/SimpleData.java	Consolidated simple data model with name, number, and integer fields
model/ComplexData.java	Comprehensive data model for testing various data type conversions
model/ConverterData.java	Unified converter data model (replaces separate read/write classes)
model/ImageData.java	Image data model supporting File, InputStream, String path, and byte array
integration/read/SimpleReadWriteTest.java	Refactored tests using parameterized formats with nested test organization
integration/converter/ConverterIntegrationTest.java	Comprehensive type conversion tests across all formats
integration/converter/CustomConverterIntegrationTest.java	Tests for custom converter registration and usage
unit/converter/ConverterUnitTest.java	Unit tests for individual converter implementations
simple/SimpleDataTest.java	Marked as deprecated with reference to new test
converter/ConverterDataTest.java	Marked as deprecated with migration guidance
pom.xml	Added AssertJ 3.24.2 dependency for fluent assertions

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

[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:

private static final Random RANDOM = new Random(/* configurable seed */);

or providing a method to set the seed for test reproducibility.

Suggested change

	private static final Random RANDOM = new Random();
	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);
	}

[Copilot]

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:

if (destination.exists()) {
    destination.delete();
}

before line 99, or use Files.copy with StandardCopyOption.REPLACE_EXISTING.

[Copilot]

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:

return Files.readAllBytes(file.toPath());

This is simpler and handles partial reads correctly.

Suggested change

	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;
	}
	return java.nio.file.Files.readAllBytes(file.toPath());

[Copilot]

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.

[Copilot]

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

[Copilot]

[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:

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.

Suggested change

	file.delete();
	boolean deleted = file.delete();
	if (!deleted) {
	System.err.println("Warning: Failed to delete temporary file: " + file.getAbsolutePath());
	}

[Copilot]

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:

if (tempFile.exists()) {
    boolean deleted = tempFile.delete();
    if (!deleted) {
        throw new IOException("Failed to delete existing temp file: " + tempFile);
    }
}

Suggested change

	tempFile.delete();
	boolean deleted = tempFile.delete();
	if (!deleted) {
	throw new IOException("Failed to delete existing temp file: " + tempFile);
	}

[Copilot]

[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.

Suggested change

	@Tag("unit")
	@Tag("integration")

[Copilot]

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:

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.

Suggested change

	import org.apache.fesod.sheet.simple.SimpleData;
	import org.apache.fesod.sheet.model.SimpleData;

[alaahong]

please manage version with parent dependency management

[alaahong]

By default, it's better to provide English header in code. You can create another class with Chinese prefix.

[alaahong]

Decimal~

[alaahong]

Why this is not the object as other fields？

[alaahong]

Why is not the wrapper class？

[alaahong]

What's the behavior of Instant ?

[alaahong]

Why mix Lombok and hard code for bean usage code?

[alaahong]

is that helpful to append the random value for multiple trigger in one method?

[alaahong]

will we have the unit test for such test method? otherwise why protected ?

[alaahong]

As project involved RandomUtils and RandomStringUtils, which methods are not mandatory?