test(amber): cover WorkflowExecutionService state events and teardown - #8035
Open
aglinxinyuan wants to merge 1 commit into
Open
test(amber): cover WorkflowExecutionService state events and teardown#8035aglinxinyuan wants to merge 1 commit into
aglinxinyuan wants to merge 1 commit into
Conversation
Contributor
Automated Reviewer SuggestionsBased on the
|
Contributor
✅ No material benchmark regressions detected🟢 4 better · 🔴 0 worse · ⚪ 11 noise (<±5%) · 0 without baseline
Baseline detailsLatest main
Raw CSVconfig_idx,batch_size,schema_width,string_len,num_batches,total_ms,total_tuples,total_bytes,tuples_per_sec,mb_per_sec,lat_p50_us,lat_p95_us,lat_p99_us
0,10,10,64,20,335.65,200,128000,596,0.364,16353.27,20841.91,20841.91
1,100,10,64,20,1712.61,2000,1280000,1168,0.713,82501.04,114692.88,114692.88
2,1000,10,64,20,14004.26,20000,12800000,1428,0.872,698561.07,739775.69,739775.69 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8035 +/- ##
============================================
+ Coverage 93.33% 93.35% +0.01%
- Complexity 4692 4698 +6
============================================
Files 1179 1179
Lines 47713 47713
Branches 5314 5314
============================================
+ Hits 44534 44543 +9
+ Misses 1730 1721 -9
Partials 1449 1449
*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:
|
Comment on lines
+160
to
+164
| override protected def beforeAll(): Unit = { | ||
| initializeDBAndReplaceDSLContext() | ||
|
|
||
| val user = new User | ||
| user.setUid(testUid) |
Comment on lines
+215
to
+220
| override protected def afterAll(): Unit = { | ||
| try { | ||
| TestKit.shutdownActorSystem(system) | ||
| closeConnectionPool() | ||
| } finally super.afterAll() | ||
| } |
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?
WorkflowExecutionServiceSpecgoes from 3 tests to 11. The three existing tests stop at construction; the new ones drive the metadata-store handler and the teardown path.+10 fully-covered lines and +5 branch arms. The two metrics differ by one because line 179 was already line-hit and flips only by completing its second arm — they are not interchangeable, so both are given.
Of the +10, six are logic and four are not. Lines 95, 179, 181–184 are the recovery-banner arm and the four
unsubscribeAllcalls. Lines 101, 107, 108 and 109 are scalac-generated public accessor pairs (confirmed withjavap -p -l) that move because the teardown test assigns those vars from outside. Those four are a real consequence of driving teardown rather than coverage farming, but they are not logic and I would rather split them out than present all ten as equivalent.What the reviewers found
Two independent adversarial reviewers ran against the first draft. Six of eight fresh mutants survived it. The three worth naming:
workflowContext.workflowSettings = request.workflowSettings(line 90) could be deleted outright and all 8 tests still passed. Every test executes that line, so it was fully covered and entirely unconstrained. It is not cosmetic:outputPortsNeedingStorageis whatCostBasedScheduleGeneratorreads to decide which output ports get materialized, anddataTransferBatchSize/executionModefeed the resource allocator. Dropping it silently reverts every execution to the default settings while the request's are ignored.isRecoveringfalse → true, so weakeningnewState.isRecovering != oldState.isRecoveringto a barenewState.isRecoveringsurvived. That mutant is the mirror of the failure the test's own comment claimed to guard: an update that clears the flag emits no event, so the frontend's Recovering banner never comes down.fatalErrorsguard could be deleted wholesale and nothing failed.All are now killed by a named test. The published mutation table was also re-run from scratch, one mutant at a time, because one row's failure message had been copy-pasted from another row.
Verification
Measured with an identical suite-name filter on both sides, one fresh sbt JVM per measurement,
rm -rfof the jacoco dir between runs, counters read per-line out ofjacoco.xml.An independent measurer re-derived the figure a second way — over the whole amber unit module, the actual CI scope, rather than the six-suite filter — and got byte-identical per-line data on both sides. That independently confirms the six-suite list is complete and that 26 → 36 is what Codecov will show.
Two corrections that measurement forced, both worth stating:
mi=4, ci=1, because the method'sreturninstruction is attributed to it. It is genuinely executed only now.Full amber unit scope, both sides: 190 suites, the same 7 pre-existing Windows-only failures by name, zero new. The only per-suite change anywhere is this spec going 3 → 11 tests, so the new
beforeAllinserts do not leak —MockTexeraDBgives each suite its own database, and ids 9207–9210 are unique across the repo because the Iceberg statistics URI is machine-global.Deliberately not included
executeWorkflow's live-runtime half — the 44-line hole — has no seam. Line 124 callsComputingUnitMaster.createAmberRuntime, which builds anAmberClientoverAmberRuntime.actorSystem, a JVM-globalprivate varthat nothing in unit scope initialises. Injecting a seam would be a production change.Lines 113/114/115 are refused for a sharper reason: JaCoCo probes the
tryblock only at its exit, so they flip only if compile andWorkflow.fromCompilationResultboth succeed — which falls straight intocreateAmberRuntime. Whether that is survivable depends onClientEventSpechaving restored the global tonullin itsafterAll.WorkflowServiceSpec's own header documents this cross-suite hazard as its reason for deliberately steering into a compile failure instead. Cementing an accident is worse than leaving three lines.Three mutants are reported as live rather than dropped: reordering
client.shutdown()against the fourunsubscribeAllcalls (order is not a stated contract), and two whose only kill would be to assert current behaviour that is arguably wrong — addingFAILEDto the stuck-banner guard, and pinning which field the teardown guard reads, where the only discriminating state currently NPEs.No production file is touched.
Any related issues, documentation, discussions?
Closes #8033
How was this PR tested?
WorkflowExecutionService/Test/scalafmtCheckandWorkflowExecutionService/Test/scalafix --checkboth pass.Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 5)