Skip to content

[Test] Add testkit infrastructure and refactor unit tests with JUnit 5 parameterized patterns#901

Open
GOODBOY008 wants to merge 3 commits intoapache:mainfrom
GOODBOY008:test/717-1-testkit-foundation
Open

[Test] Add testkit infrastructure and refactor unit tests with JUnit 5 parameterized patterns#901
GOODBOY008 wants to merge 3 commits intoapache:mainfrom
GOODBOY008:test/717-1-testkit-foundation

Conversation

@GOODBOY008
Copy link
Copy Markdown
Member

@GOODBOY008 GOODBOY008 commented Apr 22, 2026

Summary

Implements Phase 1-6 of the test modernization proposal from issue #717.

Changes

New Testkit Infrastructure (testkit/ package)

  • AbstractExcelTest — base class with format providers and temp file management
  • ExcelFormat enum — XLSX/XLS/CSV with capability metadata
  • CollectingReadListener — reusable data collector without embedded assertions
  • ExcelAssertions — fluent Excel file assertions via AssertJ

Parameterized Tests (JUnit 5)

  • Replace 3 nearly identical per-format test methods with a single @ParameterizedTest
  • Automatic format coverage: XLSX, XLS, and CSV in one method

Separation of Concerns

  • Remove listener classes that embed Assertions.* calls (SRP violation)
  • Move all assertions into test methods

Package Reorganization

  • Consolidate legacy scattered packages into core/, format/, readwrite/, sheet/, style/
  • Merge near-duplicate model classes

Metrics

Before After
Listener Classes 25+ 1
Per-format test methods 3x 1x (parameterized)

Related

Closes #717

… parameterized patterns

Implements Phase 1-2 of the test modernization proposal from issue apache#717:

- Add testkit/ package with AbstractExcelTest, ExcelFormat enum,
  CollectingReadListener, and ExcelAssertions for shared test infrastructure
- Migrate converter and other legacy test packages to parameterized JUnit 5 tests
- Remove listener classes with embedded assertions (SRP violation)
- Consolidate scattered model classes into organized packages (core/, format/,
  readwrite/, sheet/, style/)
- Apply parameterized test pattern to cover XLSX/XLS/CSV in single methods
- Update DateUtils to support test infrastructure

Closes apache#717
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR modernizes the fesod-sheet test suite by introducing a reusable testkit layer (formats, data builders, listeners, assertions) and refactoring many tests to JUnit 5 parameterized patterns, while reorganizing legacy test packages into clearer functional groupings.

Changes:

  • Added org.apache.fesod.sheet.testkit infrastructure (format enum + base test class + reusable listener + fluent Excel assertions).
  • Refactored many tests to parameterized per-format execution (XLSX/XLS/CSV) and moved assertions out of listeners into test methods.
  • Reorganized test packages/models and updated several CSV headers/test data to align with the new shared models.

Reviewed changes

