docs: clarify config zero-value and clamp semantics#67
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the batch configuration documentation to clarify how zero and conflicting values are handled by the engine. It also introduces several contract-locking tests to verify zero-value round-tripping, extreme value handling in mutators, and concurrent read/write safety. The review feedback suggests improving the robustness of the mutator tests by initializing the configurations with non-zero dummy values, ensuring that the mutators can successfully overwrite existing non-zero values with zero.
| // Update: replaces all values at once. | ||
| cfgUpdate := NewDynamicConfig(nil) | ||
| cfgUpdate.Update(tc.values) | ||
| if got := cfgUpdate.Get(); got != tc.values { | ||
| t.Errorf("Update round-trip mismatch: expected %+v, got %+v", tc.values, got) | ||
| } | ||
|
|
||
| // UpdateBatchSize + UpdateTiming: the split setters must reach the | ||
| // same state, with no clamping or rejection at the config layer. | ||
| cfgSplit := NewDynamicConfig(nil) | ||
| cfgSplit.UpdateBatchSize(tc.values.MinItems, tc.values.MaxItems) | ||
| cfgSplit.UpdateTiming(tc.values.MinTime, tc.values.MaxTime) | ||
| if got := cfgSplit.Get(); got != tc.values { | ||
| t.Errorf("UpdateBatchSize/UpdateTiming round-trip mismatch: expected %+v, got %+v", tc.values, got) | ||
| } |
There was a problem hiding this comment.
The test currently initializes cfgUpdate and cfgSplit with nil (which defaults to all zero values). When testing with tc.values that contain zero values (like the "all zero" case), this doesn't actually verify that the mutators can overwrite existing non-zero values with zero. To make this contract-locking test robust, we should initialize the configs with non-zero dummy values first, ensuring that the update to zero is actually applied and verified.
dummy := ConfigValues{
MinItems: 99,
MaxItems: 99,
MinTime: 99 * time.Second,
MaxTime: 99 * time.Second,
}
// Update: replaces all values at once.
cfgUpdate := NewDynamicConfig(&dummy)
cfgUpdate.Update(tc.values)
if got := cfgUpdate.Get(); got != tc.values {
t.Errorf("Update round-trip mismatch: expected %+v, got %+v", tc.values, got)
}
// UpdateBatchSize + UpdateTiming: the split setters must reach the
// same state, with no clamping or rejection at the config layer.
cfgSplit := NewDynamicConfig(&dummy)
cfgSplit.UpdateBatchSize(tc.values.MinItems, tc.values.MaxItems)
cfgSplit.UpdateTiming(tc.values.MinTime, tc.values.MaxTime)
if got := cfgSplit.Get(); got != tc.values {
t.Errorf("UpdateBatchSize/UpdateTiming round-trip mismatch: expected %+v, got %+v", tc.values, got)
}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #67 +/- ##
=======================================
Coverage 96.73% 96.73%
=======================================
Files 12 12
Lines 368 368
=======================================
Hits 356 356
Misses 9 9
Partials 3 3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Correct and complete the ConfigValues / Config godoc to match the engine's actual behavior (batch.go fixConfig + waitForItems): - Config.Get: the min/max clamp applies only when the corresponding max is greater than zero (non-zero); a zero max is unset, so no clamping occurs. - MaxTime == 0 and MaxItems == 0 mean unset/disabled (no max-time flush, no max-count trigger) -- documented explicitly since zero is counter-intuitive. - MinTime == 0 means no minimum-time wait; MinItems == 0 is treated as 1 by the engine (at least one item per batch). Documentation only; no behavioral change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add contract-locking tests at the config-type level that assert existing behavior, so future regressions against the documented contract are caught: - ConstantConfig/DynamicConfig Get faithfully round-trips zero values (zero carries documented unset/disabled meaning; the config holder must not rewrite it -- that is the engine's job). - Update/UpdateBatchSize/UpdateTiming accept zero and extreme (max-uint64, max-duration, min>max) values without panicking or rejecting them (backwards-compat guard: no validation at the config layer). - DynamicConfig concurrent Get/Update/UpdateBatchSize/UpdateTiming under -race to lock thread-safety across every exported mutator. These pass immediately by design; no behavior changed. Tests stay at the config-type level and assert no batch-flush/engine behavior. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ba39c73 to
1705ef2
Compare
Summary
Documentation and contract-locking tests for
ConfigValues/Configsemantics that were previously unstated or misstated. No behavioral code change.Changes
Config.Getgodoc: the min→max clamp applies only when the corresponding max is> 0(matching the engine'sfixConfig); a zero max means "unset", no clamp. The previous wording implied the clamp was unconditional.MaxTime == 0/MaxItems == 0: documented as "unset / disabled" (no time or count trigger). Previously counter-intuitive (zero reads like "immediately") and undocumented.MinTime == 0(no minimum wait) andMinItems == 0(engine treats as 1) documented.Get()round-trips, mutators accept zero/extreme values without panic, concurrentGet/Updateunder-race.Testing
go test -race ./...green ·go vet/gofmtclean. The new tests assert existing behavior to guard against future regressions of the now-corrected contract.Backwards compatibility
Documentation plus additive tests only. No exported signature changed; no validation added that could reject existing inputs.
Part of a 5-PR set from a full-repo review; file-disjoint and independently mergeable in any order.
🤖 Generated with Claude Code