-
-
Notifications
You must be signed in to change notification settings - Fork 0
chore(bench): migrate to modestbench #169
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR migrates the benchmark infrastructure to modestbench and introduces new benchmark suites for performance validation. The migration removes the previous performance validation test suite and adds comprehensive test coverage for value-to-schema benchmarks and sync function assertion suites.
Key changes:
- Removes existing performance validation test infrastructure
- Adds new value-to-schema benchmark integration and testing
- Introduces sync function pure/schema assertion suite tests
- Updates pre-commit hooks to include knip linting
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 7 comments.
Show a summary per file
File | Description |
---|---|
test/property/performance-validation.test.ts | Removes entire performance validation test suite (186 lines) |
test/bench/value-to-schema-integration.test.ts | Adds integration tests for value-to-schema benchmark runner |
test/bench/value-to-schema-generators.test.ts | Adds comprehensive generator validation tests for value-to-schema benchmarks |
test/bench/value-to-schema-bench.test.ts | Adds API contract tests for value-to-schema benchmark execution |
test/bench/suite-overlap.test.ts | Adds tests for suite overlap resolution logic |
test/bench/cli-help.test.ts | Adds CLI help output validation tests |
test/bench/backward-compatibility.test.ts | Adds backward compatibility validation tests |
test/assertion/sync-function-schema-suite.test.ts | Adds contract tests for sync function schema assertion suite |
test/assertion/sync-function-pure-suite.test.ts | Adds contract tests for sync function pure assertion suite |
.husky/pre-commit | Updates pre-commit hook to include knip linting |
valueToSchemaGeneratorFactory, | ||
} from '../../test-data/value-to-schema-generators.js'; | ||
|
||
const NUM_RUNS = 100; |
Copilot
AI
Oct 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The hardcoded NUM_RUNS constant should be configurable or environment-dependent. In CI environments, 100 runs might be excessive and slow down the build process.
const NUM_RUNS = 100; | |
const NUM_RUNS = Number.parseInt(process.env.NUM_RUNS ?? '', 10) || 100; |
Copilot uses AI. Check for mistakes.
'Error for nested object:', | ||
error instanceof Error ? error.message : String(error), | ||
); | ||
return true; // Allow this to pass for now |
Copilot
AI
Oct 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fallback return statement masks potential errors in nested object generation. Consider using a more specific error handling strategy or documenting why errors are acceptable here.
Copilot uses AI. Check for mistakes.
// Log the error but continue | ||
console.error( | ||
'Error for nested array:', | ||
error instanceof Error ? error.message : String(error), | ||
); | ||
return true; // Allow this to pass for now |
Copilot
AI
Oct 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the nested object case, this fallback masks errors for nested arrays. The error handling strategy should be consistent and well-documented.
// Log the error but continue | |
console.error( | |
'Error for nested array:', | |
error instanceof Error ? error.message : String(error), | |
); | |
return true; // Allow this to pass for now | |
// Only allow known, documented errors (e.g., stack overflow due to deep recursion) | |
// If you want to allow specific errors, check for them here and document why. | |
// For all other errors, rethrow to fail the test. | |
console.error( | |
'Error for nested array:', | |
error instanceof Error ? error.message : String(error), | |
); | |
throw error; // Fail the test on unexpected errors |
Copilot uses AI. Check for mistakes.
iterations: 3, // Reduce iterations | ||
sampleSize: 5, // Much smaller sample size | ||
timeout: 2000, // Reduce timeout | ||
warmupIterations: 1, // Reduce warmup |
Copilot
AI
Oct 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The reduced benchmark configuration values with inline comments suggest these are temporary settings for testing. Consider extracting these to a test configuration constant or documenting why these specific values are appropriate for tests.
Copilot uses AI. Check for mistakes.
test/bench/suite-overlap.test.ts
Outdated
if (typeof runner.resolveSuiteOverlaps === 'function') { | ||
// eslint-disable-next-line @typescript-eslint/no-unsafe-call | ||
const resolved = runner.resolveSuiteOverlaps(testInput) as string[]; |
Copilot
AI
Oct 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of type assertion and eslint-disable indicates uncertainty about the function signature. Consider using proper TypeScript types or guards to ensure type safety.
if (typeof runner.resolveSuiteOverlaps === 'function') { | |
// eslint-disable-next-line @typescript-eslint/no-unsafe-call | |
const resolved = runner.resolveSuiteOverlaps(testInput) as string[]; | |
// Type guard for resolveSuiteOverlaps | |
function isResolveSuiteOverlapsFn( | |
fn: unknown | |
): fn is (input: string[]) => string[] { | |
return ( | |
typeof fn === 'function' && | |
// Try to check arity (not perfect, but helps) | |
fn.length === 1 | |
); | |
} | |
if (isResolveSuiteOverlapsFn(runner.resolveSuiteOverlaps)) { | |
const resolved = runner.resolveSuiteOverlaps(testInput); |
Copilot uses AI. Check for mistakes.
test/bench/cli-help.test.ts
Outdated
const { AVAILABLE_SUITES } = await import(runnerPath); | ||
// eslint-disable-next-line @typescript-eslint/no-unsafe-argument |
Copilot
AI
Oct 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The eslint-disable for unsafe-argument suggests type issues with AVAILABLE_SUITES. Consider adding proper type definitions for this exported constant.
const { AVAILABLE_SUITES } = await import(runnerPath); | |
// eslint-disable-next-line @typescript-eslint/no-unsafe-argument | |
type AvailableSuites = Record<string, string>; | |
const { AVAILABLE_SUITES } = await import(runnerPath) as { AVAILABLE_SUITES: AvailableSuites }; |
Copilot uses AI. Check for mistakes.
// eslint-disable-next-line @typescript-eslint/no-unsafe-call | ||
const bench = createSyncFunctionAssertionsBench(); |
Copilot
AI
Oct 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Repeated use of no-unsafe-call eslint disables throughout the test files suggests missing or incomplete type definitions for the benchmark functions. Consider adding proper TypeScript interfaces.
Copilot uses AI. Check for mistakes.
c4be698
to
2b12e42
Compare
chore(copilot): update instructions
chore(benchmark): add new benchmark suites
Conflicts:
cspell.json
test/property/performance-validation.test.ts
chore(bench): integrate value-to-schema benchmarks
chore: update cspell config
chore(husky): update pre-commit to run knip
chore: clanker review edits
chore(test): remove benchmark test suite