Copilot reviewed 162 out of 163 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
fesod-sheet/src/test/resources/csv/simple.csv Updates CSV header text to match refactored CSV test models.
fesod-sheet/src/test/resources/bom/office_bom.csv Updates BOM CSV header text to match refactored models.
fesod-sheet/src/test/resources/bom/no_bom.csv Updates non-BOM CSV header text to match refactored models.
fesod-sheet/src/test/java/org/apache/fesod/sheet/util/StyleUtilTest.java Comment language cleanup in existing unit test.
fesod-sheet/src/test/java/org/apache/fesod/sheet/util/FileTypeUtilsTest.java Fixes minor try-with-resources formatting.
fesod-sheet/src/test/java/org/apache/fesod/sheet/util/DateUtilsTest.java Removes unnecessary checked exception from a parameterized test.
fesod-sheet/src/test/java/org/apache/fesod/sheet/util/ConverterUtilsTest.java Uses JUnit 5 assertInstanceOf for type assertion.
fesod-sheet/src/test/java/org/apache/fesod/sheet/util/ClassUtilsTest.java Removes unnecessary checked exceptions from test signature.
fesod-sheet/src/test/java/org/apache/fesod/sheet/util/BeanMapUtilsTest.java Replaces manual getters/setters with Lombok for test DTO.
fesod-sheet/src/test/java/org/apache/fesod/sheet/testkit/models/TitleData.java Moves/renames shared title model into testkit.models.
fesod-sheet/src/test/java/org/apache/fesod/sheet/testkit/models/SimpleData.java Consolidates shared SimpleData model and updates headers/fields.
fesod-sheet/src/test/java/org/apache/fesod/sheet/testkit/models/ConverterBaseData.java Introduces base class for converter read/write test models.
fesod-sheet/src/test/java/org/apache/fesod/sheet/testkit/listeners/CollectingReadListenerTest.java Adds unit tests for new reusable collecting listener.
fesod-sheet/src/test/java/org/apache/fesod/sheet/testkit/listeners/CollectingReadListener.java Adds reusable row-collecting listener (no embedded test assertions).
fesod-sheet/src/test/java/org/apache/fesod/sheet/testkit/enums/ExcelFormatTest.java Adds tests for ExcelFormat capability flags and temp file behavior.
fesod-sheet/src/test/java/org/apache/fesod/sheet/testkit/enums/ExcelFormat.java Adds test-only format enum bridging to production ExcelTypeEnum.
fesod-sheet/src/test/java/org/apache/fesod/sheet/testkit/enums/ApiMode.java Adds enum for file-vs-stream API parameterization.
fesod-sheet/src/test/java/org/apache/fesod/sheet/testkit/base/AbstractExcelTest.java Adds shared JUnit 5 base class with temp dir + method sources + helpers.
fesod-sheet/src/test/java/org/apache/fesod/sheet/testkit/assertions/WorkbookAssert.java Adds fluent workbook assertions for tests.
fesod-sheet/src/test/java/org/apache/fesod/sheet/testkit/assertions/SheetAssert.java Adds fluent sheet assertions for tests.
fesod-sheet/src/test/java/org/apache/fesod/sheet/testkit/assertions/RowAssert.java Adds fluent row assertions for tests.
fesod-sheet/src/test/java/org/apache/fesod/sheet/testkit/assertions/ExcelAssertions.java Adds fluent entrypoint for workbook assertions (AutoCloseable).
fesod-sheet/src/test/java/org/apache/fesod/sheet/template/TemplateDataTest.java Refactors template test to parameterized binary-format execution.
fesod-sheet/src/test/java/org/apache/fesod/sheet/template/TemplateDataListener.java Removes assertion-embedding listener (SRP cleanup).
fesod-sheet/src/test/java/org/apache/fesod/sheet/template/TemplateData.java Updates header names to match refactored test data.
fesod-sheet/src/test/java/org/apache/fesod/sheet/style/StyleDataListener.java Removes assertion-embedding listener (SRP cleanup).
fesod-sheet/src/test/java/org/apache/fesod/sheet/style/StyleData.java Updates header names to match refactored test data.
fesod-sheet/src/test/java/org/apache/fesod/sheet/style/FillStyleData.java Moves package into consolidated style test namespace.
fesod-sheet/src/test/java/org/apache/fesod/sheet/style/FillStyleAnnotatedData.java Moves package into consolidated style test namespace.
fesod-sheet/src/test/java/org/apache/fesod/sheet/style/FillData.java Moves package into consolidated style test namespace.
fesod-sheet/src/test/java/org/apache/fesod/sheet/style/FillAnnotationDataTest.java Refactors fill-annotation test to parameterized binary formats + shared builders.
fesod-sheet/src/test/java/org/apache/fesod/sheet/style/FillAnnotationData.java Moves package into consolidated style test namespace.
fesod-sheet/src/test/java/org/apache/fesod/sheet/style/ExcelPropertyFormatTest.java Moves test into style package and fixes imports.
fesod-sheet/src/test/java/org/apache/fesod/sheet/style/AnnotationStyleData.java Moves model into style package and updates headers/imports.
fesod-sheet/src/test/java/org/apache/fesod/sheet/style/AnnotationIndexAndNameDataTest.java Adds parameterized test covering all formats for index/name annotations.
fesod-sheet/src/test/java/org/apache/fesod/sheet/style/AnnotationIndexAndNameData.java Moves model into style package and updates header names.
fesod-sheet/src/test/java/org/apache/fesod/sheet/style/AnnotationDataTest.java Adds parameterized annotation tests + uses fluent Excel assertions for styles.
fesod-sheet/src/test/java/org/apache/fesod/sheet/style/AnnotationData.java Moves model into style package and updates header names/imports.
fesod-sheet/src/test/java/org/apache/fesod/sheet/sort/SortDataTest.java Removes legacy per-format sort test class (migrated elsewhere).
fesod-sheet/src/test/java/org/apache/fesod/sheet/sort/SortDataListener.java Removes legacy assertion-embedding listener.
fesod-sheet/src/test/java/org/apache/fesod/sheet/skip/SkipData.java Removes legacy model (covered by shared models after reorg).
fesod-sheet/src/test/java/org/apache/fesod/sheet/simple/SimpleDataListener.java Removes legacy assertion-embedding listener.
fesod-sheet/src/test/java/org/apache/fesod/sheet/sheet/WriteSheetData.java Moves model into sheet package and updates header text.
fesod-sheet/src/test/java/org/apache/fesod/sheet/sheet/MultipleSheetsDataTest.java Refactors to parameterized binary formats + shared listener/model.
fesod-sheet/src/test/java/org/apache/fesod/sheet/sheet/HiddenSheetsTest.java Adds parameterized binary-format tests for hidden sheet handling.
fesod-sheet/src/test/java/org/apache/fesod/sheet/repetition/RepetitionDataTest.java Removes legacy repetition test class (replaced by parameterized core test).
fesod-sheet/src/test/java/org/apache/fesod/sheet/repetition/RepetitionDataListener.java Removes legacy assertion-embedding listener.
fesod-sheet/src/test/java/org/apache/fesod/sheet/readwrite/XlsxSaxAnalyserReadOpcPackageTest.java Moves test into readwrite package and extends shared base test.
fesod-sheet/src/test/java/org/apache/fesod/sheet/readwrite/LargeDataTest.java Refactors large-data tests to use temp dir + moves assertions into test.
fesod-sheet/src/test/java/org/apache/fesod/sheet/readwrite/LargeDataListener.java Moves listener into readwrite package and removes embedded assertions.
fesod-sheet/src/test/java/org/apache/fesod/sheet/readwrite/LargeData.java Moves model into readwrite package.
fesod-sheet/src/test/java/org/apache/fesod/sheet/readwrite/ExceptionThrowDataListener.java Moves/retargets listener to shared SimpleData for exception-path tests.
fesod-sheet/src/test/java/org/apache/fesod/sheet/readwrite/ExceptionDataTest.java Adds parameterized exception-path tests using shared builders/models.
fesod-sheet/src/test/java/org/apache/fesod/sheet/readwrite/ExceptionDataListener.java Moves listener into readwrite, removes embedded assertions, captures metadata for test assertions.
fesod-sheet/src/test/java/org/apache/fesod/sheet/readwrite/ExcelAnalysisStopSheetExceptionDataListener.java Moves listener into readwrite and removes embedded assertions.
fesod-sheet/src/test/java/org/apache/fesod/sheet/readwrite/EncryptDataTest.java Adds parameterized password/stream/file combination coverage.
fesod-sheet/src/test/java/org/apache/fesod/sheet/readwrite/DemoData.java Moves model into readwrite and translates class doc comment.
fesod-sheet/src/test/java/org/apache/fesod/sheet/readwrite/DateWindowingListener.java Renames/moves listener to capture windowing setting without embedded assertions.
fesod-sheet/src/test/java/org/apache/fesod/sheet/readwrite/CacheData.java Moves model into readwrite and updates header text.
fesod-sheet/src/test/java/org/apache/fesod/sheet/parameter/ParameterDataListener.java Removes legacy assertion-embedding listener.
fesod-sheet/src/test/java/org/apache/fesod/sheet/parameter/ParameterData.java Removes legacy model superseded by shared models.
fesod-sheet/src/test/java/org/apache/fesod/sheet/parameter/DateWindowingListener.java Removes legacy listener superseded by refactored readwrite listener.
fesod-sheet/src/test/java/org/apache/fesod/sheet/noncamel/UnCamelDataTest.java Removes legacy per-format test class (replaced by parameterized core test).
fesod-sheet/src/test/java/org/apache/fesod/sheet/noncamel/UnCamelDataListener.java Removes legacy assertion-embedding listener.
fesod-sheet/src/test/java/org/apache/fesod/sheet/multiplesheets/MultipleSheetsListener.java Removes legacy assertion-embedding listener.
fesod-sheet/src/test/java/org/apache/fesod/sheet/hiddensheets/HiddenSheetsTest.java Removes legacy per-format hidden sheet test (replaced by parameterized version).
fesod-sheet/src/test/java/org/apache/fesod/sheet/hiddensheets/HiddenSheetsListener.java Removes legacy assertion-embedding listener.
fesod-sheet/src/test/java/org/apache/fesod/sheet/head/NoHeadDataTest.java Refactors to parameterized test and uses shared data builder.
fesod-sheet/src/test/java/org/apache/fesod/sheet/head/NoHeadData.java Updates header text to match refactored test data.
fesod-sheet/src/test/java/org/apache/fesod/sheet/head/MaxHeadSizeTest.java Refactors to temp-dir base and removes file path globals.
fesod-sheet/src/test/java/org/apache/fesod/sheet/head/MaxHeadReadListener.java Simplifies listener to capture head metadata only.
fesod-sheet/src/test/java/org/apache/fesod/sheet/head/ListHeadDataTest.java Refactors to parameterized test and in-test assertions on sync read output.
fesod-sheet/src/test/java/org/apache/fesod/sheet/head/ListHeadDataListener.java Removes legacy assertion-embedding listener.
fesod-sheet/src/test/java/org/apache/fesod/sheet/head/ImmutableListHeadDataTest.java Refactors to parameterized test and replaces legacy API usage with FesodSheet.
fesod-sheet/src/test/java/org/apache/fesod/sheet/head/ImmutableListHeadDataListener.java Removes legacy assertion-embedding listener.
fesod-sheet/src/test/java/org/apache/fesod/sheet/head/ComplexHeadDataTest.java Refactors to parameterized tests and moves assertions into test methods.
fesod-sheet/src/test/java/org/apache/fesod/sheet/head/ComplexHeadData.java Updates complex head labels to English equivalents.
fesod-sheet/src/test/java/org/apache/fesod/sheet/head/ComplexDataListener.java Removes legacy assertion-embedding listener.
fesod-sheet/src/test/java/org/apache/fesod/sheet/handler/WriteHandlerTest.java Refactors to parameterized tests using shared SimpleData + includeColumnFieldNames.
fesod-sheet/src/test/java/org/apache/fesod/sheet/handler/WriteHandlerData.java Removes legacy model superseded by shared SimpleData.
fesod-sheet/src/test/java/org/apache/fesod/sheet/format/DateFormatTest.java Moves/refactors date-format tests into format package using parameterization.
fesod-sheet/src/test/java/org/apache/fesod/sheet/format/DateFormatData.java Moves model into format package.
fesod-sheet/src/test/java/org/apache/fesod/sheet/format/CsvDataListener.java Moves listener into format package.
fesod-sheet/src/test/java/org/apache/fesod/sheet/format/CsvData.java Moves model into format and updates header strings + comment text.
fesod-sheet/src/test/java/org/apache/fesod/sheet/format/CsvBenignErrorToleranceTest.java Moves test into format and extends shared base test.
fesod-sheet/src/test/java/org/apache/fesod/sheet/format/CharsetDataTest.java Adds/updates charset tests using shared models/listener.
fesod-sheet/src/test/java/org/apache/fesod/sheet/format/BomDataTest.java Adds/updates BOM tests using shared models/listener.
fesod-sheet/src/test/java/org/apache/fesod/sheet/extra/ExtraDataTest.java Removes legacy extra-data test class (reorganized).
fesod-sheet/src/test/java/org/apache/fesod/sheet/extra/ExtraDataListener.java Removes legacy assertion-embedding listener.
fesod-sheet/src/test/java/org/apache/fesod/sheet/exception/ExceptionData.java Removes legacy model superseded by shared SimpleData.
fesod-sheet/src/test/java/org/apache/fesod/sheet/encrypt/EncryptDataListener.java Removes legacy assertion-embedding listener.
fesod-sheet/src/test/java/org/apache/fesod/sheet/encrypt/EncryptData.java Removes legacy model superseded by shared SimpleData.
fesod-sheet/src/test/java/org/apache/fesod/sheet/dataformat/DateFormatTest.java Removes legacy date-format test (moved/refactored).
fesod-sheet/src/test/java/org/apache/fesod/sheet/core/UnCamelDataTest.java Adds parameterized camel-case behavior tests using shared helper patterns.
fesod-sheet/src/test/java/org/apache/fesod/sheet/core/UnCamelData.java Moves model into core package.
fesod-sheet/src/test/java/org/apache/fesod/sheet/core/SimpleDataTest.java Adds parameterized simple read/write tests using shared helpers and API mode.
fesod-sheet/src/test/java/org/apache/fesod/sheet/core/RepetitionDataTest.java Adds parameterized repetition tests using shared listener and builder.
fesod-sheet/src/test/java/org/apache/fesod/sheet/core/RepetitionData.java Moves model into core and updates header text.
fesod-sheet/src/test/java/org/apache/fesod/sheet/core/NoModelDataTest.java Adds parameterized no-model read/write tests across formats.
fesod-sheet/src/test/java/org/apache/fesod/sheet/converter/SortData.java Moves SortData model into converter package.
fesod-sheet/src/test/java/org/apache/fesod/sheet/converter/ReadAllConverterDataListener.java Removes legacy assertion-embedding listener.
fesod-sheet/src/test/java/org/apache/fesod/sheet/converter/GlobalConverterWriteData.java Updates header text for converter write data.
fesod-sheet/src/test/java/org/apache/fesod/sheet/converter/ExtraDataListener.java Repurposes listener to capture extras without embedded assertions.
fesod-sheet/src/test/java/org/apache/fesod/sheet/converter/ExtraData.java Moves extra-data model into converter package.
fesod-sheet/src/test/java/org/apache/fesod/sheet/converter/ExcludeOrIncludeData.java Moves model into converter package.
fesod-sheet/src/test/java/org/apache/fesod/sheet/converter/CustomConverterWriteData.java Updates header text for converter write model fields.
fesod-sheet/src/test/java/org/apache/fesod/sheet/converter/CustomConverterTest.java Refactors converter tests to use temp dir and shared TestDataBuilder.
fesod-sheet/src/test/java/org/apache/fesod/sheet/converter/ConverterWriteData.java Deduplicates converter write model via shared base class.
fesod-sheet/src/test/java/org/apache/fesod/sheet/converter/ConverterTest.java Cleans up and modernizes converter unit test class.
fesod-sheet/src/test/java/org/apache/fesod/sheet/converter/ConverterReadData.java Deduplicates converter read model via shared base class.
fesod-sheet/src/test/java/org/apache/fesod/sheet/converter/ConverterDataListener.java Removes legacy assertion-embedding listener.
fesod-sheet/src/test/java/org/apache/fesod/sheet/converter/CellDataWriteData.java Moves cell-data write model into converter package.
fesod-sheet/src/test/java/org/apache/fesod/sheet/converter/CellDataReadData.java Moves cell-data read model into converter package.
fesod-sheet/src/test/java/org/apache/fesod/sheet/converter/CellDataDataTest.java Adds parameterized cell-data read/write tests using shared listener/builder.
fesod-sheet/src/test/java/org/apache/fesod/sheet/charset/CharsetDataTest.java Removes legacy charset test class (replaced by format suite).
fesod-sheet/src/test/java/org/apache/fesod/sheet/charset/CharsetData.java Removes legacy charset model (replaced by shared SimpleData).
fesod-sheet/src/test/java/org/apache/fesod/sheet/celldata/CellDataDataTest.java Removes legacy cell-data test class (replaced by parameterized version).
fesod-sheet/src/test/java/org/apache/fesod/sheet/celldata/CellDataDataListener.java Removes legacy assertion-embedding listener.
fesod-sheet/src/test/java/org/apache/fesod/sheet/cache/CacheInvokeMemoryData.java Removes legacy cache model (reorganized into shared models).
fesod-sheet/src/test/java/org/apache/fesod/sheet/cache/CacheInvokeData.java Removes legacy cache model (reorganized into shared models).
fesod-sheet/src/test/java/org/apache/fesod/sheet/bom/BomData.java Removes legacy BOM model (replaced by shared SimpleData).
fesod-sheet/src/test/java/org/apache/fesod/sheet/annotation/AnnotationIndexAndNameDataTest.java Removes legacy annotation index/name test (replaced by style suite).
fesod-sheet/src/test/java/org/apache/fesod/sheet/annotation/AnnotationIndexAndNameDataListener.java Removes legacy assertion-embedding listener.
fesod-sheet/src/test/java/org/apache/fesod/sheet/annotation/AnnotationDataListener.java Removes legacy assertion-embedding listener.
fesod-sheet/src/test/java/org/apache/fesod/sheet/FesodSheetTest.java Removes unnecessary checked exception from test signature.
fesod-sheet/src/main/java/org/apache/fesod/sheet/util/DateUtils.java Comment indentation tweak inside switch (no functional change).

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

