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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
