From cbc3692a17894e7455b4a0569d71a993abcf0300 Mon Sep 17 00:00:00 2001 From: Said Boudjelda Date: Sat, 17 Oct 2026 19:19:58 +0200 Subject: [PATCH] fix(cli): kates test apply refuses a scenario spec number key that holds something other than a number A scenario with spec records: "5000000" and durationSeconds: 10m, common when CI templates the file, decodes both as strings. scenarioToRequest read them as no number and left them out of the request, so the run used the type's default records and duration, and test apply --wait graded the validate gates against that much shorter run and exited 0. Only the validate block, typed float64, made such a value an error. scenarioSpecProblems now checks the 13 number keys scenarioToRequest reads, listed in scenarioIntKeys, as it already checked the three enable keys. A quoted number, a unit or a bare key is refused before any test starts, as spec.records is "5000000"; write a number, and scenario-diff refuses the file the same way instead of listing the key under Not compared. draft_scenario reports such a key as invalid, with the wording of its true/false check, where it used to warn that the value was left out of the request. A test holds scenarioIntKeys and scenarioBoolKeys to the key kinds draft_scenario describes, so a new key cannot skip the check. The scenario-validate-grading caveat's apply.go ref is re-pointed to the moved validateSLAs and unevaluableSLAs, and the book's spec reference says the number keys take a number. --- cli/cmd/apply.go | 13 +++++++++ cli/cmd/apply_test.go | 45 ++++++++++++++++++++++++++++++ cli/cmd/mcp_tools_scenario.go | 15 ++++------ cli/cmd/mcp_tools_scenario_test.go | 15 +++++++++- cli/cmd/scenario_diff_test.go | 12 ++++++-- docs/book/13-scenario-files.md | 2 +- 6 files changed, 88 insertions(+), 14 deletions(-) diff --git a/cli/cmd/apply.go b/cli/cmd/apply.go index 44c6f5a3..19f0b4a5 100644 --- a/cli/cmd/apply.go +++ b/cli/cmd/apply.go @@ -510,10 +510,23 @@ func toBoolPtr(v interface{}) *bool { // scenarioBoolKeys are the spec keys scenarioToRequest sends as true or false. var scenarioBoolKeys = []string{"enableIdempotence", "enableTransactions", "enableCrc"} +// scenarioIntKeys are the spec keys scenarioToRequest reads as numbers. A +// quoted number, a unit such as 10m, or a bare key was left out of the +// request, and the run used its type's default without a word. +var scenarioIntKeys = []string{ + "records", "parallelProducers", "recordSizeBytes", "durationSeconds", "batchSize", "lingerMs", "numConsumers", + "replicationFactor", "partitions", "minInsyncReplicas", "targetThroughput", "fetchMinBytes", "fetchMaxWaitMs", +} + // scenarioSpecProblems names the spec keys of a scenario that // scenarioToRequest cannot send as written. func scenarioSpecProblems(s TestScenario) []string { var problems []string + for _, key := range scenarioIntKeys { + if v, ok := s.Spec[key]; ok && toIntPtr(v) == nil { + problems = append(problems, fmt.Sprintf("spec.%s is %s; write a number", key, mcpScnShow(v))) + } + } for _, key := range scenarioBoolKeys { if v, ok := s.Spec[key]; ok && toBoolPtr(v) == nil { problems = append(problems, fmt.Sprintf("spec.%s is %v; write true or false", key, v)) diff --git a/cli/cmd/apply_test.go b/cli/cmd/apply_test.go index 1ffd1f32..9c85ddef 100644 --- a/cli/cmd/apply_test.go +++ b/cli/cmd/apply_test.go @@ -455,3 +455,48 @@ func TestApply_RefusesAFlagThatIsNotABoolean(t *testing.T) { t.Errorf("%d requests reached the backend", requests) } } + +// A number key that holds no number is refused too. A quoted number, a unit +// or a bare key was left out of the request, so the run used its type's +// default records and duration, and --wait graded that shorter run. +func TestScenarioSpecProblems_ANumberKeyThatIsNotANumber(t *testing.T) { + var sf ScenarioFile + if err := yaml.Unmarshal([]byte("scenarios:\n - name: x\n type: LOAD\n spec:\n records: \"5000000\"\n durationSeconds: 10m\n lingerMs:\n"), &sf); err != nil { + t.Fatal(err) + } + problems := scenarioSpecProblems(sf.Scenarios[0]) + if len(problems) != 3 || !slices.Contains(problems, `spec.records is "5000000"; write a number`) || + !slices.Contains(problems, "spec.lingerMs is empty; write a number") { + t.Errorf("problems = %q", problems) + } + sf.Scenarios[0].Spec = map[string]any{"records": 5000000, "lingerMs": 0, "durationSeconds": 1.5} + if problems := scenarioSpecProblems(sf.Scenarios[0]); len(problems) != 0 { + t.Errorf("numbers, 0 among them, are fine: %q", problems) + } +} + +// kates test apply refuses a file with such a number before it starts any of +// its tests. +func TestApply_RefusesANumberThatIsNotANumber(t *testing.T) { + path := filepath.Join(t.TempDir(), "s.yaml") + data := "scenarios:\n - name: first\n type: LOAD\n - name: second\n type: LOAD\n spec:\n records: \"5000000\"\n" + if err := os.WriteFile(path, []byte(data), 0o600); err != nil { + t.Fatal(err) + } + var requests int + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { requests++ })) + defer ts.Close() + apiClient = client.New(ts.URL) + output.ResetForTesting() + applyFile, applyWait = path, false + defer func() { applyFile = "" }() + + err := testApplyCmd.RunE(testApplyCmd, nil) + + if err == nil || !strings.Contains(err.Error(), `scenario 2 (second): spec.records is "5000000"; write a number`) { + t.Errorf("err = %v", err) + } + if requests != 0 { + t.Errorf("%d requests reached the backend", requests) + } +} diff --git a/cli/cmd/mcp_tools_scenario.go b/cli/cmd/mcp_tools_scenario.go index 1376612d..aa51b280 100644 --- a/cli/cmd/mcp_tools_scenario.go +++ b/cli/cmd/mcp_tools_scenario.go @@ -898,9 +898,9 @@ func mcpScnCheckSpecKey(i int, key string, v any, testType string, fs *mcpScnFin } } -// mcpScnCheckSpecNumber flags a value scenarioToRequest leaves out (one that -// is not a number, or a 0 where the field leaves one out), a number it cuts, -// and one it sends outside the range the backend accepts. +// mcpScnCheckSpecNumber flags a value that is not a number, which kates test +// apply refuses, a 0 where the field leaves one out, a number scenarioToRequest +// cuts, and one it sends outside the range the backend accepts. func mcpScnCheckSpecNumber(i int, field string, k mcpScnSpecKey, v any, fs *mcpScnFindings) { var n int64 switch x := v.(type) { @@ -916,12 +916,7 @@ func mcpScnCheckSpecNumber(i int, field string, k mcpScnSpecKey, v any, fs *mcpS fs.add(i, mcpScnWarning, field, fmt.Sprintf("it is %v, which scenarioToRequest cuts to %d", x, n)) } default: - read := "which scenarioToRequest reads as 0" - if k.sendsZero { - read = "which is not a number" - } - fs.add(i, mcpScnWarning, field, "it is "+mcpScnShow(v)+", "+read+", so it is left out "+ - "of the request and the type default applies") + fs.add(i, mcpScnInvalid, field, "it is "+mcpScnShow(v)+"; kates test apply refuses it: write a number") return } scale := max(k.scale, 1) @@ -1276,7 +1271,7 @@ var mcpCaveatsScenario = []mcpCaveat{ "produce, and RPO is measured only when a resilience run marks a fault, so a scenario run never has one. " + "Keys ValidationSpec does not name, such as maxDuplicatePercent, are dropped when the file is read.", Refs: []string{ - mcpAnchoredRef("cli/cmd/apply.go:29-39,217-250,339-351,525-580,597-622", + mcpAnchoredRef("cli/cmd/apply.go:29-39,217-250,339-351,538-593,610-635", "type ValidationSpec struct {", `yaml:"maxCrcFailures,omitempty"`, "if !applyWait {", "nonNil(validateSLAs(finalResult, scenario.Validate))", "} else if r.SLA != nil {", `"✓ SLA Pass"`, diff --git a/cli/cmd/mcp_tools_scenario_test.go b/cli/cmd/mcp_tools_scenario_test.go index aa6f9e67..3d095908 100644 --- a/cli/cmd/mcp_tools_scenario_test.go +++ b/cli/cmd/mcp_tools_scenario_test.go @@ -455,7 +455,7 @@ validate: if s := sc.Request.Spec; sc.Request.TestType != "STRESS" || s.Records != 0 || s.LingerMs == nil || *s.LingerMs != 0 { t.Errorf("request = %+v", sc.Request.Spec) } - mcpScnHasFinding(t, out, 0, "spec.records", mcpScnWarning, "reads as 0") + mcpScnHasFinding(t, out, 0, "spec.records", mcpScnInvalid, "kates test apply refuses it") mcpScnHasFinding(t, out, 0, "spec.throughput", mcpScnWarning, "the scenario key for the producer rate is targetThroughput") mcpScnHasFinding(t, out, 0, "spec.numRecords", mcpScnWarning, "the scenario key is records") if f := mcpScnFindingsOn(out, 0, "spec.lingerMs"); len(f) != 0 { @@ -768,6 +768,19 @@ func TestMCPDraftScenarioSpecKeysMatchScenarioToRequest(t *testing.T) { if !slices.Equal(read, ours) { t.Errorf("scenarioToRequest reads %v; draft_scenario describes %v", read, ours) } + // scenarioSpecProblems checks every number and true/false key, so a key + // added to scenarioToRequest cannot skip apply's refusal. + kinds := map[byte][]string{} + for key, k := range mcpScnSpecKeys { + kinds[k.kind] = append(kinds[k.kind], key) + } + for kind, checked := range map[byte][]string{'i': scenarioIntKeys, 'b': scenarioBoolKeys} { + checked = slices.Sorted(slices.Values(checked)) + slices.Sort(kinds[kind]) + if !slices.Equal(checked, kinds[kind]) { + t.Errorf("scenarioSpecProblems checks %v; the %c keys are %v", checked, kind, kinds[kind]) + } + } // Each key's wire name is the JSON name of the field it sets. for key, k := range mcpScnSpecKeys { sc := TestScenario{Type: "LOAD", Spec: map[string]any{key: 7}} diff --git a/cli/cmd/scenario_diff_test.go b/cli/cmd/scenario_diff_test.go index d1431909..bd802ed7 100644 --- a/cli/cmd/scenario_diff_test.go +++ b/cli/cmd/scenario_diff_test.go @@ -273,14 +273,12 @@ func TestScenarioDiff_KeysApplyDoesNotSendAreNotCompared(t *testing.T) { spec: numRecords: 500000 throughput: 5000 - records: "50000" topic: "" partitions: 0 ` d := diffScenarioJSON(t, "s.yaml", file, diffRun("STRESS", `{"numRecords": 1000000, "throughput": -1, "partitions": 3, "topic": "stress-test"}`), "") want := []scenarioDiffSkip{ - {"spec.records", "it is not a number, so kates test apply leaves it out"}, {"spec.topic", "it is empty, so kates test apply leaves it out"}, {"spec.partitions", "kates test apply reads it as 0 and leaves it out"}, {"spec.numRecords", "kates test apply does not read it; the scenario key is records"}, @@ -384,6 +382,16 @@ func TestScenarioDiff_RefusesAFlagApplyRefuses(t *testing.T) { } } +// kates test apply refuses a number key that holds no number, so no run came +// from such a scenario either. +func TestScenarioDiff_RefusesANumberApplyRefuses(t *testing.T) { + const file = "scenarios:\n - name: quoted\n type: LOAD\n spec:\n records: \"50000\"\n" + _, err := runScenarioDiff(t, "s.yaml", file, diffRun("LOAD", `{"numRecords": 50000}`), "", "table") + if err == nil || !strings.Contains(err.Error(), `scenario 1 (quoted): spec.records is "50000"; write a number`) { + t.Errorf("err = %v", err) + } +} + // scenarioDiffFields compares every spec key scenarioToRequest reads (the keys // of mcpScnSpecKeys, which TestMCPDraftScenarioSpecKeysMatchScenarioToRequest // holds to apply.go), and reads each from the field its key sets in the diff --git a/docs/book/13-scenario-files.md b/docs/book/13-scenario-files.md index b21337e6..2c85599e 100644 --- a/docs/book/13-scenario-files.md +++ b/docs/book/13-scenario-files.md @@ -91,7 +91,7 @@ The `spec` object controls all test parameters. Every field is optional. The [Ka ::: {.callout-important} The Kates API merges `spec` with the test type's defaults, and every key below reaches the run. `targetThroughput` sets the producer rate in place of the type's default. `parallelProducers` counts only for STRESS and CAPACITY, and no test type reads `numConsumers`, so a LOAD scenario runs one producer and one consumer whatever they say. -The three `enable` keys take `true` or `false`. `kates test apply` refuses a file where one holds anything else, such as `yes`, `on` or nothing, naming the scenario and the key, before it starts any of the file's tests. +The three `enable` keys take `true` or `false`. `kates test apply` refuses a file where one holds anything else, such as `yes`, `on` or nothing, naming the scenario and the key, before it starts any of the file's tests. The number keys take a number: `kates test apply` and `kates scenario-diff` refuse a file where one holds a quoted number (`"5000000"`), a unit (`10m`, `5M`) or nothing, naming the scenario and the key, before any test starts. A key the scenario's type or benchmark backend cannot apply is refused: `kates test apply` gets a `400` naming it, and the scenario does not start. That is a rate other than -1 for SPIKE or CAPACITY, which run unthrottled; `consumerGroup` or a fetch setting for a type that takes no consumer settings (every type but LOAD, ENDURANCE and INTEGRITY); `enableCrc: true` for any type but INTEGRITY; [`enableIdempotence: true`](appendix-a-glossary.md#gl-idempotent-producer) or `enableTransactions: true` when [`acks`](appendix-a-glossary.md#gl-acks) is not `all` (SPIKE's default is `1`); `enableTransactions: true` with `enableIdempotence: false`, or on the `trogdor` benchmark backend. The [API Reference](11-api-reference.md#post-apitests) lists the rules; [Test Types Deep Dive](05-test-types.md) and [Data Integrity Verification](08-data-integrity.md) cover what the options do. :::