Comment on lines +29 to +72
* A generic, reusable {@link AnalysisEventListener} that simply collects every row into a list.
*
* <p>Unlike the legacy per-test listeners, this class contains <b>no assertions</b> and
* <b>no logging</b>. Assertions belong in the test method; data collection belongs here.
*
* @param <T> the row model type
*/
public class CollectingReadListener<T> extends AnalysisEventListener<T> {

private final List<T> rows = new ArrayList<T>();

@Override
public void invoke(T data, AnalysisContext context) {
rows.add(data);
}

@Override
public void doAfterAllAnalysed(AnalysisContext context) {
// intentionally empty — no assertions, no logging
}

/**
* Returns an unmodifiable view of all collected rows.
*/
public List<T> getRows() {
return Collections.unmodifiableList(rows);
}

/**
* Returns the number of collected rows.
*/
public int getRowCount() {
return rows.size();
}

/**
* Returns the first collected row.
*
* @throws AssertionError if no rows have been collected
*/
public T getFirstRow() {
if (rows.isEmpty()) {
throw new AssertionError("Expected at least one row, but CollectingReadListener collected none");
}
Copy link

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CollectingReadListener Javadoc states it contains “no assertions”, but getFirstRow() throws an AssertionError when empty. Either adjust the documentation to reflect this behavior or change the exception type (e.g., IllegalStateException/NoSuchElementException) so this helper doesn’t embed assertion semantics.

Copilot uses AI. Check for mistakes.
Comment on lines +57 to +66
public static ExcelAssertions assertThat(File file) {
if (!file.exists()) {
throw new AssertionError("File does not exist: " + file.getAbsolutePath());
}
try {
Workbook wb = WorkbookFactory.create(file);
return new ExcelAssertions(wb, file);
} catch (IOException e) {
throw new AssertionError("Failed to open workbook: " + file.getAbsolutePath(), e);
}
Copy link

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ExcelAssertions.assertThat(File) claims it throws AssertionError when the workbook cannot be opened, but it only catches IOException. WorkbookFactory.create(...) can also throw other exceptions (e.g., POI runtime exceptions for invalid/encrypted files), which will currently escape and violate the documented contract. Consider catching broader exceptions and wrapping them in AssertionError (preserving the cause) to keep behavior consistent.

Copilot uses AI. Check for mistakes.
@GOODBOY008 GOODBOY008 force-pushed the test/717-1-testkit-foundation branch from 71c7d69 to fe43798 Compare April 22, 2026 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Proposal] Refactor Unit Tests with Modern Testing Patterns

2 participants