Skip to content

perf(test): split the six build-running artifact tests into parallel files - #8950

Merged
JSONbored merged 1 commit into
mainfrom
perf/split-artifact-build-tests
Jul 31, 2026
Merged

perf(test): split the six build-running artifact tests into parallel files#8950
JSONbored merged 1 commit into
mainfrom
perf/split-artifact-build-tests

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Follow-up to #8937 / #8938.

Why

After #8938, tests/artifacts.test.ts is 118.8s of the 144.3s shared-registry pass — 82% of CI's test floor in one file. Six of its tests each run a full scripts/build-artifacts.ts (7.2–24.7s apiece, 99.4s combined), and vitest parallelizes across files, never within one. test.concurrent cannot help: the builds go through execFileSync, which blocks the whole worker. Separate files on separate workers is the only arrangement that overlaps them.

What

  • Four new sibling files (artifacts-build-{health,schema,determinism,schema-index}.test.ts), each with its own repo sandbox via the new tests/helpers/artifact-build-harness.ts factory (sandbox + path helpers + support-artifact snapshot/restore, extracted from artifacts.test.ts). The ~0.6s clone per file is noise against a ~20s build.
  • Moved tests' explicit timeouts: 30s → 120s. They measured 21–25s in CI against the old 30s cap — a margin that already blew past on a loaded machine.

A latent bug the split surfaced

The llms.txt test runs the real build, which rm's + repopulates the staging dir without an r2-manifest.json (that file is r2-manifest.ts's product, not the build's). The two R2 upload tests 2000 lines later only found a manifest because the departed build tests' cleanup happened to recreate one — a hidden in-file ordering dependency. They now regenerate it themselves (memoized ensureStagingManifest), so they pass in any order and under vitest -t isolation, which they never did before (verified: -t isolation fails on the pre-split file too).

Verification

check result
the 5 files on this base, --isolate=false --fileParallelism 23/23 green, 37.8s wall / 106.7s test time inside
vitest -t isolation on the two R2 tests passes (failed even pre-split)
typecheck / lint / format clean
run-ci-tests classification all four new files land in the shared pass (no module mocking)

@loopover-orb

loopover-orb Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Warning

⏸️ LoopOver review result - manual review recommended

Review updated: 2026-07-31 22:17:30 UTC

8 files · 1 AI reviewer · 1 blocker · CI green · clean

⏸️ Suggested Action - Manual Review

Review summary
Pure mechanical split of tests/artifacts.test.ts's six build-running tests into four sibling files, backed by a new tests/helpers/artifact-build-harness.ts factory that reproduces the original sandbox/snapshot/support-artifact logic almost verbatim (only call sites change from module-scope vars to harness.* properties). The PR also fixes a real ordering bug the split would otherwise introduce: with the six build tests removed from artifacts.test.ts, nothing in that file regenerates dist/metagraph-r2/metagraph/r2-manifest.json after the llms.txt test's real build wipes the staging tree, so the two remaining R2-upload tests would fail to find a manifest — the new memoized ensureStagingManifest() at tests/artifacts.test.ts:2415ish correctly guards both call sites and is only invoked by tests that need it, with nothing later in the file mutating the staging tree to invalidate the memoized flag. Workflow/script doc comments referencing the old file path were updated to point at the new artifacts-build-determinism.test.ts, keeping cross-references accurate.

Nits — 5 non-blocking
  • tests/artifacts.test.ts still hand-rolls its own sandbox creation, walkFilesRecursive, and public-tree snapshot/restore (tests/artifacts.test.ts top) that are now near-duplicated in tests/helpers/artifact-build-harness.ts — consider having artifacts.test.ts itself adopt the harness rather than keeping two copies of the same ~50 lines.
  • createArtifactBuildHarness's JSDoc block (tests/helpers/artifact-build-harness.ts) is a multi-paragraph docstring restating the file-header rationale already given above it; could be trimmed now that both the header comment and the function doc make the same 'one file per build' argument.
  • The 120s timeouts (up from 30s) are generous relative to the 7-25s measured build times — fine as a safety margin, but worth a one-line note on why 120s specifically was chosen over a tighter number like 60s.
  • Consider a follow-up to fold artifacts.test.ts's own sandbox bootstrap into createArtifactBuildHarness (or a shared lower-level sandbox+snapshot helper) so future changes to the snapshot/restore semantics don't need to be kept in sync across two files by hand.
  • The ensureStagingManifest() memoization in tests/artifacts.test.ts is currently correct only because no test between its definition and its two call sites touches r2StagingRoot — a code comment noting that invariant would help a future contributor who inserts a new R2-staging test in between.

