chore(groups): retarget the TASK_FORCE characterization tests at the engine that owns them - #653
Conversation
…engine that owns them
Wave D of the Agent / Group Agent review, first slice.
GroupConversationService carries ~27 private methods with ZERO production
callers -- pure delegators kept alive because ~34 getDeclaredMethod call
sites across seven test classes resolve against them. That inverts the
dependency: the tests pin a shim, not the path the engines actually take,
so PhaseExecutionEngine or TaskForceEngine could change how they call the
real method and every assertion would still pass.
This slice retargets the TASK_FORCE surface --
GroupConversationServiceTaskForceTest (all 20 tests) plus the two
recordTaskFailure tests in GroupConversationServiceHitlCoverage3Test -- at
TaskForceEngine and MemberTurnExecutor directly, and deletes the
recordTaskFailure delegator that pinning kept alive.
Two things the retarget surfaced, both illustrating the point:
- Three assertions described the reflection wrapper, not the code. They
asserted InvocationTargetException and unwrapped one getCause() layer to
reach the real exception. Called directly, the quota tests now assert
GroupDiscussionException with a QuotaExceededException cause -- the
actual contract, which the reflective form had obscured.
- A grep for "recordTaskFailure(" finds one test file; there were two. The
second builds the name as a STRING LITERAL for a file-local
method(name, params) helper. GroupConversationService's own comments warn
about exactly this ("a plain grep for one calling convention isn't enough
when sweeping for these") -- and the sweep hit it. Deleting the delegator
on the first grep's evidence broke the build; the string-literal site is
now retargeted too.
Scope: deliberately one surface, not all seven test classes. The remaining
reflection (the HITL cluster, the context-builder methods in
UncoveredBranchTest, resolveParticipants/extractResponse/failConversation
in GroupConversationServiceTest) is entangled with file-local helper
indirection a mechanical pass cannot safely rewrite -- an attempt to regex
through it produced a broken intermediate and was reverted. Each remaining
class is its own follow-up, following the pattern established here:
construct the real collaborator in setUp, leave the test bodies untouched.
562 tests green across GroupConversationService* and TaskForceEngine*.
|
Warning Review limit reached
Next review available in: 20 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
…characterization-tests # Conflicts: # docs/changelog.md
2 participants
Wave D of the Agent / Group Agent review (A #648, B #649, C #650, F #651, E #652) — first slice.
The problem
GroupConversationServicecarries ~27 private methods with zero production callers. They are pure delegators, kept alive because ~34getDeclaredMethodcall sites across seven test classes resolve against them — the file says so itself, repeatedly:That inverts the dependency. The tests pin a shim, not the path
executeTaskPhaseactually takes — soPhaseExecutionEngineorTaskForceEnginecould change how they call the real method and every assertion would still pass. It also blocks the Wave-R extraction from finishing: the facade cannot shrink while the tests hold it open.This slice
Retargets the TASK_FORCE surface at the engines that own it:
GroupConversationServiceTaskForceTest— all 20 tests, now callingTaskForceEngineandMemberTurnExecutordirectlyrecordTaskFailuretests inGroupConversationServiceHitlCoverage3TestrecordTaskFailuredelegator that pinning kept aliveTest bodies are untouched; only
setUpand the smallinvoke(...)helpers change. The pattern: construct the real collaborator insetUp, call it directly.Two things the retarget surfaced
Both illustrate why this matters rather than being incidental:
1. Three assertions described the reflection wrapper, not the code. They asserted
InvocationTargetExceptionand unwrapped onegetCause()layer to reach the real exception:Called directly, the quota tests now assert
GroupDiscussionExceptionwith aQuotaExceededExceptioncause — the actual contract, which the reflective form had obscured behind a property of how the test invoked the code.2. A grep for
recordTaskFailure(finds one test file. There were two. The second builds the method name as a string literal for a file-localmethod(name, params)helper, so no grep for the call syntax finds it.GroupConversationService's own comments warn about precisely this — "several through a third, file-local wrapper name — a plain grep for one calling convention isn't enough when sweeping for these" — and the sweep walked straight into it: deleting the delegator on the first grep's evidence broke the build. The string-literal site is retargeted too.Scope — deliberately one surface
Not all seven test classes. The remaining reflection (the HITL cluster across
HitlCoverage/HitlCoverage2/HitlCoverage3, the context-builder methods inUncoveredBranchTest, andresolveParticipants/extractResponse/failConversationinGroupConversationServiceTest) is entangled with file-local helper indirection that a mechanical pass cannot safely rewrite — an attempt to regex through it produced a broken intermediate and was reverted rather than shipped.Each remaining class is its own follow-up, following the pattern established here. Splitting it this way also keeps the diff reviewable and avoids colliding with the four other open PRs against these files.
Testing
562 tests green across
GroupConversationService*andTaskForceEngine*, with the delegator deleted.