[Proposal] Refactor Unit Tests with Modern Testing Patterns

[GOODBOY008]

Summary

This proposal outlines a comprehensive refactoring of FesodSheet's test suite to improve maintainability, reduce code duplication, and adopt modern JUnit 5 testing patterns.

Reference PR: #716

---

Why? Current Issues

1. Code Duplication (~40% redundant code)

Current State:
├── SimpleDataTest.java      → 3 nearly identical test methods (XLSX, XLS, CSV)
├── ConverterDataTest.java   → Same pattern repeated
├── FillDataTest.java        → Same pattern repeated
└── ... (15+ test classes)

Each format requires separate test methods with copy-pasted logic:

// Current: 3 methods doing the same thing
@Test void t01ReadAndWrite07() { readAndWrite(file07); }
@Test void t02ReadAndWrite03() { readAndWrite(file03); }
@Test void t03ReadAndWriteCsv() { readAndWrite(fileCsv); }

2. Listeners Mixed with Assertions (Violates SRP)

// Current: Listener contains test assertions
public class SimpleDataListener extends AnalysisEventListener<SimpleData> {
    @Override
    public void doAfterAllAnalysed(AnalysisContext context) {
        Assertions.assertEquals(10, list.size());  // ❌ Test logic in listener
    }
}

3. No Shared Test Infrastructure

• Each test package has its own data classes
• No reusable format providers
• No common assertion utilities for Excel-specific validations

4. Scattered Model Classes

• ConverterReadData.java and ConverterWriteData.java are nearly identical
• ImageData.java duplicated across packages

---

How? Solution Approach

New Testkit Infrastructure (testkit/ package)

testkit/
├── base/
│   └── AbstractExcelTest.java      # Format providers, temp file management
├── enums/
│   └── ExcelFormat.java            # XLSX/XLS/CSV with capability metadata
├── listeners/
│   └── CollectingReadListener.java # Reusable data collector (no assertions)
└── assertions/
    └── ExcelAssertions.java        # Fluent Excel file assertions

Parameterized Tests (JUnit 5)

// After: Single parameterized method
@ParameterizedTest(name = "Format: {0}")
@MethodSource("allFormats")
void shouldWriteAndReadCorrectly(ExcelFormat format) {
    File file = createTempFile("test", format);
    List<SimpleData> result = roundTrip(file, SimpleData.class, testData);
    assertThat(result).hasSize(10);
}

Separation of Concerns

// After: Listener only collects data
CollectingReadListener<T> listener = new CollectingReadListener<>();
FesodSheet.read(file, clazz, listener).sheet().doRead();

// Assertions in test method
assertThat(listener.getData()).hasSize(10);

---

Target Structure

src/test/java/org/apache/fesod/sheet/
├── testkit/                    # Shared test infrastructure
├── model/                      # Shared data models
├── unit/                       # Pure unit tests (no I/O)
├── integration/                # Integration tests (file I/O)
└── [legacy packages]           # Deprecated, to be removed

---

Benefits

Metric	Before	After	Improvement
Test Methods	~300	~100	-67%
Listener Classes	25+	1	-96%
Lines of Test Code	~8,000	~5,000	-37%
Format Coverage	Manual	Automatic	100%

---

Migration Strategy

Phase	Scope	Status
Phase 1	Testkit Infrastructure	✅ PR #716
Phase 2	simple/, converter/	✅ PR #716
Phase 3	fill/, style/, head/	🔲 Future
Phase 4	template/, annotation/	🔲 Future
Phase 5	Remaining packages	🔲 Future
Phase 6	Legacy cleanup	🔲 Future

---

Discussion Points

1. Backward Compatibility: Keep deprecated tests running during migration?
2. Package Naming: integration/ vs feature-based (read/, write/)?
3. AssertJ Dependency: Added for fluent assertions - acceptable?
4. Timeline: Phased migration or big-bang?

---

References

• Implementation PR: [Test] Add testkit infrastructure and refactor simple/converter tests #716

[GOODBOY008]

@psxjoy @delei To facilitate discussion and demonstrate the benefits quantitatively, I've created a draft PR: #716