Conversation
Signed-off-by: Cursor Agent <cursoragent@cursor.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a crucial release smoke test for the @openfeature/flagd-ofrep-cf-worker package. The primary goal is to enhance release confidence by automatically validating the integrity and usability of the packaged artifact across different JavaScript module systems (CommonJS, ESM) and TypeScript, ensuring that consumers can successfully integrate the library. Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive release smoke test that validates the packaged tarball for @openfeature/flagd-ofrep-cf-worker. The test is well-designed, covering consumption via CommonJS, ESM, and TypeScript, which is excellent. My feedback includes a suggestion to refactor the main test function to improve its structure and maintainability.
| async function main() { | ||
| await assertBuildArtifactsExist(); | ||
|
|
||
| const tempRoot = await fs.mkdtemp(path.join(os.tmpdir(), 'release-smoke-')); | ||
|
|
||
| try { | ||
| const packDir = path.join(tempRoot, 'pack'); | ||
| await fs.mkdir(packDir, { recursive: true }); | ||
|
|
||
| // Validate the packed tarball rather than the workspace source tree. | ||
| const packOutput = run('npm', ['pack', '--json', '--pack-destination', packDir], { cwd: packageDir }); | ||
| const [packResult] = JSON.parse(packOutput); | ||
| const tarballPath = path.join(packDir, packResult.filename); | ||
| const packedFiles = new Set((packResult.files ?? []).map((file) => file.path)); | ||
|
|
||
| for (const expectedFile of requiredPackedFiles) { | ||
| assert(packedFiles.has(expectedFile), `Packed tarball is missing ${expectedFile}`); | ||
| } | ||
|
|
||
| const consumerDir = path.join(tempRoot, 'consumer'); | ||
| await fs.mkdir(consumerDir, { recursive: true }); | ||
|
|
||
| await writeFile( | ||
| path.join(consumerDir, 'package.json'), | ||
| `${JSON.stringify({ name: 'release-smoke-consumer', private: true }, null, 2)}\n`, | ||
| ); | ||
|
|
||
| run('npm', ['install', '--ignore-scripts', '--no-package-lock', tarballPath], { cwd: consumerDir }); | ||
|
|
||
| await writeFile( | ||
| path.join(consumerDir, 'consumer.cjs'), | ||
| `const assert = require('node:assert/strict'); | ||
| const { createOfrepHandler, FlagStore, OfrepHandler, extractAuthToken } = require('${packageName}'); | ||
|
|
||
| const flags = { | ||
| flags: { | ||
| 'release-smoke': { | ||
| state: 'ENABLED', | ||
| defaultVariant: 'on', | ||
| variants: { | ||
| on: true, | ||
| off: false, | ||
| }, | ||
| }, | ||
| }, | ||
| }; | ||
|
|
||
| async function main() { | ||
| const store = new FlagStore(flags); | ||
| assert.equal(store.hasFlag('release-smoke'), true); | ||
|
|
||
| const handler = createOfrepHandler({ staticFlags: flags, cors: false }); | ||
| const response = await handler( | ||
| new Request('https://example.test/ofrep/v1/evaluate/flags/release-smoke', { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: '{}', | ||
| }), | ||
| ); | ||
| const body = await response.json(); | ||
|
|
||
| assert.equal(response.status, 200); | ||
| assert.equal(body.key, 'release-smoke'); | ||
| assert.equal(body.value, true); | ||
| assert.equal(typeof OfrepHandler, 'function'); | ||
| assert.equal( | ||
| extractAuthToken(new Request('https://example.test', { headers: { authorization: 'Bearer token' } })), | ||
| 'token', | ||
| ); | ||
| } | ||
|
|
||
| main().catch((error) => { | ||
| console.error(error); | ||
| process.exitCode = 1; | ||
| }); | ||
| `, | ||
| ); | ||
|
|
||
| await writeFile( | ||
| path.join(consumerDir, 'consumer.mjs'), | ||
| `import assert from 'node:assert/strict'; | ||
| import { createOfrepHandler, FlagStore, OfrepHandler, extractAuthToken } from '${packageName}'; | ||
|
|
||
| const flags = { | ||
| flags: { | ||
| 'release-smoke': { | ||
| state: 'ENABLED', | ||
| defaultVariant: 'on', | ||
| variants: { | ||
| on: true, | ||
| off: false, | ||
| }, | ||
| }, | ||
| }, | ||
| }; | ||
|
|
||
| const store = new FlagStore(flags); | ||
| assert.equal(store.hasFlag('release-smoke'), true); | ||
|
|
||
| const handler = createOfrepHandler({ staticFlags: flags, cors: false }); | ||
| const response = await handler( | ||
| new Request('https://example.test/ofrep/v1/evaluate/flags/release-smoke', { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: '{}', | ||
| }), | ||
| ); | ||
| const body = await response.json(); | ||
|
|
||
| assert.equal(response.status, 200); | ||
| assert.equal(body.key, 'release-smoke'); | ||
| assert.equal(body.value, true); | ||
| assert.equal(typeof OfrepHandler, 'function'); | ||
| assert.equal(extractAuthToken(new Request('https://example.test', { headers: { 'x-api-key': 'key' } })), 'key'); | ||
| `, | ||
| ); | ||
|
|
||
| await writeFile( | ||
| path.join(consumerDir, 'consumer.ts'), | ||
| `import { createOfrepHandler, type JsonValue, type OfrepHandlerOptions } from '${packageName}'; | ||
|
|
||
| const options = { | ||
| staticFlags: '{"flags":{"release-smoke":{"state":"ENABLED","defaultVariant":"on","variants":{"on":true,"off":false}}}}', | ||
| cors: false, | ||
| } satisfies OfrepHandlerOptions; | ||
|
|
||
| const handler = createOfrepHandler(options); | ||
| const responsePromise: Promise<Response> = handler( | ||
| new Request('https://example.test/ofrep/v1/evaluate/flags/release-smoke', { | ||
| method: 'POST', | ||
| body: '{}', | ||
| }), | ||
| ); | ||
| const sampleValue: JsonValue = { enabled: true }; | ||
|
|
||
| void responsePromise; | ||
| void sampleValue; | ||
| `, | ||
| ); | ||
|
|
||
| await writeFile( | ||
| path.join(consumerDir, 'tsconfig.json'), | ||
| `${JSON.stringify( | ||
| { | ||
| compilerOptions: { | ||
| target: 'ES2022', | ||
| module: 'NodeNext', | ||
| moduleResolution: 'NodeNext', | ||
| strict: true, | ||
| noEmit: true, | ||
| lib: ['ES2022', 'DOM'], | ||
| }, | ||
| include: ['consumer.ts'], | ||
| }, | ||
| null, | ||
| 2, | ||
| )}\n`, | ||
| ); | ||
|
|
||
| run(process.execPath, [path.join(consumerDir, 'consumer.cjs')], { cwd: consumerDir }); | ||
| run(process.execPath, [path.join(consumerDir, 'consumer.mjs')], { cwd: consumerDir }); | ||
| run( | ||
| process.execPath, | ||
| [ | ||
| path.join(workspaceRoot, 'node_modules', 'typescript', 'bin', 'tsc'), | ||
| '--project', | ||
| path.join(consumerDir, 'tsconfig.json'), | ||
| ], | ||
| { | ||
| cwd: consumerDir, | ||
| }, | ||
| ); | ||
|
|
||
| console.log(`Release smoke test passed for ${packResult.filename}`); | ||
| } finally { | ||
| await fs.rm(tempRoot, { recursive: true, force: true }); | ||
| } | ||
| } |
There was a problem hiding this comment.
The main function is quite long and handles many distinct steps: packing the module, creating multiple consumer test files, and running them. For improved readability and long-term maintainability, consider refactoring this function by extracting logical blocks into smaller, single-purpose functions (e.g., createCjsConsumerFile, runTsChecks).
This structural change would also make it easier to address the duplication of the flags object, which is currently defined multiple times within the template strings for the generated consumer files. The flags object could be defined once and passed to the new helper functions.
2 participants
Summary
@openfeature/flagd-ofrep-cf-workerTesting
npm run buildnpm testnpm run test:smoke:releasenpm run test:smoke:workernpx prettier --check README.md package.json .github/workflows/ci.yml scripts/release-smoke-test.js