[ENG-2103] add E2E test helper class with CLI runner, retry utilities…#433
[ENG-2103] add E2E test helper class with CLI runner, retry utilities…#433leehpham wants to merge 5 commits intoproj/e2e-testsfrom
Conversation
…, and BrvE2EHelper.
PR Review: [ENG-2103] E2E Test Helpers
|
|
Claude finished @leehpham's task in 2m 45s —— View job PR Review: [ENG-2103] E2E Test Helpers
SummaryScore: 4 / 5 Solid E2E test infrastructure. The design is clean — Issues to fix before merge:
What's good:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // so tsx resolves correctly regardless of the child process cwd | ||
| const nodeArgs = ['--import', TSX_IMPORT_PATH, '--no-warnings', BIN_DEV_PATH, ...args] | ||
|
|
||
| return new Promise((resolve) => { |
There was a problem hiding this comment.
issue (naming/shadowing): The Promise callback parameter resolve shadows the resolve import from node:path. This will likely trigger the no-shadow ESLint rule and is genuinely confusing — a reader seeing resolve({exitCode, ...}) on line 52 has to mentally track which resolve is in scope.
| return new Promise((resolve) => { | |
| return new Promise((fulfill) => { |
Then update lines 52 and 58 to use fulfill(...) instead. Fix this →
| const PROJECT_ROOT = resolve(dirname(fileURLToPath(import.meta.url)), '..', '..', '..') | ||
| const BIN_DEV_PATH = resolve(PROJECT_ROOT, 'bin', 'dev.js') | ||
| // Resolve tsx from project root so it works even when cwd is a temp dir | ||
| const TSX_IMPORT_PATH = resolve(PROJECT_ROOT, 'node_modules', 'tsx', 'dist', 'esm', 'index.mjs') |
There was a problem hiding this comment.
nitpick (fragile path): Hardcoding tsx/dist/esm/index.mjs couples the runner to tsx's internal directory layout. If tsx restructures its package (as it has done between versions), this silently breaks E2E runs.
Consider resolving via the bin field instead:
// More resilient: find tsx via its package.json#bin map
const TSX_BIN_PATH = resolve(PROJECT_ROOT, 'node_modules', '.bin', 'tsx')Or use require.resolve/import.meta.resolve with a try-catch fallback. Either approach survives tsx version bumps.
| throw new Error(`No valid JSON found in CLI output.\nstdout: ${result.stdout}\nstderr: ${result.stderr}`) | ||
| } | ||
|
|
||
| async setup(): Promise<void> { |
There was a problem hiding this comment.
issue (resource leak): setup() is not idempotent. Calling it twice creates a new temp dir and overwrites _cwd, orphaning the first directory without cleaning it up.
Add a guard (or call cleanup() first) to prevent the leak:
| async setup(): Promise<void> { | |
| async setup(): Promise<void> { | |
| if (this._cwd) { | |
| await this.cleanup() | |
| } | |
| const dir = realpathSync(mkdtempSync(join(tmpdir(), 'brv-e2e-'))) |
Alternatively, simply throw if already set up — forcing explicit cleanup before re-use. Fix this →
| expect(existsSync(configPath)).to.be.true | ||
|
|
||
| const config = JSON.parse(readFileSync(configPath, 'utf8')) | ||
| expect(config).to.deep.equal({version: '0.0.1'}) |
There was a problem hiding this comment.
issue (magic string): Hardcoding '0.0.1' couples this test to the constant's current value. If BRV_CONFIG_VERSION is bumped, this test breaks with a confusing mismatch. Use the imported constant directly:
| expect(config).to.deep.equal({version: '0.0.1'}) | |
| expect(config).to.deep.equal({version: BRV_CONFIG_VERSION}) |
BRV_CONFIG_VERSION is already imported at line 8 of brv-e2e-helper.ts — add it to this test's imports too. Fix this →
| const elapsed = Date.now() - start | ||
|
|
||
| // 2 retries * 50ms delay = at least 100ms | ||
| expect(elapsed).to.be.at.least(80) // small margin for timer imprecision |
There was a problem hiding this comment.
nitpick (flaky timing test): The 80ms lower bound on a 100ms expected delay is only a 20% margin. On a heavily loaded CI runner this can fail spuriously. A slightly tighter lower bound assertion (e.g., at.least(60)) or switching to counting calls instead of timing would make this more reliable.
Alternatively, verify that the delay utility is called rather than relying on wall-clock elapsed time — but since sleep is a module-local unexported function that pattern isn't practical here. Widening the margin to at.least(50) is the safest path.
| webAppUrl: 'http://localhost:0', | ||
| } | ||
|
|
||
| describe('BrvE2EHelper', () => { |
There was a problem hiding this comment.
nitpick (naming inconsistency): The describe block uses 'BrvE2EHelper' but the class is BrvE2eHelper. Minor, but matching the actual class name reduces confusion when searching logs or test output.
|
Claude finished @leehpham's task in 1m 52s —— View job PR Review: [ENG-2103] E2E Test Helpers
SummaryScore: 3.5 / 5 The PR was synchronized (merge commits from Outstanding Issues (from previous review, still present)
New Issues Found
...process.env as Record<string, string>,
...Object.fromEntries(Object.entries(process.env).filter(([, v]) => v !== undefined)) as Record<string, string>,
export const getE2eConfig = (): E2eConfig => {Stray double space before What's Good
|
|
Claude finished @leehpham's task in 1m 25s —— View job PR Review: [ENG-2103] E2E Test Helpers
SummaryScore: 3 / 5 This is the third review cycle. The PR was synchronized (merge commits from Outstanding Issues — All Still Present
Required Fixes Before Merge1. // before
return new Promise((resolve) => {
// after
return new Promise((fulfill) => {Update lines 50, 52, and 58 to call 2. async setup(): Promise<void> {
if (this._cwd) {
await this.cleanup()
}
const dir = realpathSync(mkdtempSync(join(tmpdir(), 'brv-e2e-')))
// ...
}3. // before
expect(config).to.deep.equal({version: '0.0.1'})
// after
expect(config).to.deep.equal({version: BRV_CONFIG_VERSION})Import 4. // before
...process.env as Record<string, string>,
// after
...Object.fromEntries(Object.entries(process.env).filter(([, v]) => v !== undefined)) as Record<string, string>,What's Good
|
1 participant
…, and BrvE2EHelper.
Summary
Type of change
Scope (select all touched areas)
Linked issues
Root cause (bug fixes only, otherwise write
N/A)Test plan
User-visible changes
List user-visible changes (including defaults, config, or CLI output).
If none, write
None.Evidence
Attach at least one:
Checklist
npm test)npm run lint)npm run typecheck)npm run build)mainRisks and mitigations
List real risks for this PR. If none, write
None.