Skip to content

[2026-11-15t02-00-02-912z] slow-test-optimizer - #424

Merged
danii1 merged 1 commit into
mainfrom
feature/2026-11-15t02-00-02-912z
Nov 15, 2026
Merged

danii1 merged 1 commit into
mainfrom
feature/2026-11-15t02-00-02-912z

Conversation

@danii1

@danii1 danii1 commented Nov 15, 2026

Copy link
Copy Markdown
Contributor

Why

Task 2026-11-15t02-00-02-912z: slow-test-optimizer

The suite is mostly fast. The slowest tests are either deliberate real-time tests or sequential process spawns. should reject removed worker flags in packages/code/tests/cli.test.ts spawned six independent bun CLI processes one after another, so it paid process startup six times in sequence. That cost is fixable without changing what the test checks.

What changed

In packages/code/tests/cli.test.ts, the six runCLI(["worker", flag]) calls now run with Promise.all. The assertions are identical: stderr contains <flag> has been removed from devintern worker, and exit code is 1, checked per flag. No other file changed.

Slowest tests measured (JUnit, full-suite run):

Package Test Time Verdict
code runCLI timeout diagnostic – escalates to SIGKILL (tests/markdown-file.test.ts) 2.52s Left alone. It asserts the real timeout + grace interval.
code CLI Argument Handling – reject removed worker flags 2.38s Optimized.
code Default branch detection – terminates descendant processes before a timed-out git command resolves (tests/default-branch-detection.test.ts) 2.02s Left alone. It deliberately waits out a 2s git timeout.
code CLI Argument Handling – invalid --auto-review-iterations 1.70s Single CLI spawn.
code CLI Argument Handling – help for non-interactive worker setup commands 1.62s Single CLI spawn.
pm runInteractiveMode – waitForEdit 0.051s Noise floor.
agent-harness spawnAgent output sink – multi-byte character split across chunks 0.30s Not worth targeting.

Before and after for the target:

  • Before: 2.42s, 2.28s, 2.27s (3 runs of cli.test.ts).
  • After: 0.74–1.13s across the 10 verification runs, roughly 2.5× faster.

Scope

Included: the one test above. Excluded: all other slow tests. The SIGKILL and git-timeout tests are slow because they assert real timing, so they were left alone. The other CLI-spawn tests were not touched. packages/pm and packages/agent-harness have nothing near worth optimizing.

Tradeoffs

Six bun processes now start at once instead of one at a time. That means a brief CPU spike, and in principle more exposure to load-induced slowness. The file's default timeout is 30s, so there's a lot of margin. Each runCLI call uses its own temp directory, so there's no shared state between the parallel runs. I did not change any timeouts.

Blast Radius

Only this test body changed. No source code, helpers, or other tests were affected. The remaining risk is flakiness from parallel spawns under heavy load; this didn't show up in the runs below.

Verification

  • Ran bun run tests/run-tests.ts cli.test.ts 10 times after the change. All 10 exited 0 with 52 pass and 0 fail.
  • Ran bun run test in packages/code. 2931 pass, 0 fail, 186 files.
  • bun run format:check passes. bun run typecheck printed no errors.
  • oxlint was run on the file but it only printed "No config found, using defaults" and its result was not otherwise examined. I did not run the repo's configured lint (bun run lint).
  • Plain bun test in packages/code refuses to run, because tests/run-tests.ts must set GIT_CEILING_DIRECTORIES. I used bun run tests/run-tests.ts for all package-code measurements.
  • packages/pm and packages/agent-harness were measured once each and not re-run.

This PR was automatically created by @devintern/code

@danii1 danii1 added the devintern-authored Pull request written by DevIntern working its own backlog label Nov 15, 2026
@danii1

danii1 commented Nov 15, 2026

Copy link
Copy Markdown
Contributor Author

Artifacts & verification

Run: 370 · Commit: 3b39a2008f95baf5df0a5b95b86c5e995c6ad67d

Verification report

Produced by __verify-1, execution 1 at 3b39a2008f95baf5df0a5b95b86c5e995c6ad67d.

Show artifact
{
  "summary": "The change makes one bounded optimization. The `should reject removed worker flags` test in `packages/code/tests/cli.test.ts` spawned six CLI subprocesses one after another. It now spawns them with `Promise.all`. All six flags are still checked, with the same `stderr` and `exitCode` assertions, and nothing was skipped, deleted, or given a longer timeout. I measured the test at about 1116ms before and about 249ms after. It passed 10 of 10 consecutive runs, and the full `cli.test.ts` file passed (48 pass, 0 fail). I did not run the whole `packages/code` suite. The diff is test-only, so there is no product regression risk. The parallel runs are safe because `runCLI` gives each call its own temp directory (`cli-test-<Date.now()>-<random>`) and its own `Bun.spawn` (`cli.test.ts:46-91`), with no shared state. The task also asked for the top slow tests across packages and the cost of repeating the run at package level. The diff cannot show whether the per-package JUnit ranking was done. The commit is also missing a before/after write-up, though the task says the pipeline reports that.",
  "items": [
    {
      "priority": "low",
      "confidence": "medium",
      "category": "testing",
      "file": "packages/code/tests/cli.test.ts",
      "line": "206",
      "issue": "Six `bun` processes now start at once inside a `describe.concurrent` block. Under heavy CI load that adds some risk of a timeout, since each spawn is limited to 30s (`CLI_SPAWN_TIMEOUT_MS`) and the test to 30s (`setDefaultTimeout`).",
      "evidence": "Ran the test 10 times in a row, all passed. The whole 48-test file also passed in 2.22s. Not run under artificial CPU pressure, so the load risk is not proven.",
      "suggestion": "None required. If it turns out flaky on CI, run the six spawns in smaller batches."
    },
    {
      "priority": "info",
      "confidence": "medium",
      "category": "documentation",
      "file": "packages/code/tests/cli.test.ts",
      "line": "206",
      "issue": "The measurements the task asked to report (top slow tests per package, before/after timings, 10-run result) are not part of the diff.",
      "evidence": "Only `cli.test.ts` changed in the commit (`git log -1 --stat`: 1 file, 8 insertions, 5 deletions). I did not see the per-package JUnit ranking in the diff.",
      "suggestion": "Put the ranking, the 1116ms to 249ms timing, and the 10/10 result in the PR description."
    }
  ],
  "approved": true
}