Concerns raised — review before merging

  • No linked issue detected: No closing reference or linked issue number was found in the PR metadata/body. — If this PR is intended to solve an issue, link it explicitly in the PR body.
📋 Copy for AI agents — paste into your coding agent
Fix the following blocker(s) from this PR review:

1. No linked issue detected: No closing reference or linked issue number was found in the PR metadata/body. — If this PR is intended to solve an issue, link it explicitly in the PR body.

Decision drivers

  • ❌ Code review — 1 blocker (1 reviewer)
  • ❌ Gate result — Blocking (Repo-configured hard blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ No-issue rationale PR body explains why no issue is linked.
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (no linked issue context).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 9 registered-repo PR(s), 8 merged, 279 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 9 PR(s), 279 issue(s).
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository registration is not available in the local LoopOver cache.
  • Public profile languages: Python, TypeScript, Ruby, Go, MDX, Shell, Solidity, JavaScript
  • Official Gittensor activity: 9 PR(s), 279 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Start here: Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • Then work through the remaining 1 step in the Signals table above.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
🧪 Chat with LoopOver

Ask LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.

  • @loopover ask <question> answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat <question> answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @loopover mention with a real question is routed to the closest matching read-only command automatically — no exact syntax required.

Full command reference: https://loopover.ai/docs/loopover-commands

🧪 Experimental — new and may change.

Decision record
  • action: hold · clause: missing_linked_issue
  • config: d0c2d34aefe030920b47fe4ecf565b94f4452543781d9403d4e19d5794149309 · pack: oss-anti-slop · ci: passed
  • record: 86c8c66a3c8ff866bf4f831591eb1c46e23ea7764d32c6f9390c42db5dae3692 (schema v6, head 23708c1)

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.

  • Re-run LoopOver review

…files

tests/artifacts.test.ts was 118.8s of the 144.3s shared-registry pass -- 82% of
CI's test floor in one file. Six of its tests each ran a full
scripts/build-artifacts.ts (7.2s-24.7s apiece, 99.4s combined), and vitest
parallelizes across FILES, never within one; test.concurrent could not help
either, because execFileSync blocks the whole worker. The only arrangement
that overlaps the builds is separate files on separate workers.

Move the four heaviest into sibling files -- artifacts-build-health,
artifacts-build-schema, artifacts-build-determinism, artifacts-build-schema-
index -- each with its own repo sandbox via the new
tests/helpers/artifact-build-harness.ts factory (per-file sandbox + path
helpers + support-artifact snapshot/restore, extracted from artifacts.test.ts).
The extra ~0.6s clone per file is noise against a ~20s build. Locally the five
files now finish in 61.5s wall with 165.8s of test time inside -- the slowest
file is 38.6s, down from 118.8s.

The split surfaced a hidden in-file ordering dependency: the llms.txt test
runs the real build, which rm's + repopulates the staging dir WITHOUT an
r2-manifest.json (that file is r2-manifest.ts's product, not the build's), and
the two R2 upload tests 2000 lines later only found a manifest because the
departed build tests' cleanup happened to recreate one. They now regenerate it
themselves (memoized ensureStagingManifest), so they pass in any order and
under vitest -t isolation, which they never did before.

The moved tests' explicit timeouts go from 30s to 120s: they measured
21-25s in CI against the old 30s cap, a margin that already blew up on a
loaded machine.
@superagent-security

Copy link
Copy Markdown

Superagent didn't find any vulnerabilities or security issues in this PR.

@JSONbored
JSONbored force-pushed the perf/split-artifact-build-tests branch from 52eb90f to 23708c1 Compare July 31, 2026 22:06
@loopover-orb loopover-orb Bot added the manual-review Gittensor contributor context label Jul 31, 2026
@JSONbored
JSONbored merged commit 762d7f1 into main Jul 31, 2026
6 checks passed
@JSONbored
JSONbored deleted the perf/split-artifact-build-tests branch July 31, 2026 22:18
@JSONbored JSONbored mentioned this pull request Aug 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

manual-review Gittensor contributor context

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant