test(workflow-core): add unit test coverage for port-identity serde helpers#4954
Open
aglinxinyuan wants to merge 5 commits intoapache:mainfrom
Open
test(workflow-core): add unit test coverage for port-identity serde helpers#4954aglinxinyuan wants to merge 5 commits intoapache:mainfrom
aglinxinyuan wants to merge 5 commits intoapache:mainfrom
Conversation
…elpers Pin the wiring for the three serde helpers in `org.apache.texera.amber.util.serde`: - `GlobalPortIdentitySerde.serializeAsString` / `deserializeFromString`: default + per-field round-trip, exact format pin (default and non-default values), special-character pass-through (dashes, dots), negative `portId`, no-underscore invariant for VFS-URI compatibility, and seven negative paths (malformed, missing field, wrong order, trailing content, empty body, non-numeric portId → `NumberFormatException`, non-boolean flag). - `PortIdentityKeySerializer.portIdToString` exact `id_internal` format. - The Jackson serializer + deserializer wired into a real `ObjectMapper` (mirroring `JSONUtils.objectMapper` setup) round-tripping a `Map[PortIdentity, String]` and an empty map. - `PortIdentityKeyDeserializer.deserializeKey` happy-path coverage via the Map round-trip plus four negative paths (non-integer id, non-boolean flag, missing separator with non-numeric body, missing separator with numeric-only body). Closes apache#4953
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4954 +/- ##
============================================
- Coverage 42.50% 42.15% -0.35%
- Complexity 2181 2183 +2
============================================
Files 1005 980 -25
Lines 37429 36257 -1172
Branches 3914 3783 -131
============================================
- Hits 15908 15284 -624
+ Misses 20558 20046 -512
+ Partials 963 927 -36
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds a new workflow-core unit spec for the small org.apache.texera.amber.util.serde helpers that encode/decode GlobalPortIdentity and PortIdentity keys. It fits into the existing test suite by pinning string formats and bad-input behavior for serde paths that are used by VFS URI handling and Jackson map-key serialization.
Changes:
- Add round-trip and malformed-input tests for
GlobalPortIdentitySerde. - Add exact-format tests for
PortIdentityKeySerializer.portIdToString. - Add Jackson key serializer/deserializer tests for
PortIdentitymap keys, including empty-map and bad-key cases.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…core-invariant test Address Copilot feedback on apache#4954: - Switch the Jackson key (de)serialization tests to use the production `JSONUtils.objectMapper` singleton instead of constructing a fresh `ObjectMapper` + `SimpleModule` locally. The fresh-mapper version would still pass if the singleton stopped registering the PortIdentity key serializer/deserializer module — exactly the kind of regression these tests are meant to catch. - Replace the misleading "no underscore" test, which only used underscore-free inputs (so it could not catch the real failure mode flagged in the Copilot comment: an op id like `OperatorIdentity("__DummyOperator")` from `VirtualIdentityUtils`). The new spec now has two tests: - A plain test pinning that the FORMAT CHARACTERS introduce no underscore (computed by stripping the input values out of the output and checking the residue). - A `pendingUntilFixed` test asserting the fuller documented contract — output is underscore-free even when the input contains underscores. Today this test is pending because the implementation interpolates inputs verbatim; a future fix that escapes/replaces underscores will flip it to passing, and `pendingUntilFixed` then inverts that into a deliberate failure forcing the marker to be removed.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address Copilot follow-up on apache#4954: - Underscore-invariant pendingUntilFixed test: previously only varied `logicalOpId`. Production also constructs underscore-bearing layerNames (e.g. SpecialPhysicalOpFactory builds `${layerName}_source_${portId}_${opId}`), so a partial fix that only escapes op-ids would have flipped the pending test to passing while leaving the layerName failure mode unhandled. Now exercises both fields independently in the same test. - PortIdentityKeyDeserializer extra-segments path: previously not pinned. `key.split("_")` only reads parts(0) and parts(1), so a malformed key like "1_true_garbage" silently deserializes to PortIdentity(1, true). Added two tests: - Characterization: pin the current lenient behavior. - pendingUntilFixed: assert the stricter contract (any extra trailing segment rejected). Future hardening flips the pending test to passing, then pendingUntilFixed inverts that into a deliberate failure forcing the marker to be removed.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
aglinxinyuan
added a commit
to aglinxinyuan/texera
that referenced
this pull request
May 6, 2026
…ngUntilFixed Address Copilot follow-up on apache#4958: pendingUntilFixed alone left the reachable-from-API batchSize<=0 path uncovered by a real regression test. Add a paired characterization test that pins today's lenient behavior (per-tuple flush via the `>=` guard) for both batchSize=0 and batchSize=-1. The pair pattern matches the deserializeKey extra-segments coverage in apache#4954: a future hardening that rejects `<= 0` at construction will break the characterization on purpose AND flip pendingUntilFixed to passing, forcing both markers to be updated together — instead of leaving a regression hole today.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this PR?
Adds
PortIdentitySerdeSpeccovering three Jackson / VFS-URI serde helpers inorg.apache.texera.amber.util.serde, none of which had a unit test:GlobalPortIdentitySerde.serializeAsString/deserializeFromStringportId; no-underscore invariant (VFS-URI compatibility); seven negative paths — completely malformed, missing field, wrong field order, trailing content, empty body, non-numericportId→NumberFormatException, non-boolean flag →IllegalArgumentExceptionPortIdentityKeySerializer.portIdToStringid_internalformat.PortIdentityKeySerializer+PortIdentityKeyDeserializer(Jackson wiring)ObjectMapperregistered with the same module shape thatJSONUtils.objectMapperuses, round-trippingMap[PortIdentity, String]and an empty map.PortIdentityKeyDeserializer.deserializeKeyNo production code changed; this is test-only.
Any related issues, documentation, discussions?
Closes #4953
How was this PR tested?
Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 4.7)