Add fixture profiles for runtime scenario comparisons#46
Conversation
Pan14ek
left a comment
Pan14ek
left a comment
There was a problem hiding this comment.
Overview
This PR introduces versioned, named fixture profiles for runtime scenario comparisons — a solid improvement over ad-hoc k6 env overrides. The design is well-structured: profiles are versioned, validated against an allowlist, precedence rules are explicit and tested, and the manifest records provenance cleanly.
The adapter split (command / k6 / kubernetes) makes the runner properly unit-testable, and the test coverage is broad. Overall this is well-engineered tooling.
Below are findings ranging from a potential data-quality bug to minor observations.
| } | ||
|
|
||
| return result; | ||
| }; |
There was a problem hiding this comment.
No timeout on spawnSync.
kubectl rollout status can block for minutes (or indefinitely if a pod never becomes Ready), and a stuck k6 run will also block forever. Consider threading a timeout option through the adapters:
export const runCommand = (
command, args,
{ allowFailure = false, capture = false, cwd, timeoutMs } = {}
) => {
const result = spawnSync(command, args, {
cwd,
encoding: capture ? 'utf-8' : undefined,
stdio: capture ? ['ignore', 'pipe', 'pipe'] : 'inherit',
timeout: timeoutMs, // undefined = no limit
});
// spawnSync sets signal='SIGTERM', status=null on timeout
if (result.signal === 'SIGTERM' && !allowFailure) {
throw new Error(`${command} timed out after ${timeoutMs}ms`);
}
...Even a generous ceiling (e.g. 10 min for rollout, 5 min for k6 smoke) prevents a CI job from hanging until its runner wall-clock limit kills it with no useful error message.
| .map((line) => line.trim()) | ||
| .filter(Boolean) | ||
| .map((line) => { | ||
| const [name, cpu, memory] = line.split(/\s+/); |
There was a problem hiding this comment.
Malformed kubectl top pods output can silently produce NaN utilization.
If a line has fewer than three tokens (e.g. a trailing empty line that slipped past .filter(Boolean), or a metrics-server line with an unexpected format), cpu or memory is undefined. parseCpuQuantity(undefined) returns undefined, and then in sumPodUsage:
usage.reduce((total, pod) => total + pod[field], 0)
// 0 + undefined = NaNNaN propagates through ratioOrUndefined (the denominator <= 0 guard doesn't catch it) and lands in the collector summary as NaN utilization.
Consider either:
- Adding a length guard in the map:
if (parts.length < 3) return null;then.filter(Boolean) - Or throwing on an unexpected format so the anomaly is visible immediately rather than silently corrupting the metric.
| } | ||
| }; | ||
|
|
||
| const requireWorkload = (name, workload) => { |
There was a problem hiding this comment.
requireWorkload only checks non-empty string — it doesn't validate against the WORKLOADS allowlist.
A typo in fixture-profiles-v1.json (e.g. "workload": "basline") passes validation here, is silently loaded as the resolved workload, and only fails later in validateOptions after all the fixture-merge logic has already run. The error message at that point (--workload must be one of) will confuse users who didn't pass --workload at all.
Since WORKLOADS lives in scenario-runner.mjs, you'd need to either pass it in or duplicate the allowlist. The simplest fix without a circular import is to pass an allowedWorkloads set into the loader:
export const loadFixtureProfile = (name, { cwd, file, allowedWorkloads } = {}) => {
...
validateProfile(name, profile, allowedWorkloads);
...
};Alternatively, document that workload validation is intentionally deferred to validateOptions — but then the error message should mention the fixture profile file as the probable source.
| }, | ||
| { | ||
| explicitK6Env: options.k6Env ?? {}, | ||
| fixtureFile: options.fixtureProfileFile ?? FIXTURE_PROFILES_PATH, |
There was a problem hiding this comment.
Fragile "explicit workload" heuristic when runScenario is called directly.
workloadExplicit: options.workload !== undefined && options.workload !== DEFAULT_WORKLOAD,When runScenario is called programmatically (not via parseArgs), there is no explicit/implicit distinction — the caller just passes an options object. If the caller sets workload: 'baseline' (which equals DEFAULT_WORKLOAD) intending it as an explicit override, the fixture profile will silently overwrite it.
The test 'lets direct runScenario workload overrides win over fixture defaults' passes workload: 'read-heavy', which is non-default, so the heuristic works there. But workload: 'baseline' would be overridden even if it was intentional.
Consider distinguishing using a sentinel (workloadExplicit: true) passed alongside the options, or documenting that programmatic callers should not set workload alongside fixtureProfile if they want the fixture to control the workload.
|
|
||
| try { | ||
| return { | ||
| args, |
There was a problem hiding this comment.
result.status is null when the process is killed by a signal.
spawnSync sets status: null and signal: 'SIGTERM' (or another signal) when the process is terminated by a signal rather than exiting normally. Here exitCode: result.status propagates null, and the downstream check k6Result.exitCode !== 0 is true (null ≠ 0), so a ScenarioRunError is thrown with the message "k6 failed with exit code null", which is confusing.
A small guard makes this clear:
return {
args,
exitCode: result.signal ? null : result.status,
signal: result.signal ?? undefined,
summary: ...,
};And in scenario-runner.mjs, the error message can then say "k6 was killed by signal ${k6Result.signal}" instead of "exit code null".
| {"_type":"issue","id":"lined-15y","title":"Remove redundant calendar requesterId query parameter","description":"PR #43 review follow-up: conflict endpoints now bind requester identity to X-User-Id, making the requesterId query parameter redundant. Deprecate and remove requesterId from /api/calendar/conflicts and /api/calendar/user-conflict in a compatibility-focused cycle.","status":"open","priority":3,"issue_type":"task","owner":"alexmakeev2703@gmail.com","created_at":"2026-06-01T14:40:15Z","created_by":"Oleksii Makieiev","updated_at":"2026-06-01T14:40:15Z","dependency_count":0,"dependent_count":0,"comment_count":0} | ||
| {"_type":"issue","id":"lined-oss","title":"Document Notion knowledge-base workflow","description":"Add backend documentation for using Notion as the durable knowledge base, including write-back checklist, verification after write, fallback policy, and entry template.","status":"closed","priority":3,"issue_type":"task","owner":"alexmakeev2703@gmail.com","created_at":"2026-05-26T20:53:14Z","created_by":"Oleksii Makieiev","updated_at":"2026-05-26T20:53:24Z","closed_at":"2026-05-26T20:53:24Z","close_reason":"Completed: added Notion knowledge-base workflow doc, linked it from backend docs index and AGENTS routing, and verified the documentation diff.","dependency_count":0,"dependent_count":0,"comment_count":0} | ||
| {"_type":"issue","id":"lined-azh","title":"Verify backend Docker image with running daemon","description":"Docker build and container smoke checks for experiment/backend-containerization could not run in this session because Docker daemon socket /Users/oleksii_makieiev/.docker/run/docker.sock was missing. Start Docker Desktop or another daemon, run docker build -t lined-backend:local ., then run the image against PostgreSQL and verify /actuator/health and /swagger-ui.html.","acceptance_criteria":"docker build succeeds; container starts against PostgreSQL; actuator health and Swagger UI are reachable from localhost.","status":"open","priority":3,"issue_type":"task","owner":"alexmakeev2703@gmail.com","created_at":"2026-05-23T17:39:18Z","created_by":"Oleksii Makieiev","updated_at":"2026-05-23T17:39:18Z","dependency_count":0,"dependent_count":0,"comment_count":0} | ||
| {"_type":"issue","id":"lined-62z","title":"Add backend Docker image","description":"Add reproducible Docker image support and documented build/run flow for the Spring Boot backend as the experiment/backend-containerization task.","acceptance_criteria":"Dockerfile builds the backend image reproducibly; container run flow is documented; docs index routes to the containerization guide; backend behavior is unchanged.","status":"closed","priority":2,"issue_type":"task","assignee":"Oleksii Makieiev","owner":"alexmakeev2703@gmail.com","created_at":"2026-05-23T17:35:06Z","created_by":"Oleksii Makieiev","updated_at":"2026-05-23T17:41:10Z","started_at":"2026-05-23T17:35:17Z","closed_at":"2026-05-23T17:41:10Z","close_reason":"Implemented Dockerfile, .dockerignore, and containerization docs; Gradle check and JaCoCo passed. Docker daemon-backed build/run verification is tracked separately in lined-azh because the local Docker socket was unavailable.","dependency_count":0,"dependent_count":0,"comment_count":0} |
There was a problem hiding this comment.
14 issue records wiped in this diff — is this intentional?
The diff removes 14 existing issue entries (including open issues lined-51s, lined-15y, lined-azh) and retains only 1 (the now-closed lined-62z). If the Beads store is designed to compact and this is intentional (e.g. closed issues are archived elsewhere), that's fine — but it's worth confirming:
lined-51s("Add scenario runner seam",in_progress) andlined-15y("Remove redundant calendar requesterId",open) disappear from the file. If these are tracked nowhere else, the work is lost.lined-azh("Verify Docker image",open) similarly.
If this is a deliberate compaction (open issues are tracked in Notion/elsewhere and the file is ephemeral), a note in the commit message or PR description would clarify intent for reviewers.
Pan14ek
left a comment
Pan14ek
left a comment
There was a problem hiding this comment.
Summary
Overall: well-engineered. The adapter split makes the runner properly testable, the allowlist-based security model (k6 env keys, profile keys, base URL) is the right approach, and the test coverage is broad with good edge-case discipline (restart delta on counter reset, k6 summary-missing path, etc.).
Findings (by severity)
Data quality bug (should fix)
- kubernetes-adapter.mjs:189 —
parseTopPodswith fewer than 3 tokens producesundefinedCPU/memory values;NaNpropagates silently into collector utilization metrics.
Operational risk (recommended fix)
- command-runner.mjs:22 — No
spawnSynctimeout. A stuckkubectl rollout statusor a hanging k6 will block the runner forever with no useful message. - k6-adapter.mjs:68 — Signal-killed processes yield
status: null, producing a confusing"exit code null"error. A signal guard makes this diagnosable.
Correctness / UX (worth addressing)
- fixture-profiles.mjs:100 —
requireWorkloaddoesn't cross-check against theWORKLOADSallowlist. A typo in the JSON file defers to a confusing error message at validation time. - scenario-runner.mjs:250 — The
workload !== DEFAULT_WORKLOADheuristic inresolveFixtureOptionssilently overridesworkload: 'baseline'when passed directly torunScenario, even if it was intentional.
Process question
- .beads/issues.jsonl:1 — 14 issue records removed (including 3 open ones). Confirm this is an intentional compaction.
What's missing from tests
parseTopPodswith malformed / short-token input (the NaN case above is untested)command-runner.mjshas no direct unit tests; it's exercised indirectly via adapter stubs, but signal/timeout behavior is not covered
|
1 participant
Purpose
Define explicit, versioned fixture profiles for Lined runtime scenario runs so deployment comparisons use stable setup inputs instead of implicit k6 defaults or manual setup.
Type
Changes
--fixture-profile, with explicit precedence: fixture defaults first, then--workloadand repeated--k6-env KEY=valueoverrides.runtime-summary-manifest.jsonwhile keepingruntime-summary.jsonunchanged for collector input.experiment/scenario-fixture-disciplineimplemented.Files changed
load-tests/runtime-scenarios/fixture-profiles-v1.jsonload-tests/runtime-scenarios/fixture-profiles.mjsload-tests/runtime-scenarios/scenario-runner.mjs--fixture-profileparsing, override precedence, and manifest metadata wiring.load-tests/runtime-scenarios/scenario-runner-cli.mjsload-tests/runtime-scenarios/runtime-summary.mjsload-tests/runtime-scenarios/*-adapter.mjsload-tests/runtime-scenarios/scenario-runner.test.mjsdocs/runtime-scenario-summaries.mddocs/load-test-baseline.mddocs/experiment-tasks.mdexperiment/scenario-fixture-disciplineas implemented..beads/issues.jsonl,.beads/interactions.jsonlExpected result
Runtime scenario comparisons can be run with named, reproducible fixture profiles. The backend HTTP API, database schema, Kubernetes scenario semantics, and collector-facing
runtime-summary.jsonshape remain unchanged.Verification
node --test load-tests/runtime-scenarios/*.test.mjsnode --check load-tests/runtime-scenarios/scenario-runner.mjsnode --check load-tests/runtime-scenarios/fixture-profiles.mjsnode --check load-tests/runtime-scenarios/scenario-runner.test.mjsgit diff --checkkubectl kustomize k8s/kind/scenarios/fixed-smallkubectl kustomize k8s/kind/scenarios/fixed-mediumkubectl kustomize k8s/kind/scenarios/replicas-2kubectl kustomize k8s/kind/scenarios/hpa-cpuChecklist
./gradlew checkpasses locally./gradlew jacocoTestReportpasses locally