Auto-review summary

Produced by auto-review, execution 1 at 3b39a2008f95baf5df0a5b95b86c5e995c6ad67d.

Show artifact
{
  "iterations": 1,
  "success": true,
  "finalFeedback": {
    "summary": "This PR parallelizes six independent `runCLI(['worker', flag])` subprocess spawns in one test with `Promise.all`. It is safe because `runCLI` (cli.test.ts:46-86) builds a unique temp cwd per call from a timestamp and a random suffix, and it shares no mutable state between calls. I ran the test with `GIT_CEILING_DIRECTORIES` set, and it passed with 12 expect() calls in about 275ms. The blast radius is limited to this one test, and I found nothing that blocks approval.",
    "items": [
      {
        "priority": "low",
        "confidence": "medium",
        "category": "testing",
        "file": "packages/code/tests/cli.test.ts",
        "line": "206-215",
        "issue": "The test sits inside `describe.concurrent`, so adding six more concurrent spawns raises peak process load. Each spawn has a timeout (`CLI_SPAWN_TIMEOUT_MS`, cli.test.ts:71). On a loaded CI runner a timeout would surface as an empty-stderr assertion failure, not as a clear timeout message. `runCLI` returns a `timedOut` flag, but the test does not check it.",
        "evidence": "cli.test.ts:71 and :85 (`timedOut: signal.aborted`). The run was fine, but I did not stress-test it under load, so the flakiness risk is unproven.",
        "suggestion": "Optionally assert `results[i].timedOut` is false before the stderr check, so a timeout fails with a clear cause. Alternatively, leave the test as it is, since the time saved is small."
      },
      {
        "priority": "info",
        "confidence": "medium",
        "category": "code-quality",
        "file": "packages/code/tests/cli.test.ts",
        "line": "211-214",
        "issue": "`flags.forEach` with `expect` inside loses the flag name when `results[i]` is undefined. The assertion is still correct because the `toContain` message embeds the flag.",
        "evidence": "cli.test.ts:211-214",
        "suggestion": "Use `for (const [i, flag] of flags.entries())` to follow the repo's loop style. This is optional."
      }
    ],
    "approved": true
  },
  "history": [
    {
      "iteration": 1,
      "feedback": {
        "summary": "This PR parallelizes six independent `runCLI(['worker', flag])` subprocess spawns in one test with `Promise.all`. It is safe because `runCLI` (cli.test.ts:46-86) builds a unique temp cwd per call from a timestamp and a random suffix, and it shares no mutable state between calls. I ran the test with `GIT_CEILING_DIRECTORIES` set, and it passed with 12 expect() calls in about 275ms. The blast radius is limited to this one test, and I found nothing that blocks approval.",
        "items": [
          {
            "priority": "low",
            "confidence": "medium",
            "category": "testing",
            "file": "packages/code/tests/cli.test.ts",
            "line": "206-215",
            "issue": "The test sits inside `describe.concurrent`, so adding six more concurrent spawns raises peak process load. Each spawn has a timeout (`CLI_SPAWN_TIMEOUT_MS`, cli.test.ts:71). On a loaded CI runner a timeout would surface as an empty-stderr assertion failure, not as a clear timeout message. `runCLI` returns a `timedOut` flag, but the test does not check it.",
            "evidence": "cli.test.ts:71 and :85 (`timedOut: signal.aborted`). The run was fine, but I did not stress-test it under load, so the flakiness risk is unproven.",
            "suggestion": "Optionally assert `results[i].timedOut` is false before the stderr check, so a timeout fails with a clear cause. Alternatively, leave the test as it is, since the time saved is small."
          },
          {
            "priority": "info",
            "confidence": "medium",
            "category": "code-quality",
            "file": "packages/code/tests/cli.test.ts",
            "line": "211-214",
            "issue": "`flags.forEach` with `expect` inside loses the flag name when `results[i]` is undefined. The assertion is still correct because the `toContain` message embeds the flag.",
            "evidence": "cli.test.ts:211-214",
            "suggestion": "Use `for (const [i, flag] of flags.entries())` to follow the repo's loop style. This is optional."
          }
        ],
        "approved": true
      },
      "addressed": []
    }
  ]
}

@danii1
danii1 merged commit e187586 into main Nov 15, 2026
2 checks passed
@danii1
danii1 deleted the feature/2026-11-15t02-00-02-912z branch November 15, 2026 03:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

devintern-authored Pull request written by DevIntern working its own backlog

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant