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. :::