Skip to content

test: add tests for EscapeHexCellWriteHandler - #1001

Open
nkuprins wants to merge 6 commits into
apache:mainfrom
nkuprins:test/escape-hex-write-handler
Open

test: add tests for EscapeHexCellWriteHandler#1001
nkuprins wants to merge 6 commits into
apache:mainfrom
nkuprins:test/escape-hex-write-handler

Conversation

@nkuprins

@nkuprins nkuprins commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Related: #1000

Purpose of the pull request

EscapeHexCellWriteHandler has no tests. It used to have them, but they were deleted by accident (read #1000).

This PR is not a blind restore of those lost tests! I took a different approach; for example, @ParameterizedTest were used for better scalability and readability, etc.

What's changed?

  • Two parameterised tables covering the escaping rule itself. Seven inputs that must be rewritten (_xHHHH_ to _x005F_xHHHH_) and eight that must be left exactly as typed.
  • Non-idempotency: an already-escaped literal is escaped again.
  • Guard tests for the inputs the handler must not act on: non-STRING cell data, a null WriteCellData, and a STRING holding a null value.
  • One end-to-end case, parameterised over XLSX/XLS/CSV with @ExcelFormatSource: write a file through Fesod with the handler registered, read it back, and require the caller's literal. It sits in its own class tagged ROUND_TRIP, which keeps the rest tagged UNIT.

Checklist

  • I have read the Contributor Guide.
  • I have written the necessary doc or comment.
  • I have added the necessary unit tests and all cases have passed.

@nkuprins nkuprins changed the title test: add tests for escape-hex-write-handler test: add tests for EscapeHexCellWriteHandler Aug 10, 2026
@delei
delei requested a lite review from Copilot August 11, 2026 12:11

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds comprehensive regression tests for EscapeHexCellWriteHandler (per #1000), re-establishing coverage for both the core escaping rule and an end-to-end round-trip scenario across supported formats.

Changes:

  • Adds parameterized unit tests covering valid _xHHHH_ escaping, invalid patterns, non-idempotency, and null/non-string guards.
  • Adds a ROUND_TRIP integration test parameterized over XLSX/XLS/CSV to ensure the literal sequence survives write+read with the handler registered.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
fesod-sheet/src/test/java/org/apache/fesod/sheet/write/handler/EscapeHexCellWriteHandlerTest.java Unit-level parameterized coverage for pattern escaping and guard conditions.
fesod-sheet/src/test/java/org/apache/fesod/sheet/write/handler/EscapeHexCellWriteHandlerRoundTripTest.java End-to-end round-trip test across XLSX/XLS/CSV with the handler registered.
Suppressed comments (1)

fesod-sheet/src/test/java/org/apache/fesod/sheet/write/handler/EscapeHexCellWriteHandlerRoundTripTest.java:63

  • Reading the generated CSV with FileReader relies on the platform default charset. Use UTF-8 explicitly so the round-trip assertion is stable across environments.
        if (format == ExcelFormat.CSV) {
            try (BufferedReader reader = new BufferedReader(new FileReader(file))) {
                reader.readLine(); // header
                return reader.readLine();
            }

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@delei
delei requested a review from alaahong August 11, 2026 12:15
@delei

delei commented Aug 11, 2026

Copy link
Copy Markdown
Member

LGTM.
@alaahong PTAL when it is convenient for you.

@alaahong alaahong left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Generally fine, can you revise the code as common practice?

() -> handler.afterCellDataConverted(null, null, emptyStringData, cell, null, 0, Boolean.FALSE));
Assertions.assertNull(emptyStringData.getStringValue());
}
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It should be fine, a little suggestion, seems it's bringing some new additional exceptions by testing code?
https://github.com/apache/fesod/actions/runs/31603043137/job/94135710567

Image

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hi @alaahong! Thank you for the review!

I partly based my setup on PoiUtilsTest:test_customHeight_unsupportedType_false, which has exactly the same WARN. This is why I overlooked it.

One fix could be sheet.flushRows();. However, in my case, the handler does nothing with the cell beyond cell instanceof SXSSFCell, so I decided to fix it with Mockito.mock(SXSSFCell.class), and it also makes the code easier to read.

The one thing the mock gave up was coverage of the instanceof check itself, so I've pushed a follow-up test, afterCellDataConverted_ignoresNonSxssfCells, which runs the handler over a mocked HSSFCell and asserts the value is left untouched.

Please resolve the conversation if you accept this fix :)

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.

4 participants