test(amber): add unit test coverage for ClientEvent sealed-trait family#5440
Merged
Merged
Conversation
Pin the data contract and Pekko Serialization wire-path for all nine ClientEvent subtypes used in the controller→client channel. Per-subtype: each constructor exposes its fields verbatim; case-class equality + hashCode work as expected; the subtype extends ClientEvent and (via the trait) WorkflowFIFOMessagePayload — the membership pin is compile-time-enforced by `: ClientEvent` ascriptions. Pekko Serialization: round-trip via AmberRuntime.serde (the production wire path) for every subtype: - ExecutionStateUpdate(state) preserves the WorkflowAggregatedState - ExecutionStatsUpdate(operatorMetrics) preserves both populated and empty operatorMetrics maps - RuntimeStatisticsPersist preserves operatorMetrics - ReportCurrentProcessingTuple preserves operatorID and the tuple array length (arrays compared element-wise — case-class equality on Array is reference-based, so length is the right pin without over-constraining serializer choice) - WorkerAssignmentUpdate preserves workerMapping (populated and empty) - FatalError preserves both the exception (message + class) and the optional fromActor; pin that fromActor defaults to None - UpdateExecutorCompleted preserves the actor id - ReplayStatusUpdate round-trips both status = true and status = false - WorkflowRecoveryStatus round-trips both isRecovering = true and isRecovering = false Cross-subtype: ExecutionStatsUpdate and RuntimeStatisticsPersist share the Map[String, OperatorMetrics] field shape; pin that case-class equality still distinguishes them across types. The suite owns a suite-local Pekko ActorSystem injected into AmberRuntime via reflection, torn down via TestKit.shutdownActorSystem in afterAll (same pattern as CheckpointSubsystemSpec). Closes apache#5437
Contributor
|
good timing! I am adding more ClientEvents, for a self contained amber. |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a ScalaTest suite (ClientEventSpec) to pin the data contract and Pekko serialization round-trip behavior for the controller→client ClientEvent sealed-trait family using the production serialization entrypoint (AmberRuntime.serde).
Changes:
- Introduces
ClientEventSpecthat round-trips eachClientEventsubtype throughAmberRuntime.serde. - Adds checks for subtype membership in
WorkflowFIFOMessagePayloadand cross-subtype inequality for same-shaped payloads. - Uses a suite-local Pekko
ActorSysteminjected intoAmberRuntimevia reflection and torn down inafterAll.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Yicong-Huang
approved these changes
Jun 7, 2026
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5440 +/- ##
============================================
- Coverage 51.71% 51.69% -0.03%
+ Complexity 2461 2457 -4
============================================
Files 1067 1067
Lines 41254 41254
Branches 4437 4437
============================================
- Hits 21336 21327 -9
- Misses 18668 18670 +2
- Partials 1250 1257 +7
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Address three threads on apache#5440: - Yicong-Huang: drop the runtime "all extend WorkflowFIFOMessagePayload" sweep — the trait inheritance is enforced at compile time by `trait ClientEvent extends WorkflowFIFOMessagePayload` plus the `: ClientEvent` ascriptions used in the per-subtype tests below. A runtime isInstanceOf check would be tautological and would need to be edited every time a new subtype is added. - Copilot: the ReportCurrentProcessingTuple round-trip previously only exercised an empty array, so the (Tuple, AVI) element survival contract wasn't actually pinned (and the comment lied about "element-by-element verification"). Build a real single-element array fixture from a tiny Schema/Tuple, round-trip, and verify schema + field value + AVI all survive. Added a separate empty- array test for the degenerate case. - Copilot: the FatalError default-None test only checked the constructor default, not the wire-path. Combine into one test that pins both the default and the round-trip preservation of None + exception (by message + class) — a regression mishandling missing fromActor on the receive side would now surface.
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
ClientEventSpeccovering all nine subtypes of the controller→clientClientEventfamily. The trait extendsWorkflowFIFOMessagePayloadand flows over Pekko, so the spec round-trips each subtype via the production wire path (AmberRuntime.serde).ExecutionStateUpdatestate; round-trips viaAmberRuntime.serde; case-class equality + hashCode distinguish different states.ExecutionStatsUpdateoperatorMetrics; round-trip preserves both populated and empty metrics maps.RuntimeStatisticsPersistoperatorMetrics.ReportCurrentProcessingTupleoperatorIDand theArray[(Tuple, AVI)]tuple field; round-trip preserves operatorID and array length (Array equality is reference-based at the case-class level, so length is the right pin without over-constraining serializer choice).WorkerAssignmentUpdateworkerMapping; round-trip preserves both populated and empty mappings.FatalErrorfromActortoNone; explicitSome(actorId)round-trips; the carriedThrowablesurvives by message + class (Throwable equality is reference-based, so we don't pin the instance).UpdateExecutorCompletedReplayStatusUpdatestatus = trueandstatus = falseindependently.WorkflowRecoveryStatusisRecovering = trueandisRecovering = falseindependently.ExecutionStatsUpdateandRuntimeStatisticsPersistshare theMap[String, OperatorMetrics]field shape; case-class equality still distinguishes them across types.Membership in
ClientEventand (transitively)WorkflowFIFOMessagePayloadis enforced at compile time by: ClientEventascriptions plus a runtimeisInstanceOf[WorkflowFIFOMessagePayload]sweep — a regression that demoted the trait offWorkflowFIFOMessagePayloadwould surface immediately.The suite owns a suite-local Pekko
ActorSysteminjected intoAmberRuntimevia reflection, torn down viaTestKit.shutdownActorSysteminafterAll(same pattern asCheckpointSubsystemSpec).No production code changed; this is test-only.
Any related issues, documentation, discussions?
Closes #5437
How was this PR tested?
Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 4.7)