fleetd #426: pin FleetHealthMonitor.coverage and its HealthCoverageSource call site
FleetHealthMonitor.coverage had zero references in the test tree — not the method, not either output string, not the field it populates. Inverting `enabled`, swapping "full"/"detection-only", or breaking the argument pairing at the HealthCoverageSource call site in Fleetd.java all shipped a green build. Extract the HealthCoverageSource lambda out of Fleetd.main into a package-private static factory (healthCoverageSource(ConfigRef)), the same shape capacitySource/quarantineSource already use for the identical argument-pairing risk (fleetd #415). #407's "keep the config invalid, assert on the log line before validateAll() throws" option does not apply here: this call site is built well after validateAll() and after a real herdr socket connect, so driving it through a real Fleetd.main would require the socket I/O this ticket's tests must not do. Add FleetHealthMonitorCoverageTest (the three-branch method itself) and FleetdHealthCoverageSourceWiringTest (the call site, via a real FleetConfig.load + ConfigRef against @TempDir fixtures, including a hot notifications-reload case). Output strings are unchanged — "detection-only" is still what a live fleet_list reports today. Measured: all three mutations killed by the new tests.
This commit is contained in:
@@ -696,11 +696,7 @@ public final class Fleetd {
|
||||
FleetMcp mcp = new FleetMcp(messages, workers, sessions, identity, presence,
|
||||
primaryRegistry, callers, FleetMcp.AuthorizationMode.ENFORCED, metrics,
|
||||
capacitySource(config, cfg, profile -> liveCountRef.get().apply(profile)),
|
||||
new FleetMcp.HealthCoverageSource(() -> {
|
||||
var health = config.get().health();
|
||||
return FleetHealthMonitor.coverage(health != null && health.isEnabled(),
|
||||
health != null && health.notifications() != null && health.notifications().configured());
|
||||
}),
|
||||
healthCoverageSource(config),
|
||||
quarantineSource,
|
||||
leadMailbox,
|
||||
outageSource,
|
||||
@@ -1038,6 +1034,38 @@ public final class Fleetd {
|
||||
cfg.profiles()::keySet, System::nanoTime);
|
||||
}
|
||||
|
||||
/**
|
||||
* fleetd #426: package-private factory for {@code fleet_list}'s {@code healthCoverage} source,
|
||||
* extracted out of {@code main} for the same reason {@link #capacitySource} and {@link
|
||||
* #quarantineSource} were — and the same reason {@link #exhaustedPatternCoverageLine}/{@link
|
||||
* #errorPatternCoverageLine} exist: {@link FleetHealthMonitor#coverage}'s three-branch method
|
||||
* is easy to pin directly (a plain {@code (boolean, boolean) -> String} call), but that proves
|
||||
* nothing about whether <em>this call site</em> pairs the right boolean with the right meaning.
|
||||
* fleetd #415's measured lesson is the reason this matters here — swapping the two arguments at
|
||||
* a call site like this one compiled clean and left the full suite green, because every existing
|
||||
* test exercised the method in both directions without ever exercising the pairing.
|
||||
*
|
||||
* <p>{@code #407}'s "keep the config invalid, assert on the log line before the throw" option
|
||||
* does not apply to this call site: the five reporters #407 covers all run in {@code main}
|
||||
* <em>before</em> {@code cfg.validateAll()} (line ~171), so an invalid config still exercises
|
||||
* them. This call site is built during {@code FleetMcp} construction, which runs only after
|
||||
* {@code UnixSocketHerdrClient.connect} has already opened a real herdr socket (line ~188) —
|
||||
* reaching it at all means main already performed real I/O, which the no-socket constraint on
|
||||
* this ticket rules out. So the pairing is pinned by extracting it to this directly-callable
|
||||
* factory instead, the same shape {@link #capacitySource}/{@link #quarantineSource} already use.
|
||||
*
|
||||
* <p>Reads {@code config.get().health()} live (health.notifications is a {@code SPLIT_KEYS}
|
||||
* entry — see {@link ConfigRef#SPLIT_KEYS}), so a hot-reloaded notifications block changes what
|
||||
* {@code fleet_list} reports without a restart, exactly like {@link #capacitySource}'s maxLoad.
|
||||
*/
|
||||
static FleetMcp.HealthCoverageSource healthCoverageSource(ConfigRef config) {
|
||||
return new FleetMcp.HealthCoverageSource(() -> {
|
||||
var health = config.get().health();
|
||||
return FleetHealthMonitor.coverage(health != null && health.isEnabled(),
|
||||
health != null && health.notifications() != null && health.notifications().configured());
|
||||
});
|
||||
}
|
||||
|
||||
/**
|
||||
* fleetd #248: package-private factory for the member worktree/branch lookup {@link
|
||||
* CompletionResolver} uses to name a fallback report's worktree and branch (fleetd#241).
|
||||
|
||||
@@ -0,0 +1,160 @@
|
||||
package dev.ltms.fleet;
|
||||
|
||||
import dev.ltms.fleet.config.ConfigRef;
|
||||
import dev.ltms.fleet.config.FleetConfig;
|
||||
import dev.ltms.fleet.mcp.FleetMcp;
|
||||
import org.junit.jupiter.api.DisplayName;
|
||||
import org.junit.jupiter.api.Test;
|
||||
import org.junit.jupiter.api.io.TempDir;
|
||||
|
||||
import java.nio.file.Files;
|
||||
import java.nio.file.Path;
|
||||
|
||||
import static org.junit.jupiter.api.Assertions.assertEquals;
|
||||
import static org.junit.jupiter.api.Assertions.assertTrue;
|
||||
|
||||
/**
|
||||
* fleetd #426: {@code FleetHealthMonitor.coverage} had zero references anywhere in the test
|
||||
* tree — not the method, not either output string, not the field it populates. {@code
|
||||
* FleetHealthMonitorCoverageTest} (package {@code dev.ltms.fleet.health}) pins the three-branch
|
||||
* method itself; that is the easy half.
|
||||
*
|
||||
* <p>The half that actually matters is this one: {@link Fleetd#healthCoverageSource} is the exact
|
||||
* call site {@code Fleetd.main} wires into {@code FleetMcp}'s constructor, and it is what feeds
|
||||
* {@code fleet_list}'s {@code healthCoverage} field (see {@code FleetMcp#listFleet}'s {@code
|
||||
* result.put("healthCoverage", healthCoverage.value().get())}). Measured precedent on fleetd #423
|
||||
* (for #415): swapping two arguments at a call site like this one — recreating #415's defect with
|
||||
* the keys exchanged — compiled with 0 errors and ran the ENTIRE suite (1506 tests) green. A
|
||||
* thoroughly-tested method proves nothing about whether the call site pairs its arguments correctly;
|
||||
* only a test that drives the call site itself can catch that.
|
||||
*
|
||||
* <p><strong>Why this is not driven through a real {@code Fleetd.main} the way #407 drives its five
|
||||
* reporters</strong> (keep the config invalid, assert on the log line emitted before {@code
|
||||
* cfg.validateAll()} throws): all five of #407's reporters run in {@code main} before {@code
|
||||
* validateAll()} (line ~171). {@link Fleetd#healthCoverageSource} is built during {@code FleetMcp}
|
||||
* construction, which happens only after {@code UnixSocketHerdrClient.connect} has already opened a
|
||||
* real herdr socket (line ~188) and after {@code sessions}/{@code workers} are constructed. Reaching
|
||||
* this call site by actually running {@code main} would require a real socket connect — banned by
|
||||
* this ticket's hard constraints — so #407's option 1 does not apply here. Instead {@link
|
||||
* Fleetd#healthCoverageSource} is extracted to a directly-callable package-private factory, the same
|
||||
* shape {@link Fleetd#capacitySource} and {@link Fleetd#quarantineSource} already use for the same
|
||||
* reason (see {@code FleetdCapacitySourceWiringTest}, the direct precedent this test follows).
|
||||
*
|
||||
* <p>Uses a real {@link FleetConfig#load} + {@link ConfigRef} (no socket, no port bind, no spawn,
|
||||
* nothing written outside {@code @TempDir}) so the fixture goes through the actual YAML parser and
|
||||
* {@code FleetConfig.Health}/{@code Notifications} records, not a hand-built stand-in that could
|
||||
* silently drift from what the parser actually produces.
|
||||
*/
|
||||
class FleetdHealthCoverageSourceWiringTest {
|
||||
|
||||
private static final String BASE = """
|
||||
bind:
|
||||
host: 127.0.0.1
|
||||
port: 8765
|
||||
herdrSocket: ~/.config/herdr/herdr.sock
|
||||
""";
|
||||
|
||||
private static final String HEALTH_DISABLED_WITH_WEBHOOK = BASE + """
|
||||
health:
|
||||
enabled: false
|
||||
notifications:
|
||||
mode: webhook
|
||||
""";
|
||||
|
||||
private static final String HEALTH_DETECTION_ONLY = BASE + """
|
||||
health:
|
||||
enabled: true
|
||||
""";
|
||||
|
||||
private static final String HEALTH_FULL = BASE + """
|
||||
health:
|
||||
enabled: true
|
||||
notifications:
|
||||
mode: webhook
|
||||
""";
|
||||
|
||||
private static final String HEALTH_ABSENT = BASE;
|
||||
|
||||
@Test
|
||||
@DisplayName("enabled: false reports off, even with a webhook configured")
|
||||
void disabledHealthReportsOff(@TempDir Path dir) throws Exception {
|
||||
FleetMcp.HealthCoverageSource source = sourceFor(dir, HEALTH_DISABLED_WITH_WEBHOOK);
|
||||
|
||||
assertEquals("off", source.value().get(),
|
||||
"health.enabled: false must report 'off' regardless of notifications — flipping "
|
||||
+ "the 'enabled' argument at the HealthCoverageSource call site would report "
|
||||
+ "'full' here instead");
|
||||
}
|
||||
|
||||
@Test
|
||||
@DisplayName("enabled with no notifications reports detection-only")
|
||||
void enabledWithoutNotificationsReportsDetectionOnly(@TempDir Path dir) throws Exception {
|
||||
FleetMcp.HealthCoverageSource source = sourceFor(dir, HEALTH_DETECTION_ONLY);
|
||||
|
||||
assertEquals("detection-only", source.value().get());
|
||||
}
|
||||
|
||||
@Test
|
||||
@DisplayName("enabled with a webhook configured reports full")
|
||||
void enabledWithNotificationsReportsFull(@TempDir Path dir) throws Exception {
|
||||
FleetMcp.HealthCoverageSource source = sourceFor(dir, HEALTH_FULL);
|
||||
|
||||
assertEquals("full", source.value().get(),
|
||||
"health.enabled: true with notifications.mode: webhook must report 'full' — "
|
||||
+ "swapping 'full' and 'detection-only' at the call site, or breaking the "
|
||||
+ "enabled/notificationConfigured argument pairing, would report "
|
||||
+ "'detection-only' here instead");
|
||||
}
|
||||
|
||||
@Test
|
||||
@DisplayName("an absent health: block reports off")
|
||||
void absentHealthBlockReportsOff(@TempDir Path dir) throws Exception {
|
||||
FleetMcp.HealthCoverageSource source = sourceFor(dir, HEALTH_ABSENT);
|
||||
|
||||
assertEquals("off", source.value().get());
|
||||
}
|
||||
|
||||
/**
|
||||
* fleetd #426, the live-wiring half: {@code health.notifications} is a {@code
|
||||
* ConfigRef.SPLIT_KEYS} entry, and {@link Fleetd#healthCoverageSource} reads {@code
|
||||
* config.get().health()} live (not the frozen startup {@code cfg}) — exactly like {@link
|
||||
* Fleetd#capacitySource}'s {@code maxLoad} ({@code FleetdCapacitySourceWiringTest}'s {@code
|
||||
* reloadedMaxLoadStillChangesWhatFleetListReports}). A hot-reloaded notifications block must
|
||||
* change what {@code fleet_list} reports without a restart; a fix that froze the whole source
|
||||
* against the startup snapshot would silently break that and every other test above would stay
|
||||
* green, since none of them reload.
|
||||
*/
|
||||
@Test
|
||||
@DisplayName("a hot notifications reload still changes what fleet_list reports")
|
||||
void reloadedNotificationsStillChangeWhatFleetListReports(@TempDir Path dir) throws Exception {
|
||||
Path file = dir.resolve("fleetd.yaml");
|
||||
Files.writeString(file, HEALTH_DETECTION_ONLY);
|
||||
FleetConfig cfg = FleetConfig.load(file);
|
||||
ConfigRef config = new ConfigRef(file, cfg);
|
||||
|
||||
FleetMcp.HealthCoverageSource source = Fleetd.healthCoverageSource(config);
|
||||
assertEquals("detection-only", source.value().get(),
|
||||
"sanity: detection-only before any reload");
|
||||
|
||||
Files.writeString(file, HEALTH_FULL);
|
||||
assertTrue(config.reload().applied());
|
||||
// The live snapshot now carries the webhook — proves the reload really happened and this
|
||||
// test is not accidentally passing because nothing changed.
|
||||
assertTrue(config.get().health().notifications() != null
|
||||
&& config.get().health().notifications().configured(),
|
||||
"sanity: the reloaded config really carries a configured webhook");
|
||||
|
||||
assertEquals("full", source.value().get(),
|
||||
"the SAME HealthCoverageSource instance must reflect a reloaded notifications "
|
||||
+ "block without a restart — health.notifications is read live off "
|
||||
+ "config.get(), exactly like capacitySource's maxLoad");
|
||||
}
|
||||
|
||||
private static FleetMcp.HealthCoverageSource sourceFor(Path dir, String yaml) throws Exception {
|
||||
Path file = dir.resolve("fleetd.yaml");
|
||||
Files.writeString(file, yaml);
|
||||
FleetConfig cfg = FleetConfig.load(file);
|
||||
ConfigRef config = new ConfigRef(file, cfg);
|
||||
return Fleetd.healthCoverageSource(config);
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,40 @@
|
||||
package dev.ltms.fleet.health;
|
||||
|
||||
import org.junit.jupiter.api.Test;
|
||||
|
||||
import static org.junit.jupiter.api.Assertions.assertEquals;
|
||||
|
||||
/**
|
||||
* fleetd #426: {@link FleetHealthMonitor#coverage} had zero references anywhere in the test tree —
|
||||
* not the method, not either output string, not the field it populates. This pins the method's own
|
||||
* three branches directly.
|
||||
*
|
||||
* <p>This is the easy half. It proves the method words each combination correctly, but it proves
|
||||
* nothing about whether {@code Fleetd.java}'s two call sites pass the right argument in the right
|
||||
* position — see {@code FleetdHealthCoverageSourceWiringTest} (package {@code dev.ltms.fleet}) for
|
||||
* the half that actually guards the call site, following the measured fleetd #415 lesson that a
|
||||
* thoroughly-tested method and an untested argument pairing at its call site are different risks.
|
||||
*
|
||||
* <p><strong>The three output strings are load-bearing and must not change here.</strong> {@code
|
||||
* "detection-only"} is read live off a running daemon's {@code fleet_list} today (measured
|
||||
* 2026-09-12) — this test intentionally asserts the exact literal strings so a future edit to the
|
||||
* wording trips it here first.
|
||||
*/
|
||||
class FleetHealthMonitorCoverageTest {
|
||||
|
||||
@Test
|
||||
void disabledIsOffRegardlessOfNotificationConfig() {
|
||||
assertEquals("off", FleetHealthMonitor.coverage(false, false));
|
||||
assertEquals("off", FleetHealthMonitor.coverage(false, true));
|
||||
}
|
||||
|
||||
@Test
|
||||
void enabledWithoutNotificationsIsDetectionOnly() {
|
||||
assertEquals("detection-only", FleetHealthMonitor.coverage(true, false));
|
||||
}
|
||||
|
||||
@Test
|
||||
void enabledWithNotificationsIsFull() {
|
||||
assertEquals("full", FleetHealthMonitor.coverage(true, true));
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user