[Test] Add testkit infrastructure and refactor unit tests with JUnit 5 parameterized patterns#901
[Test] Add testkit infrastructure and refactor unit tests with JUnit 5 parameterized patterns#901GOODBOY008 wants to merge 3 commits intoapache:mainfrom
Conversation
… 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
There was a problem hiding this comment.
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.testkitinfrastructure (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.
| * 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"); | ||
| } |
There was a problem hiding this comment.
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.
| 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); | ||
| } |
There was a problem hiding this comment.
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.
71c7d69 to
fe43798
Compare
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 managementExcelFormatenum — XLSX/XLS/CSV with capability metadataCollectingReadListener— reusable data collector without embedded assertionsExcelAssertions— fluent Excel file assertions via AssertJParameterized Tests (JUnit 5)
@ParameterizedTestSeparation of Concerns
Assertions.*calls (SRP violation)Package Reorganization
core/,format/,readwrite/,sheet/,style/Metrics
Related
Closes #717