-
Notifications
You must be signed in to change notification settings - Fork 438
[ENG-2103] add E2E test helper class with CLI runner, retry utilities… #433
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: proj/e2e-tests
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,158 @@ | ||||||
| import {expect} from 'chai' | ||||||
| import {existsSync, readFileSync} from 'node:fs' | ||||||
| import {join} from 'node:path' | ||||||
|
|
||||||
| import type {E2eConfig} from './env-guard.js' | ||||||
|
|
||||||
| import {BrvE2eHelper} from './brv-e2e-helper.js' | ||||||
| import {getE2eConfig, requireE2eEnv} from './env-guard.js' | ||||||
|
|
||||||
| const dummyConfig: E2eConfig = { | ||||||
| apiBaseUrl: 'http://localhost:0', | ||||||
| apiKey: 'test-key', | ||||||
| cogitApiBaseUrl: 'http://localhost:0', | ||||||
| gitRemoteBaseUrl: 'http://localhost:0', | ||||||
| llmApiBaseUrl: 'http://localhost:0', | ||||||
| webAppUrl: 'http://localhost:0', | ||||||
| } | ||||||
|
|
||||||
| describe('BrvE2EHelper', () => { | ||||||
| describe('mechanics', () => { | ||||||
| let helper: BrvE2eHelper | ||||||
|
|
||||||
| beforeEach(() => { | ||||||
| helper = new BrvE2eHelper(dummyConfig) | ||||||
| }) | ||||||
|
|
||||||
| afterEach(async () => { | ||||||
| await helper.cleanup() | ||||||
| }) | ||||||
|
|
||||||
| it('should instantiate with E2eConfig', () => { | ||||||
| expect(helper).to.be.instanceOf(BrvE2eHelper) | ||||||
| }) | ||||||
|
|
||||||
| it('should throw when accessing cwd before setup()', () => { | ||||||
| expect(() => helper.cwd).to.throw('setup() must be called') | ||||||
| }) | ||||||
|
|
||||||
| it('should create a temp directory with .brv/config.json on setup()', async () => { | ||||||
| await helper.setup() | ||||||
|
|
||||||
| expect(helper.cwd).to.be.a('string').that.is.not.empty | ||||||
| expect(existsSync(helper.cwd)).to.be.true | ||||||
|
|
||||||
| const configPath = join(helper.cwd, '.brv', 'config.json') | ||||||
| expect(existsSync(configPath)).to.be.true | ||||||
|
|
||||||
| const config = JSON.parse(readFileSync(configPath, 'utf8')) | ||||||
| expect(config).to.deep.equal({version: '0.0.1'}) | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. issue (magic string): Hardcoding
Suggested change
|
||||||
| }) | ||||||
|
|
||||||
| it('should remove the temp directory on cleanup()', async () => { | ||||||
| await helper.setup() | ||||||
| const dir = helper.cwd | ||||||
|
|
||||||
| await helper.cleanup() | ||||||
|
|
||||||
| expect(existsSync(dir)).to.be.false | ||||||
| expect(() => helper.cwd).to.throw('setup() must be called') | ||||||
| }) | ||||||
|
|
||||||
| it('should run all registered teardown functions during cleanup() in reverse order', async () => { | ||||||
| await helper.setup() | ||||||
|
|
||||||
| const order: number[] = [] | ||||||
| helper.onTeardown(async () => { order.push(1) }) | ||||||
| helper.onTeardown(async () => { order.push(2) }) | ||||||
| helper.onTeardown(async () => { order.push(3) }) | ||||||
|
|
||||||
| await helper.cleanup() | ||||||
|
|
||||||
| expect(order).to.deep.equal([3, 2, 1]) | ||||||
| }) | ||||||
|
|
||||||
| it('should be safe to call cleanup() multiple times', async () => { | ||||||
| await helper.setup() | ||||||
| await helper.cleanup() | ||||||
| await helper.cleanup() // should not throw | ||||||
| }) | ||||||
|
|
||||||
| it('should still cleanup temp dir if a teardown throws', async () => { | ||||||
| await helper.setup() | ||||||
| const dir = helper.cwd | ||||||
|
|
||||||
| const ran: number[] = [] | ||||||
| helper.onTeardown(async () => { ran.push(1) }) | ||||||
| helper.onTeardown(async () => { throw new Error('teardown failed') }) | ||||||
| helper.onTeardown(async () => { ran.push(3) }) | ||||||
|
|
||||||
| // cleanup should not throw despite the failing teardown | ||||||
| await helper.cleanup() | ||||||
|
|
||||||
| expect(existsSync(dir)).to.be.false | ||||||
| expect(ran).to.deep.equal([3, 1]) // reverse order, skipping the one that threw | ||||||
| }) | ||||||
|
|
||||||
| it('should run a CLI command and return the result', async () => { | ||||||
| await helper.setup() | ||||||
|
|
||||||
| const result = await helper.run('--help') | ||||||
|
|
||||||
| expect(result.exitCode).to.equal(0) | ||||||
| expect(result.stdout).to.include('USAGE') | ||||||
| expect(result.stderr).to.be.a('string') | ||||||
| }) | ||||||
|
|
||||||
| it('should throw when runJson() receives non-JSON output', async () => { | ||||||
| await helper.setup() | ||||||
|
|
||||||
| try { | ||||||
| await helper.runJson('--help') | ||||||
| expect.fail('should have thrown') | ||||||
| } catch (error) { | ||||||
| expect(error).to.be.instanceOf(Error) | ||||||
| expect((error as Error).message).to.include('No valid JSON') | ||||||
| } | ||||||
| }) | ||||||
| }) | ||||||
|
|
||||||
| describe('auth (requires E2E env)', () => { | ||||||
| before(requireE2eEnv) | ||||||
|
|
||||||
| let helper: BrvE2eHelper | ||||||
|
|
||||||
| beforeEach(async () => { | ||||||
| const config = getE2eConfig() | ||||||
| helper = new BrvE2eHelper(config) | ||||||
| await helper.setup() | ||||||
| }) | ||||||
|
|
||||||
| afterEach(async () => { | ||||||
| await helper.cleanup() | ||||||
| }) | ||||||
|
|
||||||
| it('should login with the configured API key', async () => { | ||||||
| const result = await helper.login() | ||||||
|
|
||||||
| // login() returns void on success, throws on failure | ||||||
| expect(result).to.be.undefined | ||||||
| }) | ||||||
|
|
||||||
| it('should logout after login', async () => { | ||||||
| await helper.login() | ||||||
| const result = await helper.logout() | ||||||
|
|
||||||
| expect(result).to.be.undefined | ||||||
| }) | ||||||
|
|
||||||
| it('should parse JSON response via runJson()', async () => { | ||||||
| const result = await helper.runJson<{userEmail?: string}>('login', ['--api-key', getE2eConfig().apiKey]) | ||||||
|
|
||||||
| expect(result).to.have.property('command', 'login') | ||||||
| expect(result).to.have.property('success').that.is.a('boolean') | ||||||
| expect(result).to.have.property('data').that.is.an('object') | ||||||
| expect(result).to.have.property('timestamp').that.is.a('string') | ||||||
| }) | ||||||
| }) | ||||||
| }) | ||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,123 @@ | ||||||||||||||
| import {mkdirSync, mkdtempSync, realpathSync, rmSync, writeFileSync} from 'node:fs' | ||||||||||||||
| import {tmpdir} from 'node:os' | ||||||||||||||
| import {join} from 'node:path' | ||||||||||||||
|
|
||||||||||||||
| import type {CLIResult} from './cli-runner.js' | ||||||||||||||
| import type {E2eConfig} from './env-guard.js' | ||||||||||||||
|
|
||||||||||||||
| import {BRV_CONFIG_VERSION, BRV_DIR, PROJECT_CONFIG_FILE} from '../../../src/server/constants.js' | ||||||||||||||
| import {runBrv} from './cli-runner.js' | ||||||||||||||
|
|
||||||||||||||
| export type JsonResult<T> = { | ||||||||||||||
| command: string | ||||||||||||||
| data: T | ||||||||||||||
| success: boolean | ||||||||||||||
| timestamp: string | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| export type RunOptions = { | ||||||||||||||
| env?: Record<string, string> | ||||||||||||||
| timeout?: number | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| export class BrvE2eHelper { | ||||||||||||||
| private _cwd: string | undefined | ||||||||||||||
| private readonly config: E2eConfig | ||||||||||||||
| private teardowns: Array<() => Promise<void>> = [] | ||||||||||||||
|
|
||||||||||||||
| constructor(config: E2eConfig) { | ||||||||||||||
| this.config = config | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| get cwd(): string { | ||||||||||||||
| if (!this._cwd) { | ||||||||||||||
| throw new Error('setup() must be called before accessing cwd') | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| return this._cwd | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| async cleanup(): Promise<void> { | ||||||||||||||
| if (!this._cwd) return | ||||||||||||||
|
|
||||||||||||||
| const dir = this._cwd | ||||||||||||||
|
|
||||||||||||||
| // Run teardowns in reverse order (LIFO), continue even if one throws | ||||||||||||||
| for (let i = this.teardowns.length - 1; i >= 0; i--) { | ||||||||||||||
| try { | ||||||||||||||
| // eslint-disable-next-line no-await-in-loop | ||||||||||||||
| await this.teardowns[i]() | ||||||||||||||
| } catch { | ||||||||||||||
| // Swallow — cleanup must always complete | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| this.teardowns = [] | ||||||||||||||
| this._cwd = undefined | ||||||||||||||
| rmSync(dir, {force: true, recursive: true}) | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| async login(): Promise<void> { | ||||||||||||||
| const result = await this.runJson('login', ['--api-key', this.config.apiKey]) | ||||||||||||||
| if (!result.success) { | ||||||||||||||
| throw new Error(`Login failed: ${JSON.stringify(result.data)}`) | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // Auto-register logout as teardown | ||||||||||||||
| this.onTeardown(async () => { | ||||||||||||||
| try { | ||||||||||||||
| await this.runJson('logout') | ||||||||||||||
| } catch { | ||||||||||||||
| // Best-effort logout during cleanup | ||||||||||||||
| } | ||||||||||||||
| }) | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| async logout(): Promise<void> { | ||||||||||||||
| const result = await this.runJson('logout') | ||||||||||||||
| if (!result.success) { | ||||||||||||||
| throw new Error(`Logout failed: ${JSON.stringify(result.data)}`) | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| onTeardown(fn: () => Promise<void>): void { | ||||||||||||||
| this.teardowns.push(fn) | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| async run(command: string, args?: string[], opts?: RunOptions): Promise<CLIResult> { | ||||||||||||||
| return runBrv({ | ||||||||||||||
| args: [command, ...(args ?? [])], | ||||||||||||||
| config: this.config, | ||||||||||||||
| cwd: this.cwd, | ||||||||||||||
| ...opts, | ||||||||||||||
| }) | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| async runJson<T>(command: string, args?: string[], opts?: RunOptions): Promise<JsonResult<T>> { | ||||||||||||||
| const result = await this.run(command, [...(args ?? []), '--format', 'json'], opts) | ||||||||||||||
| const lines = result.stdout.trim().split('\n') | ||||||||||||||
|
|
||||||||||||||
| // Find the last valid JSON line (CLI may print non-JSON before it) | ||||||||||||||
| for (let i = lines.length - 1; i >= 0; i--) { | ||||||||||||||
| const line = lines[i].trim() | ||||||||||||||
| if (!line) continue | ||||||||||||||
| try { | ||||||||||||||
| return JSON.parse(line) as JsonResult<T> | ||||||||||||||
| } catch { | ||||||||||||||
| // Not JSON, try next line | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| throw new Error(`No valid JSON found in CLI output.\nstdout: ${result.stdout}\nstderr: ${result.stderr}`) | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| async setup(): Promise<void> { | ||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. issue (resource leak): Add a guard (or call
Suggested change
Alternatively, simply throw if already set up — forcing explicit cleanup before re-use. Fix this → |
||||||||||||||
| const dir = realpathSync(mkdtempSync(join(tmpdir(), 'brv-e2e-'))) | ||||||||||||||
| const brvDir = join(dir, BRV_DIR) | ||||||||||||||
|
|
||||||||||||||
| mkdirSync(brvDir, {recursive: true}) | ||||||||||||||
| writeFileSync(join(brvDir, PROJECT_CONFIG_FILE), JSON.stringify({version: BRV_CONFIG_VERSION})) | ||||||||||||||
|
|
||||||||||||||
| this._cwd = dir | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| import {expect} from 'chai' | ||
|
|
||
| import type {E2eConfig} from './env-guard.js' | ||
|
|
||
| import {runBrv} from './cli-runner.js' | ||
|
|
||
| const dummyConfig: E2eConfig = { | ||
| apiBaseUrl: 'http://localhost:0', | ||
| apiKey: 'test-key', | ||
| cogitApiBaseUrl: 'http://localhost:0', | ||
| gitRemoteBaseUrl: 'http://localhost:0', | ||
| llmApiBaseUrl: 'http://localhost:0', | ||
| webAppUrl: 'http://localhost:0', | ||
| } | ||
|
|
||
| describe('runBrv', () => { | ||
| it('should capture stdout from a successful command', async () => { | ||
| const result = await runBrv({args: ['--help'], config: dummyConfig}) | ||
|
|
||
| expect(result.exitCode).to.equal(0) | ||
| expect(result.stdout).to.be.a('string').and.to.include('USAGE') | ||
| expect(result.stderr).to.be.a('string') | ||
| }) | ||
|
|
||
| it('should return non-zero exit code for invalid commands without throwing', async () => { | ||
| const result = await runBrv({args: ['nonexistent-command-xyz'], config: dummyConfig}) | ||
|
|
||
| expect(result.exitCode).to.not.equal(0) | ||
| expect(result.stderr).to.be.a('string').that.is.not.empty | ||
| }) | ||
|
|
||
| it('should pass command arguments correctly', async () => { | ||
| const result = await runBrv({args: ['login', '--help'], config: dummyConfig}) | ||
|
|
||
| expect(result.exitCode).to.equal(0) | ||
| expect(result.stdout).to.include('api-key') | ||
| }) | ||
|
|
||
| it('should accept a custom timeout option', async () => { | ||
| const result = await runBrv({args: ['--help'], config: dummyConfig, timeout: 30_000}) | ||
|
|
||
| expect(result.exitCode).to.equal(0) | ||
| expect(result.stdout).to.include('USAGE') | ||
| }) | ||
| }) |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,61 @@ | ||||||
| import {execFile} from 'node:child_process' | ||||||
| import {dirname, resolve} from 'node:path' | ||||||
| import {fileURLToPath} from 'node:url' | ||||||
|
|
||||||
| import type {E2eConfig} from './env-guard.js' | ||||||
|
|
||||||
| export type CLIResult = { | ||||||
| exitCode: number | ||||||
| stderr: string | ||||||
| stdout: string | ||||||
| } | ||||||
|
|
||||||
| export type RunBrvOptions = { | ||||||
| args: string[] | ||||||
| config: E2eConfig | ||||||
| cwd?: string | ||||||
| env?: Record<string, string> | ||||||
| timeout?: number | ||||||
| } | ||||||
|
|
||||||
| 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') | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nitpick (fragile path): Hardcoding Consider resolving via the // More resilient: find tsx via its package.json#bin map
const TSX_BIN_PATH = resolve(PROJECT_ROOT, 'node_modules', '.bin', 'tsx')Or use |
||||||
|
|
||||||
| export function runBrv(opts: RunBrvOptions): Promise<CLIResult> { | ||||||
| const {args, config, cwd, env, timeout = 60_000} = opts | ||||||
|
|
||||||
| const childEnv: Record<string, string> = { | ||||||
| ...process.env as Record<string, string>, | ||||||
| BRV_API_BASE_URL: config.apiBaseUrl, | ||||||
| BRV_COGIT_API_BASE_URL: config.cogitApiBaseUrl, | ||||||
| BRV_E2E_API_KEY: config.apiKey, | ||||||
| BRV_ENV: 'development', | ||||||
| BRV_GIT_REMOTE_BASE_URL: config.gitRemoteBaseUrl, | ||||||
| BRV_LLM_API_BASE_URL: config.llmApiBaseUrl, | ||||||
| BRV_WEB_APP_URL: config.webAppUrl, | ||||||
| ...env, | ||||||
| } | ||||||
|
|
||||||
| // Use node explicitly with tsx import path instead of the shebang, | ||||||
| // 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) => { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. issue (naming/shadowing): The Promise callback parameter
Suggested change
Then update lines 52 and 58 to use |
||||||
| execFile(process.execPath, nodeArgs, {cwd, env: childEnv, maxBuffer: 10 * 1024 * 1024, timeout}, (error, stdout, stderr) => { | ||||||
| if (error) { | ||||||
| // execFile rejects on non-zero exit — extract result instead of throwing | ||||||
| const exitCode = typeof error.code === 'number' ? error.code : 1 | ||||||
| resolve({ | ||||||
| exitCode, | ||||||
| stderr: stderr || error.message, | ||||||
| stdout: stdout || '', | ||||||
| }) | ||||||
| return | ||||||
| } | ||||||
|
|
||||||
| resolve({exitCode: 0, stderr: stderr || '', stdout: stdout || ''}) | ||||||
| }) | ||||||
| }) | ||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,7 @@ | ||
| export type {JsonResult, RunOptions} from './brv-e2e-helper.js' | ||
| export {BrvE2eHelper} from './brv-e2e-helper.js' | ||
| export type {CLIResult, RunBrvOptions} from './cli-runner.js' | ||
| export {runBrv} from './cli-runner.js' | ||
| export type {E2eConfig} from './env-guard.js' | ||
| export {getE2eConfig, requireE2eEnv} from './env-guard.js' | ||
| export {retry, waitUntil} from './retry.js' |
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 (naming inconsistency): The describe block uses
'BrvE2EHelper'but the class isBrvE2eHelper. Minor, but matching the actual class name reduces confusion when searching logs or test output.