You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fesod.apache.org by "GOODBOY008 (via GitHub)" <gi...@apache.org> on 2026/04/22 09:39:55 UTC
[PR] [Test] Add testkit infrastructure and refactor unit tests with JUnit 5 parameterized patterns [fesod]
GOODBOY008 opened a new pull request, #901:
URL: https://github.com/apache/fesod/pull/901
## Summary
Implements Phase 1-2 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
--
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: commits-unsubscribe@fesod.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For additional commands, e-mail: commits-help@fesod.apache.org
Re: [PR] [Test] Add testkit infrastructure and refactor unit tests with JUnit 5 parameterized patterns [fesod]
Posted by "psxjoy (via GitHub)" <gi...@apache.org>.
psxjoy merged PR #901:
URL: https://github.com/apache/fesod/pull/901
--
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: commits-unsubscribe@fesod.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For additional commands, e-mail: commits-help@fesod.apache.org
Re: [PR] [Test] Add testkit infrastructure and refactor unit tests with JUnit 5 parameterized patterns [fesod]
Posted by "bengbengbalabalabeng (via GitHub)" <gi...@apache.org>.
bengbengbalabalabeng commented on code in PR #901:
URL: https://github.com/apache/fesod/pull/901#discussion_r3458575131
##########
fesod-sheet/src/test/java/org/apache/fesod/sheet/template/TemplateDataTest.java:
##########
@@ -25,73 +25,45 @@
package org.apache.fesod.sheet.template;
+import static org.apache.fesod.sheet.testkit.params.FormatCapability.*;
+import static org.junit.jupiter.api.Assertions.assertEquals;
import java.io.File;
-import java.util.ArrayList;
import java.util.List;
import org.apache.fesod.sheet.FesodSheet;
+import org.apache.fesod.sheet.testkit.Tags;
+import org.apache.fesod.sheet.testkit.base.AbstractExcelTest;
+import org.apache.fesod.sheet.testkit.builders.TestDataBuilder;
+import org.apache.fesod.sheet.testkit.enums.ExcelFormat;
+import org.apache.fesod.sheet.testkit.params.ExcelFormatSource;
+import org.apache.fesod.sheet.testkit.params.FormatScope;
import org.apache.fesod.sheet.util.TestFileUtil;
-import org.junit.jupiter.api.BeforeAll;
-import org.junit.jupiter.api.MethodOrderer;
-import org.junit.jupiter.api.Test;
-import org.junit.jupiter.api.TestMethodOrder;
+import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.params.ParameterizedTest;
/**
- *
+ * Test template write/read for binary Excel formats using parameterized tests.
*/
-@TestMethodOrder(MethodOrderer.MethodName.class)
-public class TemplateDataTest {
-
- private static File file07;
- private static File file03;
-
- @BeforeAll
- public static void init() {
- file07 = TestFileUtil.createNewFile("template07.xlsx");
- file03 = TestFileUtil.createNewFile("template03.xls");
- }
-
- @Test
- public void t01ReadAndWrite07() {
- readAndWrite07(file07);
- }
-
- @Test
- public void t02ReadAndWrite03() {
- readAndWrite03(file03);
- }
+@Tag(Tags.ROUND_TRIP)
+public class TemplateDataTest extends AbstractExcelTest {
- private void readAndWrite07(File file) {
+ @ParameterizedTest
+ @ExcelFormatSource(value = FormatScope.BINARY, requires = TEMPLATES)
+ void readAndWrite(ExcelFormat format) throws Exception {
+ File file = createTempFile("template", format);
+ String templateName = "template" + File.separator + "template" + (format == ExcelFormat.XLSX ? "07" : "03")
+ + format.getExtension();
+ File template = TestFileUtil.readFile(templateName);
Review Comment:
The File path resolved by `TestFileUtil.readFile()` is `file:/Users/.../META-INF/versions/9/template/template07.xlsx`. This type of path does not correspond to a real file, and running tests locally (on macOS) will fail.
Could we either obtain the `InputStream` via `getResourceAsStream` for use, or write the stream into a TempDir and then return a File for use?
Additionally, since the usage of `TestFileUtil` is limited to unit testing and only the `readFile()` method is utilized, it is recommended to refactor `readFile()` into an internal utility method within `AbstractExcelTest` and remove `TestFileUtil`.
```java
protected File readFile(String name) throws IOException {
Path target = tempDir.toPath().resolve(name);
Files.createDirectories(target.getParent());
try (InputStream inputStream = getClass().getClassLoader().getResourceAsStream(name)) {
Files.copy(inputStream, target, StandardCopyOption.REPLACE_EXISTING);
return target.toFile();
}
}
```
--
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: commits-unsubscribe@fesod.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For additional commands, e-mail: commits-help@fesod.apache.org
Re: [PR] [Test] Add testkit infrastructure and refactor unit tests with JUnit 5 parameterized patterns [fesod]
Posted by "GOODBOY008 (via GitHub)" <gi...@apache.org>.
GOODBOY008 commented on PR #901:
URL: https://github.com/apache/fesod/pull/901#issuecomment-4362729772
> lgtm
Conflict resolved.
--
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: commits-unsubscribe@fesod.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For additional commands, e-mail: commits-help@fesod.apache.org
Re: [PR] [Test] Add testkit infrastructure and refactor unit tests with JUnit 5 parameterized patterns [fesod]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #901:
URL: https://github.com/apache/fesod/pull/901#discussion_r3123049670
##########
fesod-sheet/src/test/java/org/apache/fesod/sheet/testkit/listeners/CollectingReadListener.java:
##########
@@ -0,0 +1,82 @@
+/*
+ * 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.listeners;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import org.apache.fesod.sheet.context.AnalysisContext;
+import org.apache.fesod.sheet.event.AnalysisEventListener;
+
+/**
+ * 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");
+ }
Review 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.
##########
fesod-sheet/src/test/java/org/apache/fesod/sheet/testkit/assertions/ExcelAssertions.java:
##########
@@ -0,0 +1,99 @@
+/*
+ * 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.assertions;
+
+import java.io.File;
+import java.io.IOException;
+import org.apache.poi.ss.usermodel.Workbook;
+import org.apache.poi.ss.usermodel.WorkbookFactory;
+
+/**
+ * Entry point for the fluent Excel assertion API.
+ *
+ * <p>Opens a workbook from a file and provides chainable assertions on its contents.
+ * Implements {@link AutoCloseable} to ensure the workbook is closed after assertions.
+ *
+ * <p>Usage:
+ * <pre>{@code
+ * try (ExcelAssertions ea = ExcelAssertions.assertThat(file)) {
+ * ea.sheet(0).row(0).cell(0).hasStringValue("Name");
+ * }
+ * }</pre>
+ */
+public final class ExcelAssertions implements AutoCloseable {
+
+ private final Workbook workbook;
+ private final File file;
+
+ private ExcelAssertions(Workbook workbook, File file) {
+ this.workbook = workbook;
+ this.file = file;
+ }
+
+ /**
+ * Opens the given Excel file and returns a fluent assertion entry point.
+ *
+ * @param file the Excel file to assert against
+ * @return a new {@code ExcelAssertions} instance
+ * @throws AssertionError if the file does not exist or cannot be opened as a workbook
+ */
+ 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);
+ }
Review 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.
--
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: commits-unsubscribe@fesod.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For additional commands, e-mail: commits-help@fesod.apache.org
Re: [PR] [Test] Add testkit infrastructure and refactor unit tests with JUnit 5 parameterized patterns [fesod]
Posted by "GOODBOY008 (via GitHub)" <gi...@apache.org>.
GOODBOY008 commented on code in PR #901:
URL: https://github.com/apache/fesod/pull/901#discussion_r3466332050
##########
fesod-sheet/src/test/java/org/apache/fesod/sheet/template/TemplateDataTest.java:
##########
@@ -25,73 +25,45 @@
package org.apache.fesod.sheet.template;
+import static org.apache.fesod.sheet.testkit.params.FormatCapability.*;
+import static org.junit.jupiter.api.Assertions.assertEquals;
import java.io.File;
-import java.util.ArrayList;
import java.util.List;
import org.apache.fesod.sheet.FesodSheet;
+import org.apache.fesod.sheet.testkit.Tags;
+import org.apache.fesod.sheet.testkit.base.AbstractExcelTest;
+import org.apache.fesod.sheet.testkit.builders.TestDataBuilder;
+import org.apache.fesod.sheet.testkit.enums.ExcelFormat;
+import org.apache.fesod.sheet.testkit.params.ExcelFormatSource;
+import org.apache.fesod.sheet.testkit.params.FormatScope;
import org.apache.fesod.sheet.util.TestFileUtil;
-import org.junit.jupiter.api.BeforeAll;
-import org.junit.jupiter.api.MethodOrderer;
-import org.junit.jupiter.api.Test;
-import org.junit.jupiter.api.TestMethodOrder;
+import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.params.ParameterizedTest;
/**
- *
+ * Test template write/read for binary Excel formats using parameterized tests.
*/
-@TestMethodOrder(MethodOrderer.MethodName.class)
-public class TemplateDataTest {
-
- private static File file07;
- private static File file03;
-
- @BeforeAll
- public static void init() {
- file07 = TestFileUtil.createNewFile("template07.xlsx");
- file03 = TestFileUtil.createNewFile("template03.xls");
- }
-
- @Test
- public void t01ReadAndWrite07() {
- readAndWrite07(file07);
- }
-
- @Test
- public void t02ReadAndWrite03() {
- readAndWrite03(file03);
- }
+@Tag(Tags.ROUND_TRIP)
+public class TemplateDataTest extends AbstractExcelTest {
- private void readAndWrite07(File file) {
+ @ParameterizedTest
+ @ExcelFormatSource(value = FormatScope.BINARY, requires = TEMPLATES)
+ void readAndWrite(ExcelFormat format) throws Exception {
+ File file = createTempFile("template", format);
+ String templateName = "template" + File.separator + "template" + (format == ExcelFormat.XLSX ? "07" : "03")
+ + format.getExtension();
+ File template = TestFileUtil.readFile(templateName);
Review Comment:
Great catch!
--
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: commits-unsubscribe@fesod.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For additional commands, e-mail: commits-help@fesod.apache.org