fix(e2e): use unique SSM paths per CLI test run to prevent race conditions#130
fix(e2e): use unique SSM paths per CLI test run to prevent race conditions#130
Conversation
…tions Concurrent CI runs shared hardcoded SSM paths (/Test/Token, /Test/SingleVariable) against real AWS SSM, causing ParameterNotFound errors when one run's cleanup deleted parameters another run had just created. Each test run now generates a unique runId and creates temporary map files with isolated SSM paths (/Test/<runId>/Token). Temp files are cleaned up in afterAll.
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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 addresses a critical flakiness issue in the E2E CLI tests, particularly in CI environments where concurrent test runs could lead to race conditions. By introducing unique identifiers for each test execution and dynamically managing AWS SSM paths and local map files, the changes ensure that tests operate in isolated environments, preventing one run from interfering with another's resources. This significantly improves the reliability and stability of the E2E test suite. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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 enhances E2E tests by introducing unique identifiers for each test run, dynamically generating test-specific configuration files, and implementing cleanup logic for these temporary files. This approach prevents race conditions and ensures better test isolation. Feedback includes suggestions to improve file I/O operations: using fs.promises.writeFile with Promise.all for parallel and asynchronous file creation in beforeAll, and fs.promises.rm with force: true and Promise.all for more efficient and simplified file cleanup in afterAll.
| writeFileSync( | ||
| mapFilePath, | ||
| JSON.stringify({ TOKEN_SECRET: `${ssmPrefix}/Token` }, null, 2), | ||
| ); | ||
| writeFileSync( | ||
| mapFileWithConfigPath, | ||
| JSON.stringify( | ||
| { | ||
| $config: { provider: 'aws' }, | ||
| TOKEN_SECRET: `${ssmPrefix}/Token`, | ||
| }, | ||
| null, | ||
| 2, | ||
| ), | ||
| ); |
There was a problem hiding this comment.
In an async function like beforeAll, it's a good practice to use asynchronous file operations to avoid blocking the event loop. You can replace the synchronous writeFileSync with the asynchronous writeFile from fs/promises. This also allows you to write both files in parallel using Promise.all, which is more efficient.
To apply this suggestion, you'll need to import writeFile from node:fs/promises and adjust the existing fs imports.
| writeFileSync( | |
| mapFilePath, | |
| JSON.stringify({ TOKEN_SECRET: `${ssmPrefix}/Token` }, null, 2), | |
| ); | |
| writeFileSync( | |
| mapFileWithConfigPath, | |
| JSON.stringify( | |
| { | |
| $config: { provider: 'aws' }, | |
| TOKEN_SECRET: `${ssmPrefix}/Token`, | |
| }, | |
| null, | |
| 2, | |
| ), | |
| ); | |
| await Promise.all([ | |
| writeFile( | |
| mapFilePath, | |
| JSON.stringify({ TOKEN_SECRET: `${ssmPrefix}/Token` }, null, 2), | |
| ), | |
| writeFile( | |
| mapFileWithConfigPath, | |
| JSON.stringify( | |
| { | |
| $config: { provider: 'aws' }, | |
| TOKEN_SECRET: `${ssmPrefix}/Token`, | |
| }, | |
| null, | |
| 2, | |
| ), | |
| ), | |
| ]); |
| for (const f of [mapFilePath, mapFileWithConfigPath]) { | ||
| if (existsSync(f)) { | ||
| await unlink(f); | ||
| } | ||
| } |
There was a problem hiding this comment.
You can simplify the file cleanup by using fs.promises.rm with the force: true option, which is already imported in this file. This avoids the need for a synchronous existsSync check and will not throw an error if the file doesn't exist. Using Promise.all also performs the cleanup in parallel, which is more efficient than a sequential loop.
await Promise.all(
[mapFilePath, mapFileWithConfigPath].map((f) => rm(f, { force: true })),
);![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR updates the E2E CLI tests to avoid cross-run interference in real AWS SSM by isolating parameter names per test run, eliminating a race where one CI run could delete another run’s parameters.
Changes:
- Generate a per-run
runIdand use it to namespace SSM parameter paths under/Test/<runId>/.... - Create per-run temporary map files (
param-map-<runId>.json,param-map-with-aws-config-<runId>.json) duringbeforeAll. - Remove the generated map files during
afterAll.
Summary
Concurrent CI runs of
e2e/cli.test.tsshared hardcoded SSM paths (/Test/Token,/Test/SingleVariable) against real AWS SSM. When two runs overlapped, one run'scleanUpSsmcould delete parameters that another run had just created, causing flakyParameterNotFounderrors in the Assert phase.This fix generates a unique
runIdper test run and creates temporary map files with isolated SSM paths, eliminating the race condition.Changes
runId(randomUUID().slice(0, 8)) at describe-block scope/Test/<runId>/Tokenand/Test/<runId>/SingleVariablebeforeAll(param-map-<runId>.json,param-map-with-aws-config-<runId>.json)afterAllTesting
pnpm testpasses (19/19 unit test files)pnpm lintpassespnpm buildpassesRelated
Fixes flaky
Should_GenerateEnvironmentFile_When_ValidArgumentsAreProvidedfailure observed in publish-action CI workflow.