Simple Partition Scaler test coverage#11076
Conversation
newSimplePartitionScaler never initialized the trackers map, so the first time OnTasks ran with the scaler enabled and an Up/Down window configured, getTracker's map write panicked with "assignment to entry in nil map" and crash-looped the matching service. Initialize trackers in the constructor and add a regression test that drives the real scaler through that path (the existing scale_manager tests only exercise a mock). Broader behavioral coverage follows in a separate PR. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Follow-up to the nil-map fix. The scaler's real OnTasks/updateAddTarget paths were only ~64-88% covered; scale-down, the Min/Max bounds, and the Up/Down hysteresis deadband were entirely untested (the existing scale_manager tests only drive a mock). Add tests covering the disabled/fixed/no-window short-circuits, scale-up, scale-down (including the floor at one partition), Min and Max bounds, the hysteresis deadband, PrivateState round-trip across calls, multiple windows gating a decision, and the factory/lifecycle wiring. This takes OnTasks and updateAddTarget to 100% statement coverage. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| dec := onTasksLoop(scaler, 1, 3, 10, nil, 0) | ||
| require.False(t, dec.NoChange) | ||
| require.Equal(t, 0, dec.NewTarget) | ||
| require.Empty(t, scaler.trackers, "disabled scaler must not create trackers") |
There was a problem hiding this comment.
this isn't part of any spec or contract. tests shouldn't care about internal implementation details like this, it just makes them more brittle for no reason
| dec := onTasksLoop(scaler, 100, 1, 10, ts, 100*time.Millisecond) | ||
| require.False(t, dec.NoChange) | ||
| require.Equal(t, 10, dec.NewTarget) | ||
| require.NotNil(t, dec.PrivateState, "a real decision carries private state") |
There was a problem hiding this comment.
I'm not sure about this either. it's private. shouldn't we just test the outcomes?
| // Current target 20, only 300 tasks/s against TargetRate 100 => target 3. | ||
| dec := onTasksLoop(scaler, 300, 20, 1, ts, 100*time.Millisecond) | ||
| require.False(t, dec.NoChange) | ||
| require.Equal(t, 3, dec.NewTarget) // TODO: this works with 300 tasks and 1 repetition, not sure why it is 1 with 30 tasks 10 reps (in 1 second) |
There was a problem hiding this comment.
maybe it's that the earliest window will look like 30 tasks in 1 second, and only grows to 300 in 1 second at the end of the sequence? priming makes the first call meaningful when here you may not want it to be
| // TestSimplePartitionScalerMultipleWindows verifies that distinct windows create | ||
| // distinct trackers and that a decision is produced only once every window is | ||
| // full (the longest window gates the first decision). |
There was a problem hiding this comment.
this is true, but I think the more interesting thing to test is different windows with different thresholds (targets)
| } | ||
|
|
||
| // onTasksLoop calls OnTasks with the same numTasks for numRepetitions. | ||
| func onTasksLoop(scaler *simplePartitionScaler, numTasks, currentTarget, numRepetitions int, ts *clock.EventTimeSource, delay time.Duration) (dec PartitionScalerDecision) { |
There was a problem hiding this comment.
I feel like the calls might be easier to read if numTasks, numReps, and delay were next to each other in the argument list
| }, | ||
| }) | ||
|
|
||
| dec := onTasksLoop(scaler, 150, 2, 2, ts, 100*time.Millisecond) // TODO: target goes to 1 after third rep, why? |
There was a problem hiding this comment.
I guess these isolated tests do have value, but I feel like it might be easier to understand and also more valuable to have one test that runs through a sequence with one scaler with a full representative configuration: add tasks at rate r0 for x seconds, check the last output, add at higher rate r1 for x seconds, check that ups applied, add at lower rate r2 for x/2 seconds, check no movement due to deadband, continue for another x/2 seconds, check that downs finally applied, add at rate r3 that would imply a target < min, check that min applied, add at rate r4 that would imply target > max, check that max applied. also check deadband on the way up, multiple windows, etc.
2 participants
What changed?
Simple Partition Scaler was mocked only and did not have tests for these paths.
Why?
We need tests :) Removed nil pointer test because it is tested implicitly.
How did you test it?
Potential risks
Only tests, no risk
Note
Low Risk
Test-only changes to matching partition scaler; no production code paths modified.
Overview
Replaces the single enabled-does-not-panic check in
simple_partition_scaler_test.gowith a broader unit suite forsimplePartitionScaler.Adds shared helpers (
newTestScaler,primedScaler,onTasksLoop) that useEventTimeSourceand a fixedscalerWindowso rate-based decisions are deterministic. New cases cover the factory (New/Stop), disabled and Fixed short-circuits, enabled with no Up/Down windows, scale up and down (including floor at 1), Min/Max bounds, hysteresis between Up/Down thresholds, and multiple measurement windows (longest window gates the first decision).The old nil-map panic test is dropped in favor of implicit coverage via tracker creation in the new tests. A couple of TODO comments note flaky or surprising behavior in scale-down and hysteresis loops.
Reviewed by Cursor Bugbot for commit 732974c. Bugbot is set up for automated code reviews on this repo. Configure here.