Stop including API benchmarks in validate - #64337
Wesley Wigham (weswigham) wants to merge 1 commit into
Conversation
typescript-automation
Bot
added
Author: Team
For Uncommitted Bug
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The broader Go benchmark exclusion and conflicting contributor guidance need clarification or correction.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (2)
What changed in this PR
Separates API benchmark smoke runs from validation and executes them through dedicated tasks.
Changes:
- Removes benchmarks from API tests.
- Adds a standalone API benchmark task and CI step.
- Excludes benchmark tasks from
validate.
| File | Description |
|---|---|
| packages/typescript/test/sync/api.test.ts | Removes sync benchmark execution. |
| packages/typescript/test/sync/api-generators.test.ts | Removes generator benchmark execution. |
| packages/typescript/test/async/api.test.ts | Removes async benchmark execution. |
| Herebyfile.mjs | Adds API benchmark orchestration and changes validation behavior. |
| .github/workflows/ci.yml | Runs API benchmarks as a dedicated CI step. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| export const validate = task({ | ||
| name: "validate", | ||
| description: "Builds, tests, lints, and formats the repo. Pass --api to include API tests, or --all to include all ancilliary repository tests.", | ||
| description: "Builds, tests, lints, and formats the repo. Pass --api to include API tests, or --all to include ancillary repository tests. Benchmarks are separate: test:benchmarks and test:benchmarks:api.", |
There was a problem hiding this comment.
Why exclude it? Don't we want to test that in --all?
I still think this is an instruction problem. One can totally break the compiler but it only affect a benchmark, no?
There was a problem hiding this comment.
The benchmarks don't really test anything unique - they just collect perf metrics. Maybe you could break them, but it's very unlikely you do so without also breaking other tests. And, moreover, the timing output is useless intermixed within the parallel test output.
They get their own test subtask executed in CI instead of being run directly in the main API tests, similar to how the
gobenchmarks already work.