From ff21f21e42858596b848c165fed958a40536e1f6 Mon Sep 17 00:00:00 2001 From: shujaat hasan Date: Tue, 25 Aug 2026 15:43:58 +0200 Subject: [PATCH 1/2] test(resources): pin that the wizard never offers more than the machine can give (backend#2221) #2221's fourth scope item -- "never offer a ladder rung the VM cannot honour" -- was covered by two examples (a shrunk machine, a too-small machine). The property it actually asks for is universal, and a future rung ladder (the epic's XS 4 / S 16 / M 32 / L 64 / XL 128 GiB decision of record) is exactly the change that satisfies both examples and breaks the property: offer a fixed rung and it is unhonourable by construction on a small VM. Swept over six machine shapes including #2221's own measurement (a 6 CPU / 11.67 GiB Docker Desktop VM). A machine that cannot seat the floor must fail honestly rather than offer something unhonourable, which folds in the Bugbot #241 rule as part of the same invariant. My first version was VACUOUS: it answered the prompt with a prefix that matched no option, so the wizard left the budget alone and the assertion read the CURRENT value. It passed on every machine and survived a mutation making the wizard offer a fixed 8-core / 32-GiB rung -- the precise case it exists to catch. Fixed by answering the full option string and choosing a current budget a no-op cannot hide behind; three mutations now redden it. Co-Authored-By: Claude Opus 5 --- internal/cli/resources_set_test.go | 126 +++++++++++++++++++++++++++++ 1 file changed, 126 insertions(+) diff --git a/internal/cli/resources_set_test.go b/internal/cli/resources_set_test.go index c7f0191..294046c 100644 --- a/internal/cli/resources_set_test.go +++ b/internal/cli/resources_set_test.go @@ -5,13 +5,17 @@ import ( "context" "errors" "os" + "regexp" + "strconv" "strings" "testing" + "k8s.io/apimachinery/pkg/api/resource" "k8s.io/client-go/kubernetes/fake" "github.com/tracebloc/cli/internal/cluster" "github.com/tracebloc/cli/internal/helm" + "github.com/tracebloc/cli/internal/resources" "github.com/tracebloc/cli/internal/ui" ) @@ -1058,3 +1062,125 @@ func TestSet_MultiClientRedirectOpensWithSingleBlank(t *testing.T) { t.Errorf("multi-client set opens with a DOUBLE blank line: %q", head) } } + +// TestWizard_NeverOffersMoreThanTheMachineCanHonour is backend#2221's fourth +// scope item — "never offer a ladder rung the VM cannot honour" — asserted as an +// INVARIANT rather than as the two examples that covered it. +// +// Why a swept test and not a third example. The existing coverage +// (TestWizard_DefaultsClampedToShrunkMachine, TestWizard_ChooseAnAmountTooSmallMachine) +// pins two machine shapes. The property #2221 actually asks for is universal: on +// ANY machine, whatever the wizard applies must be something that machine can +// still give one run. A future rung ladder (the epic's XS 4 · S 16 · M 32 · L 64 · +// XL 128 GiB decision of record) is exactly the kind of change that satisfies both +// examples and breaks the property — offer a fixed rung, and on a small VM it is +// unhonourable by construction. +// +// The k3d row is #2221's own measurement: a Docker Desktop VM of 6 CPU / 11.67 GiB +// which two uncapped node containers each report in full. Bounding on the node is +// honest for ONE run precisely because a node's allocatable can never exceed the +// VM's — the double-count only misleads about CONCURRENCY, which is backend#2419's +// admission gate rather than this prompt's business. +func TestWizard_NeverOffersMoreThanTheMachineCanHonour(t *testing.T) { + for _, tc := range []struct { + name string + nodeCPU, nodeMem string + }{ + {"k3d on a Docker Desktop VM (#2221's measurement)", "6", "11934Mi"}, + {"a small laptop VM", "4", "8Gi"}, + {"an 8-core workstation", "8", "32Gi"}, + {"a large host", "64", "256Gi"}, + {"an awkward, non-power-of-two VM", "3", "6600Mi"}, + {"barely above the floor", "2", "6Gi"}, + } { + t.Run(tc.name, func(t *testing.T) { + // "Use as much as possible" — the pre-selected answer, so this is the + // path most runs take. + // THE FULL OPTION STRING, not a prefix. My first version answered + // "Use as much as possible", which matches NO option — the fake then + // returned the default, the wizard left the budget alone, and the + // assertion below matched the CURRENT value instead of an offered one. + // The test passed on every machine and survived a mutation that made + // the wizard offer a fixed 8-core / 32-GiB rung: exactly the case it + // exists to catch. A test that never reaches the code under test + // cannot fail for it. + pr := &fakePrompter{ + answers: map[string]string{ + "How much may one training run use?": "Use as much as possible (recommended if this machine is just for tracebloc)", + }, + confirm: boolPtr(true), + } + // A deliberately distinctive current budget: if the wizard were to + // leave it alone, the assertions below would read 1/3 and the floor + // check would fail — so a silent no-op cannot masquerade as a pass. + cs := csWith(tc.nodeCPU, tc.nodeMem, + map[string]string{"RESOURCE_LIMITS": "cpu=1,memory=3Gi"}) + var buf bytes.Buffer + err := applyResourcesSet(context.Background(), + ui.New(&buf, ui.WithColor(false)), pr, + setTarget(cs), cluster.KubeconfigOptions{}, setReq{dryRun: true}) + + node := resources.Machine{ + CPU: mustQty(t, tc.nodeCPU), + Mem: mustQty(t, tc.nodeMem), + } + maxCores := resources.MaxRunCores(node) + maxGiB := resources.MaxRunGiB(node) + + // A machine that cannot seat the floor must FAIL HONESTLY rather than + // offer something unhonourable — the Bugbot #241 rule, restated as + // part of the same invariant. + if maxCores < 1 || maxGiB < 2 { + if err == nil { + t.Fatalf("machine cannot seat the floor (maxCores=%d maxGiB=%d) "+ + "but the wizard applied something:\n%s", maxCores, maxGiB, buf.String()) + } + return + } + if err != nil { + t.Fatalf("wizard failed on a machine that can seat a run "+ + "(maxCores=%d maxGiB=%d): %v\n%s", maxCores, maxGiB, err, buf.String()) + } + + gotCores, gotGiB := appliedCPUAndGiB(t, buf.String()) + if gotCores > maxCores { + t.Errorf("offered %d cores, machine can give one run at most %d", + gotCores, maxCores) + } + if gotGiB > maxGiB { + t.Errorf("offered %d GiB, machine can give one run at most %d", + gotGiB, maxGiB) + } + // And it must not have collapsed below the per-run floor either: an + // offer of nothing is not "bounded", it is broken. + if gotCores < 1 || gotGiB < 2 { + t.Errorf("offered %d cores / %d GiB, below the 1-core / 2-GiB floor", + gotCores, gotGiB) + } + }) + } +} + +func mustQty(t *testing.T, s string) resource.Quantity { + t.Helper() + q, err := resource.ParseQuantity(s) + if err != nil { + t.Fatalf("ParseQuantity(%q): %v", s, err) + } + return q +} + +// appliedCPUAndGiB reads the cpu=N,memory=MGi the dry run says it would apply. +// Parsed from the output rather than returned by applyResourcesSet, because that +// output is what a user acts on — a bound the code respects internally and +// misreports is still a bound that misleads. +func appliedCPUAndGiB(t *testing.T, out string) (cores, gib int) { + t.Helper() + m := regexp.MustCompile(`cpu=(\d+),memory=(\d+)Gi`).FindStringSubmatch(out) + if m == nil { + t.Fatalf("no cpu=N,memory=MGi in the output:\n%s", out) + } + c, _ := strconv.Atoi(m[1]) + g, _ := strconv.Atoi(m[2]) + return c, g +} From cd7e643ccf0454b118952ae1d4da020f155fcfa0 Mon Sep 17 00:00:00 2001 From: shujaat hasan Date: Tue, 25 Aug 2026 15:58:40 +0200 Subject: [PATCH 2/2] test(resources): make the no-op backstop actually back something up (backend#2221) Cursor Bugbot Medium on #571. I claimed a current budget of cpu=1,memory=3Gi made a silent no-op fail the floor check. It does not -- 1 core / 3 GiB sits at and above the 1-core / 2-GiB floor, and every shape in the table can honour it, so a wizard that never offered anything passed every assertion. The real backstop is an equality check: "use as much as possible" has an exact contract, so a no-op (which applies the CURRENT budget), a fixed rung, and a prompt string that matches no option all land somewhere other than the machine maximum and fail. A bound alone is satisfied by offering nothing. Proof it now works: re-introducing the prefix-only prompt answer -- the vacuous version this test shipped with -- reddens 5 of 6 shapes, where before it passed. Co-Authored-By: Claude Opus 5 --- internal/cli/resources_set_test.go | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/internal/cli/resources_set_test.go b/internal/cli/resources_set_test.go index 294046c..dd2adbb 100644 --- a/internal/cli/resources_set_test.go +++ b/internal/cli/resources_set_test.go @@ -1110,9 +1110,17 @@ func TestWizard_NeverOffersMoreThanTheMachineCanHonour(t *testing.T) { }, confirm: boolPtr(true), } - // A deliberately distinctive current budget: if the wizard were to - // leave it alone, the assertions below would read 1/3 and the floor - // check would fail — so a silent no-op cannot masquerade as a pass. + // A current budget that every shape here could also honour — which is + // exactly why it is NOT the no-op backstop. My first version claimed it + // was: "if the wizard left it alone the floor check would fail". It + // would not. 1 core / 3 GiB sits at and above the 1-core / 2-GiB floor, + // so a silent no-op passed every assertion (Cursor Bugbot Medium). + // + // The real backstop is the equality check below: "use as much as + // possible" must apply EXACTLY the machine's maximum, so a no-op (which + // applies the current budget) and an over-offer both fail. That is the + // contract of the option being answered, and it cannot be satisfied by + // leaving things alone. cs := csWith(tc.nodeCPU, tc.nodeMem, map[string]string{"RESOURCE_LIMITS": "cpu=1,memory=3Gi"}) var buf bytes.Buffer @@ -1157,6 +1165,17 @@ func TestWizard_NeverOffersMoreThanTheMachineCanHonour(t *testing.T) { t.Errorf("offered %d cores / %d GiB, below the 1-core / 2-GiB floor", gotCores, gotGiB) } + // THE NO-OP BACKSTOP, and the reason the bound above is not enough on + // its own: "use as much as possible" has an exact contract, so anything + // that quietly declines to offer — a prompt string that matches no + // option, a future rung ladder, a silent early return — lands on the + // current budget instead of the maximum and fails here. A bound alone + // is satisfied by offering nothing. + if gotCores != maxCores || gotGiB != maxGiB { + t.Errorf("max-out applied %d cores / %d GiB, want exactly the "+ + "machine's %d / %d — a no-op or a fixed rung would look like this", + gotCores, gotGiB, maxCores, maxGiB) + } }) } }