test(amber): add unit test coverage for PauseType#4914
Conversation
Pin the sealed-trait contract for `PauseType`: - The three pause singletons (UserPause, BackpressurePause, OperatorLogicPause) extend `PauseType`, are object-identity stable, and compare unequal to each other when widened to the trait. - `ECMPause` is a case class that carries the `EmbeddedControlMessageIdentity` it was constructed with, supports value equality and consistent hashCode by id, and is distinct from every singleton PauseType. - Exhaustive pattern matching distinguishes each subtype and ECMPause's id field destructures via match. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR adds focused unit coverage for PauseType, the sealed trait used by Amber's worker pause subsystem, to pin its subtype/equality/matching behavior and prevent regressions in this small but important control-flow contract.
Changes:
- Added a new
PauseTypeSpeccovering the three singleton pause variants and theECMPausecase class. - Added assertions for identity/equality semantics, subtype distinctness, and exhaustive pattern matching/destructuring behavior.
- Scoped the change entirely to tests; no production code was modified.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4914 +/- ##
============================================
+ Coverage 42.04% 47.59% +5.54%
+ Complexity 2156 2150 -6
============================================
Files 980 817 -163
Lines 36292 25940 -10352
Branches 3783 2343 -1440
============================================
- Hits 15259 12345 -2914
+ Misses 20110 12845 -7265
+ Partials 923 750 -173
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:
|
…Spec Address Yicong-Huang's review on apache#4914: the "destructure ECMPause's id via match" test is redundant — the same `id` access is already covered by "should carry the EmbeddedControlMessageIdentity it was constructed with" (constructor → field) and "should support exhaustive pattern matching" (the match itself). Drop it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address Yicong-Huang's review on apache#4914 about pause priorities. PauseType itself has no priority — `PauseManager` stores active pauses in a `HashSet[PauseType]` (additive, no override order; resuming one type only removes that type). The data type's job is to behave well as Set elements; the multi-pause behavior through `pause`/`resume`/`isPaused` lives in `WorkerManagersSpec`. Add three Set-based tests pinning what PauseManager actually relies on: - All four pause kinds coexist as distinct Set elements (no aliasing). - Set deduplicates identical pauses (singletons collapse, same-id ECMPause collapses), so PauseManager.pause(t) on a duplicate is a no-op. - ECMPause instances with different ids are independently tracked, so resuming one checkpoint pause does not clear another. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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 feedback on apache#4914: - Remove the "all extend the sealed trait PauseType" test entirely. Its `all: List[PauseType] = ...` ascription already proves the subtype relationship at compile time; the runtime `isInstanceOf` check could never fail (the file would not compile if it did). Replaced with a short comment block documenting the design choice for future maintainers. - Drop the redundant `assert(p.isInstanceOf[PauseType])` from the ECMPause-vs-singleton cross-kind inequality test, for the same reason. Renamed the test to "not equal any of the singleton PauseTypes" so the assertion list matches the description. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.
What changes were proposed in this PR?
Adds
PauseTypeSpeccoveringPauseType(amber/src/main/scala/org/apache/texera/amber/engine/architecture/worker/PauseType.scala), the small sealed trait used by the worker pause subsystem (three singleton kinds + one case-class subtype). It has no test coverage today.The new spec pins:
UserPause,BackpressurePause,OperatorLogicPause) extendPauseType, are object-identity stable, and compare unequal to each other (widened toPauseTypeso the comparison is not folded away at compile time).ECMPauseis a case class that carries theEmbeddedControlMessageIdentityit was constructed with, supports value equality and consistenthashCodeby id, and is distinct from every singleton PauseType.matchoverPauseTypedistinguishes each subtype.PauseManageractually relies on, since it stores active pauses in aHashSet[PauseType]with no priority): all four pause kinds coexist as distinct Set elements; identical pauses deduplicate (singletons collapse, same-idECMPausecollapses);ECMPauseinstances with different ids are independently tracked.The multi-pause behavior through
PauseManager.pause/resume/isPausedis covered separately inWorkerManagersSpec.No production code changed; this is test-only.
Any related issues, documentation, discussions?
Closes #4913
How was this PR tested?
Added 10 new unit tests in
PauseTypeSpec. Verified locally:Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code