Conversation
4f8226a to
10d79f9
Compare
|
@cursor please /review - and point out any lost functionality or regressions this may cause, if any. Don't hallucinate. |
Code Review: Bun Test Runner SupportI've completed a thorough review of the PR. Here are my findings: ✅ Implementation QualityThe Bun adapter follows the correct pattern established by the Vitest adapter:
📋 Observed Differences (Not Regressions)These are intentional adapter design patterns:
import { Try, countKeys, createStream } from './riteway';Users can do the same with Bun - import utilities from
🔍 Missing Test CoverageThe Bun test file (
📝 Documentation QualityThe README updates are comprehensive and follow the same pattern as the Vitest section. Good examples and clear setup instructions.
|
|
24ee960 to
b239090
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds support for Bun's built-in test runner to the riteway testing library. The implementation provides a familiar assert API that works with Bun's fast, built-in test runner through a custom matcher extension.
Changes:
- Added
source/bun.jsadapter withassertfunction andsetupRitewayBun()for registering custom matchers - Added TypeScript definitions in
bun.d.ts - Created test suite in
source/bun.test.jsvalidating the adapter functionality - Updated README with comprehensive Bun usage documentation including setup, examples, and failure output
- Added package.json export configuration for
riteway/bun
Reviewed changes
Copilot reviewed 6 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| source/bun.js | Core Bun adapter implementation with assert function and custom matcher setup |
| source/bun.test.js | Test suite for the Bun adapter covering basic assertion scenarios |
| bun.d.ts | TypeScript type definitions for the Bun adapter exports |
| package.json | Added export path for riteway/bun module |
| README.md | Added comprehensive documentation section for Bun integration with setup instructions and examples |
| .claude/settings.local.json | Claude Code tool configuration (development artifact) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import { test, describe, assert, setupRitewayBun } from './bun.js'; | ||
|
|
||
| setupRitewayBun(); | ||
|
|
||
| describe('riteway/bun', () => { | ||
| test('given: matching primitives, should: pass', () => { | ||
| assert({ | ||
| given: 'two identical numbers', | ||
| should: 'be equal', | ||
| actual: 42, | ||
| expected: 42, | ||
| }); | ||
| }); | ||
|
|
||
| test('given: matching strings, should: pass', () => { | ||
| assert({ | ||
| given: 'two identical strings', | ||
| should: 'be equal', | ||
| actual: 'hello', | ||
| expected: 'hello', | ||
| }); | ||
| }); | ||
|
|
||
| test('given: matching objects, should: pass', () => { | ||
| assert({ | ||
| given: 'two identical objects', | ||
| should: 'be deeply equal', | ||
| actual: { name: 'Bun', version: 1.1 }, | ||
| expected: { name: 'Bun', version: 1.1 }, | ||
| }); | ||
| }); | ||
|
|
||
| test('given: matching arrays, should: pass', () => { | ||
| assert({ | ||
| given: 'two identical arrays', | ||
| should: 'be deeply equal', | ||
| actual: [1, 2, 3], | ||
| expected: [1, 2, 3], | ||
| }); | ||
| }); | ||
|
|
||
| test('given: nested structures, should: pass', () => { | ||
| assert({ | ||
| given: 'two identical nested structures', | ||
| should: 'be deeply equal', | ||
| actual: { users: [{ name: 'Alice' }, { name: 'Bob' }] }, | ||
| expected: { users: [{ name: 'Alice' }, { name: 'Bob' }] }, | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The test file only covers happy path scenarios where all assertions pass. Based on the vitest test suite (source/vitest.test.jsx lines 72-107), the bun adapter tests should also include:
- Tests for assert() error handling when required parameters are missing (e.g., calling assert({}) or assert({ given: 'x', should: 'y' }) without actual/expected)
- Tests to verify the error message format matches: "The following parameters are required by
assert: [missing keys]" - Tests for undefined values to ensure they don't throw errors when all parameters are provided
These tests would verify that the parameter validation logic in source/bun.js works correctly.
| "./bun": { | ||
| "import": "./source/bun.js", | ||
| "types": "./bun.d.ts" | ||
| }, |
There was a problem hiding this comment.
For consistency with the vitest adapter (which has exports at lines 54-66 including "./esm/vitest.js" and "./esm/vitest.test.jsx"), consider adding corresponding esm exports for the bun adapter. This would include:
- "./esm/bun.js" pointing to "./source/bun.js" with types "./bun.d.ts"
- "./esm/bun.test.js" pointing to "./source/bun.test.js"
These exports appear to support the package's esm build process (see the "esm" script at line 77).
9b4e8a2 to
69d5dab
Compare
Add riteway/bun adapter for Bun's built-in test runner, following the
same pattern as the existing Vitest integration.
- Add source/bun.js with assert function, setupRitewayBun, and re-exported test/describe
- Add bun.d.ts TypeScript definitions
- Add source/bun.test.js with tests
- Update package.json exports
- Update README.md with Bun documentation
- Replace Travis CI with GitHub Actions
Usage:
import { test, describe, assert, setupRitewayBun } from 'riteway/bun';
setupRitewayBun();
test('example', () => {
assert({ given: 'x', should: 'y', actual: 1, expected: 1 });
});
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
69d5dab to
08e0061
Compare
Code Review: PR #396 -
|
| Concern | Files | In Scope? | Notes |
|---|---|---|---|
| Bun adapter | source/bun.js, source/bun.test.js, bun.d.ts, package.json (exports), README.md, vitest.config.js |
✅ Yes | Core feature, properly implemented |
| CI migration | .github/workflows/ci.yml, .travis.yml (deleted) |
Explicitly requested in PR comments; aligns with Context7 GitHub Action epic in tasks/ |
|
| bin rename + TS config | bin/riteway → bin/riteway.js, bin/riteway.test.js, package.json (typecheck with --skipLibCheck) |
ESM requirement fix + type suppression; would benefit from its own commit |
Conclusion: While the PR technically introduces scope creep by bundling CI migration and bin fixes, these changes are pragmatic given:
- CI migration was explicitly requested and aligns with planned infrastructure work
- The bin extension fix resolves ESM import issues
- All changes are related to test infrastructure modernization
Test Coverage Gap (Blocking)
Current Coverage Analysis:
source/bun.test.js (lines 5-50) only tests passing assertions across data types:
- ✅ Primitives (numbers, strings)
- ✅ Objects (shallow and nested)
- ✅ Arrays
Missing Critical Test Cases:
Comparing to source/vitest.test.jsx:72-107 and source/test.js:79-111:
| Test Scenario | Core/Vitest | Bun | Impact |
|---|---|---|---|
| Missing all required keys | ✅ | ❌ | Can't verify error handling works |
| Missing partial keys | ✅ | ❌ | Can't verify which keys are validated |
| Undefined values don't throw | ✅ | ❌ | Can't verify edge case handling |
| Error message format validation | ✅ | ❌ | Can't verify helpful error messages |
| Failure assertion scenarios | ✅ | ❌ | Can't verify matcher output format |
Required Test Additions:
// 1. Missing required keys validation
test('given: missing required keys, should: throw with helpful message', () => {
let error;
try {
assert({ given: 'test' }); // missing should, actual, expected
} catch (e) {
error = e;
}
assert({
given: 'incomplete assertion object',
should: 'throw error',
actual: error instanceof Error,
expected: true
});
assert({
given: 'incomplete assertion object',
should: 'list missing keys in error message',
actual: error.message.includes('should, actual, expected'),
expected: true
});
});
// 2. Undefined values should not throw
test('given: undefined values, should: not throw validation error', () => {
let error;
try {
assert({
given: 'test',
should: 'handle undefined',
actual: undefined,
expected: undefined
});
} catch (e) {
error = e;
}
assert({
given: 'undefined in actual and expected',
should: 'not throw validation error',
actual: error === undefined,
expected: true
});
});
// 3. Failure case with proper context
test('given: failing assertion, should: include given/should context', () => {
let error;
try {
assert({
given: 'two different numbers',
should: 'be equal',
actual: 42,
expected: 43
});
} catch (e) {
error = e;
}
assert({
given: 'failing assertion',
should: 'include given context',
actual: error.message.includes('two different numbers'),
expected: true
});
assert({
given: 'failing assertion',
should: 'include should context',
actual: error.message.includes('be equal'),
expected: true
});
});Implementation Quality Analysis
Strengths:
-
✅ Functional programming principles — Follows
javascript.mdcguidelines:- Uses arrow functions, destructuring, parameter defaults
- Pure functions with clear single responsibilities
- No mutation, proper const usage
-
✅ Pattern consistency — Matches Vitest adapter architecture:
assert()validates required keys before delegation- Re-exports
testanddescribefrom native runner - Utility functions (
Try,countKeys) importable from core
-
✅ Custom matcher implementation — Correctly follows Bun's API:
- Uses
this.equals()for deep equality comparison - Uses
this.utils.printExpected/printReceivedfor formatting - Returns
{ pass: boolean, message: () => string }as required
- Uses
-
✅ TypeScript definitions — Clean, type-safe interface:
- Generic
<T>allows type inference readonlyproperties enforce immutability- Proper module declaration with re-exports
- Generic
-
✅ Documentation — Comprehensive README section:
- Clear installation and setup instructions
- Documents
preloadconfiguration pattern - Includes practical examples and failure output
Code Quality Details:
// source/bun.js:33-42 - assert implementation
export const assert = (args = {}) => {
const missing = requiredKeys.filter((k) => !Object.keys(args).includes(k));
if (missing.length) {
throw new Error(
`The following parameters are required by \`assert\`: ${missing.join(', ')}`
);
}
const { given, should, actual, expected } = args;
expect(actual).toRitewayEqual(expected, given, should);
};Observation: Uses .join(', ') while core uses .reduce(concatToString, ''). Both produce identical output, but .join() is cleaner (KISS principle). Consider standardizing core to match.
Non-Blocking Issues
Priority 1: Setup Fragility
setupRitewayBun() must be called before tests run. If users misconfigure preload or forget to call it, they get:
TypeError: toRitewayEqual is not a function
Improvement options:
- Add lazy initialization check in
assert() - Provide clearer error message if matcher not registered
- Document common setup errors in README
Priority 2: TypeScript Configuration Issues
// package.json:74 - overly broad suppression
"typecheck": "... --skipLibCheck ..."Problems:
- Suppresses ALL library type errors, not just
bun:test - Could mask regressions in other dependencies
@ts-nocheckinbin/riteway.test.jsis blanket suppression
Better approach:
// Target specific file exclusion
"typecheck": "... && npx -p typescript tsc bun.d.ts --noEmit --skipLibCheck ..."Priority 3: TypeScript Enhancement
Per Bun best practices, add declaration merging for better IDE support:
// Add to bun.d.ts
declare module 'bun:test' {
interface Matchers<T> {
toRitewayEqual(expected: T, given: string, should: string): void;
}
interface AsymmetricMatchers {
toRitewayEqual(expected: unknown, given: string, should: string): void;
}
}Priority 4: CI Workflow Improvements
# .github/workflows/ci.yml:45-52
- name: Setup Bun
uses: oven-sh/setup-bun@v2 # Not pinned - uses latestRecommendations:
- Pin Bun version for reproducibility:
with: bun-version: 1.1.29 - Add Node v22 LTS to test matrix (currently only tests v20)
- Add Bun dependency caching:
cache: 'bun'(like npm cache)
Priority 5: Missing ESM Export Path
Vitest has ./esm/vitest.js export mapping, Bun adapter doesn't:
// Missing from package.json exports
"./esm/bun.js": {
"import": "./source/bun.js",
"types": "./bun.d.ts"
}Consider adding for consistency with existing adapter patterns.
Priority 6: Documentation Gap
README doesn't mention utility functions aren't re-exported. Add:
### Utility Functions
The Bun adapter focuses on the core `assert` API. For utility functions like `Try`, `countKeys`, or `createStream`, import them from the main riteway module:
```typescript
import { Try, countKeys } from 'riteway';
import { test, describe, assert } from 'riteway/bun';
### CI/CD Configuration Review
**GitHub Actions Workflow Analysis:**
```yaml
jobs:
test: # Node.js test job
- npm install --legacy-peer-deps
- npm run lint
- npm run typecheck
- npm test
bun-test: # Separate Bun job
- bun install
- bun test source/bun.test.js
Strengths:
- ✅ Separate jobs provide isolation
- ✅ Uses official
oven-sh/setup-bun@v2action - ✅ Uses latest GitHub Actions versions (v4)
- ✅
workflow_dispatchallows manual runs - ✅ Maintains existing Node test pipeline
Infrastructure Context:
Aligns with Context7 GitHub Action epic (tasks/2025-01-27-context7-github-action-epic.md) which planned GitHub Actions infrastructure. This PR lays necessary groundwork.
Security & Performance
Security:
- ✅ No new dependencies added
- ✅ Uses Bun's built-in APIs (no third-party matcher libraries)
- ✅ Input validation prevents partial assertions
- ✅ No security-sensitive file changes
Performance:
- ✅ Bun is significantly faster than Node for test execution
- ✅ Parallel job execution in CI
- ✅ No performance regressions to existing test runners
- ✅ All 37 tests pass in < 2 seconds locally
Backward Compatibility & Regressions
Analysis:
| Component | Status | Evidence |
|---|---|---|
| Core riteway tests | ✅ No regression | 31/31 tests pass |
| Vitest adapter | ✅ Unaffected | 6/6 tests pass |
| CLI functionality | ✅ Unchanged | bin tests pass |
| All exports | ✅ Maintained | Type checks pass |
| Travis CI | Replaced by GitHub Actions |
Verdict: No functional regressions. Travis → GitHub Actions is an infrastructure upgrade, not a regression.
Adapter Pattern Consistency
Comparing implementation across all test runners:
| Feature | Core (tape) | Vitest | Bun | Assessment |
|---|---|---|---|---|
assert() with validation |
✅ | ✅ | ✅ | ✅ Consistent |
describe |
✅ Native | ✅ Appropriate delegation | ||
test |
N/A | ✅ Appropriate delegation | ||
.only() / .skip() |
✅ | ✅ Native | ✅ Native | ✅ Available via re-exports |
Try utility |
✅ | ❌ | ❌ | ✅ Import from core |
countKeys utility |
✅ | ❌ | ❌ | ✅ Import from core |
createStream |
✅ | ❌ | ❌ | ✅ Import from core |
| Setup required | ❌ | ❌ |
Conclusion: Bun adapter correctly follows established Vitest adapter pattern. Setup requirement is unavoidable due to Bun's expect.extend() API design.
TDD Compliance (per tdd.mdc)
5 Questions Framework:
Checking source/bun.test.js:5-13:
describe('riteway/bun', () => {
test('given: matching primitives, should: pass', () => {
assert({
given: 'two identical numbers', // ✅ Question 2: Expected behavior
should: 'be equal', // ✅ Question 2: Expected behavior
actual: 42, // ✅ Question 3: Actual output
expected: 42, // ✅ Question 4: Expected output
});
});- ✅ Unit under test? →
riteway/bun(describe block) - ✅ Expected behavior? → Clear given/should statements
- ✅ Actual output? → Explicitly tested
- ✅ Expected output? → Explicitly declared
- ✅ How to reproduce? → Complete test context provided
Gap: Only tests passing cases. Missing failure scenarios means Question 5 isn't fully answered for error conditions.
Official Documentation Alignment
Bun Test Runner Best Practices (verified via web search):
From Bun documentation:
-
✅ Custom matcher signature correct:
expect.extend({ matcherName(received, ...args) { return { pass: boolean, message: () => string }; } });
-
✅ Uses
this.equals()for deep equality (recommended over manual comparison) -
✅ Uses
this.utilsfor formatting (printExpected/printReceived) -
⚠️ Missing TypeScript declaration merging (recommended for full type support)
Summary & Recommendations
❌ Blocking (Must Fix Before Merge)
- Add comprehensive error case tests to
source/bun.test.js:- Missing required keys validation
- Partial missing keys error messages
- Undefined values handling
- Failure scenario output format
⚠️ High Priority (Recommended)
-
Improve TypeScript configuration:
- Replace blanket
--skipLibCheckwith targeted exclusion - Remove
@ts-nocheckfrombin/riteway.test.js, use targeted suppressions - Add Bun matcher type declaration merging
- Replace blanket
-
Enhance setup robustness:
- Add guard message if
setupRitewayBun()not called - Document common setup errors in README
- Add guard message if
📝 Medium Priority (Nice to Have)
-
CI workflow improvements:
- Pin Bun version for reproducibility
- Add Node v22 to test matrix
- Add Bun dependency caching
-
Documentation enhancement:
- Add note about importing utility functions from core
- Document
.only()and.skip()availability
-
Package consistency:
- Add
./esm/bun.jsexport mapping
- Add
Final Verdict
Once test coverage is addressed, this PR will be ready to merge.
Positive Notes:
- Clean, functional code following project standards
- Proper documentation and TypeScript definitions
- Good CI infrastructure modernization
- No regressions to existing functionality
- Aligns with planned infrastructure work


Summary
riteway/bunadapter that provides the familiarassertAPI with Bun's fast, built-in test runnerbun.d.ts)Test plan
bun test source/bun.test.jsto verify the adapter works correctlyimport { test, describe, assert } from 'riteway/bun'givenandshouldcontext🤖 Generated with Claude Code