Refactor e2e Tests #221
Refactor e2e Tests #221
Conversation
- Introduced a new workflow in release.yml to automate the release process for macOS, Windows, and Linux. - Added a script (update-version.mjs) to update the version in package.json based on the release tag. - Configured artifact uploads for each platform and ensured proper version extraction and validation.
|
Caution Review failedThe pull request is closed. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a GitHub Actions release workflow and a version-update script; reorganizes UI E2E tests by adding many focused Playwright specs and removing several large legacy suites; and updates test fixture/setup state versions from 2 to 0 in multiple test utilities. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Comment |
Summary of ChangesHello @webdevcody, 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 significantly refactors the UI application's end-to-end test suite, aiming to improve its structure, maintainability, and clarity. The changes involve breaking down large test files into smaller, more focused modules, organizing them by feature, and introducing new tests for specific user flows. Additionally, a new script for version management and minor updates to test setup utilities are included. 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. 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. 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 significantly refactors the end-to-end tests by breaking down large, monolithic test files into smaller, more focused specs. This is an excellent improvement for maintainability and readability. The new directory structure for tests is also much clearer.
My review focuses on the new test files. I've identified some code duplication in the test setup across several new files and suggest creating a shared helper function to address this. I also found a couple of minor issues with a brittle selector and a loose regex, for which I've provided suggestions.
Overall, this is a great refactoring that will make the e2e test suite easier to manage going forward.
| const cleanVersion = version.startsWith('v') ? version.slice(1) : version; | ||
|
|
||
| // Validate version format (basic semver check) | ||
| if (!/^\d+\.\d+\.\d+/.test(cleanVersion)) { |
There was a problem hiding this comment.
The current regex /^\d+\.\d+\.\d+/ for version validation is a bit loose as it only checks the beginning of the string. This would incorrectly validate versions like 1.2.3.4 or 1.2.3-and-more. To ensure the version string strictly follows the X.Y.Z format, you should anchor the regex to the end of the string as well.
| if (!/^\d+\.\d+\.\d+/.test(cleanVersion)) { | |
| if (!/^\d+\.\d+\.\d+$/.test(cleanVersion)) { |
| test.beforeAll(async () => { | ||
| if (!fs.existsSync(TEST_TEMP_DIR)) { | ||
| fs.mkdirSync(TEST_TEMP_DIR, { recursive: true }); | ||
| } | ||
|
|
||
| projectPath = path.join(TEST_TEMP_DIR, projectName); | ||
| fs.mkdirSync(projectPath, { recursive: true }); | ||
|
|
||
| fs.writeFileSync( | ||
| path.join(projectPath, 'package.json'), | ||
| JSON.stringify({ name: projectName, version: '1.0.0' }, null, 2) | ||
| ); | ||
|
|
||
| const automakerDir = path.join(projectPath, '.automaker'); | ||
| fs.mkdirSync(automakerDir, { recursive: true }); | ||
| fs.mkdirSync(path.join(automakerDir, 'features'), { recursive: true }); | ||
| fs.mkdirSync(path.join(automakerDir, 'context'), { recursive: true }); | ||
| fs.mkdirSync(path.join(automakerDir, 'sessions'), { recursive: true }); | ||
|
|
||
| fs.writeFileSync( | ||
| path.join(automakerDir, 'categories.json'), | ||
| JSON.stringify({ categories: [] }, null, 2) | ||
| ); | ||
|
|
||
| fs.writeFileSync( | ||
| path.join(automakerDir, 'app_spec.txt'), | ||
| `# ${projectName}\n\nA test project for e2e testing.` | ||
| ); | ||
| }); |
There was a problem hiding this comment.
This beforeAll setup block for creating a test project is duplicated across several new test files (e.g., add-feature-to-backlog.spec.ts, edit-feature.spec.ts, etc.). To improve maintainability and reduce code duplication, consider extracting this logic into a shared helper function in apps/ui/tests/utils/project/setup.ts. The helper could accept options to conditionally create directories like sessions when needed.
| const featureId = cardTestId?.replace('kanban-card-', ''); | ||
|
|
||
| // Collapse the sidebar first to avoid it intercepting clicks | ||
| const collapseSidebarButton = page.locator('button:has-text("Collapse sidebar")'); |
There was a problem hiding this comment.
Using has-text for selectors can be brittle as UI text might change. It's more robust to use a data-testid attribute for test-specific element selection. Please consider adding a data-testid to this button, for example data-testid="sidebar-collapse-button", and updating the selector here.
| const collapseSidebarButton = page.locator('button:has-text("Collapse sidebar")'); | |
| const collapseSidebarButton = page.locator('[data-testid="sidebar-collapse-button"]'); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
apps/ui/tests/features/feature-manual-review-flow.spec.ts (1)
31-77: Same project setup duplication as other feature tests.This
beforeAllsetup duplicates the same project structure code found in other feature test files. Consider using the shared helper function suggested in the review offeature-skip-tests-toggle.spec.ts.For this file, you would call it with a feature in
waiting_approvalstatus:projectPath = await createTestProject(TEST_TEMP_DIR, projectName, { features: [ { id: featureId, status: 'waiting_approval', description: 'Test feature for manual review flow', }, ], });apps/ui/tests/features/edit-feature.spec.ts (1)
27-54: Same project setup duplication as other feature tests.This
beforeAllsetup duplicates the same project structure code. Refer to the refactoring suggestion infeature-skip-tests-toggle.spec.tsto extract this into a shared helper.apps/ui/tests/features/add-feature-to-backlog.spec.ts (1)
26-53: Same project setup duplication as other feature tests.This
beforeAllsetup duplicates the same project structure code. Refer to the refactoring suggestion infeature-skip-tests-toggle.spec.tsto extract this into a shared helper.
🧹 Nitpick comments (10)
apps/ui/tests/context/context-file-management.spec.ts (2)
28-30: Consider removing the redundantafterEachhook.Since
beforeEachalready resets the context directory before each test, theafterEachcleanup is redundant. If cleanup after all tests is desired, usetest.afterAll()instead.🔎 Proposed refactor
- test.afterEach(async () => { - resetContextDirectory(); - });
33-37: Simplify navigation by removing redundant page load.The test navigates to
/then immediately to/context. SincenavigateToContext()already handles navigation to/contextwith proper waiting, the initial navigation to/is unnecessary and adds overhead.🔎 Proposed refactor
await setupProjectWithFixture(page, getFixturePath()); - await page.goto('/'); - await waitForNetworkIdle(page); - await navigateToContext(page);apps/ui/tests/context/delete-context-file.spec.ts (3)
28-30: Consider removing the redundantafterEachhook.Since
beforeEachalready resets the context directory before each test, theafterEachcleanup is redundant. If cleanup after all tests is desired, usetest.afterAll()instead.🔎 Proposed refactor
- test.afterEach(async () => { - resetContextDirectory(); - });
35-39: Simplify navigation by removing redundant page load.The test navigates to
/then immediately to/context. SincenavigateToContext()already handles navigation to/contextwith proper waiting, the initial navigation to/is unnecessary and adds overhead.🔎 Proposed refactor
await setupProjectWithFixture(page, getFixturePath()); - await page.goto('/'); - await waitForNetworkIdle(page); - await navigateToContext(page);
41-56: Extract duplicated file creation logic into a utility function.The markdown file creation logic (lines 42-53) is duplicated from
context-file-management.spec.ts. This duplication makes maintenance harder if the creation flow changes.🔎 Proposed refactor
Add this utility function to
apps/ui/tests/utils/views/context.ts:export async function createMarkdownContextFile( page: Page, filename: string, content: string ): Promise<void> { await clickElement(page, 'create-markdown-button'); await page.waitForSelector('[data-testid="create-markdown-dialog"]', { timeout: 5000 }); await fillInput(page, 'new-markdown-name', filename); await fillInput(page, 'new-markdown-content', content); await clickElement(page, 'confirm-create-markdown'); await page.waitForFunction( () => !document.querySelector('[data-testid="create-markdown-dialog"]'), { timeout: 5000 } ); await waitForContextFile(page, filename, 10000); }Then use it in both test files:
- await clickElement(page, 'create-markdown-button'); - await page.waitForSelector('[data-testid="create-markdown-dialog"]', { timeout: 5000 }); - - await fillInput(page, 'new-markdown-name', fileName); - await fillInput(page, 'new-markdown-content', '# Test File\n\nThis file will be deleted.'); - - await clickElement(page, 'confirm-create-markdown'); - - await page.waitForFunction( - () => !document.querySelector('[data-testid="create-markdown-dialog"]'), - { timeout: 5000 } - ); - - // Wait for the file to appear in the list - await waitForContextFile(page, fileName, 10000); + await createMarkdownContextFile(page, fileName, '# Test File\n\nThis file will be deleted.');apps/ui/tests/context/add-context-image.spec.ts (4)
22-101: Simplify test image creation.The 80 lines of hardcoded PNG bytes are difficult to maintain and obscure test intent. Consider these alternatives:
- Store a test fixture image in the repository (e.g.,
apps/ui/tests/fixtures/test-image.png)- Use a PNG library like
pngjsto generate the image programmatically- Use a simpler format if PNG specifics aren't being tested
🔎 Proposed refactor using a fixture file
Create
apps/ui/tests/fixtures/test-image.png(a simple 1x1 image) and simplify the test:test.beforeAll(async () => { - // Create a simple test image (1x1 red PNG) const fixturePath = getFixturePath(); - testImagePath = path.join(fixturePath, '..', 'test-image.png'); - - // Create a minimal PNG (1x1 pixel red image) - const pngHeader = Buffer.from([ - 0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a, - // ... (80+ lines of bytes) - ]); - - fs.writeFileSync(testImagePath, pngHeader); + testImagePath = path.join(__dirname, '..', 'fixtures', 'test-image.png'); });Remove the
afterAllcleanup since the file is now a permanent fixture:- test.afterAll(async () => { - // Clean up test image - if (fs.existsSync(testImagePath)) { - fs.unlinkSync(testImagePath); - } - });
107-109: Consider removing the redundantafterEachhook.Since
beforeEachalready resets the context directory before each test, theafterEachcleanup is redundant. If cleanup after all tests is desired, usetest.afterAll()instead.🔎 Proposed refactor
- test.afterEach(async () => { - resetContextDirectory(); - });
119-123: Simplify navigation by removing redundant page load.The test navigates to
/then immediately to/context. SincenavigateToContext()already handles navigation to/contextwith proper waiting, the initial navigation to/is unnecessary and adds overhead.🔎 Proposed refactor
await setupProjectWithFixture(page, getFixturePath()); - await page.goto('/'); - await waitForNetworkIdle(page); - await navigateToContext(page);
125-133: Consider using consistent timeouts across tests.This test uses a 15-second timeout for
waitForContextFile(line 133), while the other context tests use 10 seconds. Unless image processing specifically requires more time, consider using the standard 10-second timeout for consistency.🔎 Proposed refactor
- await waitForContextFile(page, fileName, 15000); + await waitForContextFile(page, fileName, 10000);apps/ui/tests/features/edit-feature.spec.ts (1)
97-102: Consider using Playwright's built-in wait instead of hardcoded timeout.The hardcoded 300ms wait for sidebar animation could be flaky on slower systems or CI environments.
💡 Recommended approach
Replace the hardcoded timeout with a more robust wait condition:
const collapseSidebarButton = page.locator('button:has-text("Collapse sidebar")'); if (await collapseSidebarButton.isVisible()) { await collapseSidebarButton.click(); - await page.waitForTimeout(300); // Wait for sidebar animation + // Wait for sidebar to actually collapse by checking its state + await expect(page.locator('[data-testid="sidebar"]')).toHaveAttribute('data-state', 'collapsed', { timeout: 1000 }); }Alternatively, if there's no reliable attribute to check, use a shorter timeout with a comment explaining it's specifically for animation:
await page.waitForTimeout(200); // Brief wait for sidebar collapse animation
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
.github/workflows/release.ymlapps/ui/scripts/update-version.mjsapps/ui/tests/agent/start-new-chat-session.spec.tsapps/ui/tests/context-view.spec.tsapps/ui/tests/context/add-context-image.spec.tsapps/ui/tests/context/context-file-management.spec.tsapps/ui/tests/context/delete-context-file.spec.tsapps/ui/tests/feature-lifecycle.spec.tsapps/ui/tests/features/add-feature-to-backlog.spec.tsapps/ui/tests/features/edit-feature.spec.tsapps/ui/tests/features/feature-manual-review-flow.spec.tsapps/ui/tests/features/feature-skip-tests-toggle.spec.tsapps/ui/tests/git/worktree-integration.spec.tsapps/ui/tests/kanban-responsive-scaling.spec.tsapps/ui/tests/profiles-view.spec.tsapps/ui/tests/profiles/profiles-crud.spec.tsapps/ui/tests/project-creation.spec.tsapps/ui/tests/projects/new-project-creation.spec.tsapps/ui/tests/projects/open-existing-project.spec.tsapps/ui/tests/spec-editor-persistence.spec.tsapps/ui/tests/utils/git/worktree.tsapps/ui/tests/utils/project/fixtures.tsapps/ui/tests/utils/project/setup.tsapps/ui/tests/worktree-integration.spec.ts
💤 Files with no reviewable changes (7)
- apps/ui/tests/context-view.spec.ts
- apps/ui/tests/profiles-view.spec.ts
- apps/ui/tests/feature-lifecycle.spec.ts
- apps/ui/tests/spec-editor-persistence.spec.ts
- apps/ui/tests/kanban-responsive-scaling.spec.ts
- apps/ui/tests/project-creation.spec.ts
- apps/ui/tests/worktree-integration.spec.ts
🧰 Additional context used
🧬 Code graph analysis (7)
apps/ui/tests/context/delete-context-file.spec.ts (3)
apps/ui/tests/utils/project/fixtures.ts (3)
resetContextDirectory(46-51)setupProjectWithFixture(73-114)getFixturePath(119-121)apps/ui/tests/utils/navigation/views.ts (1)
navigateToContext(22-41)apps/ui/tests/utils/views/context.ts (3)
waitForContextFile(118-125)selectContextFile(131-147)deleteSelectedContextFile(88-93)
apps/ui/tests/profiles/profiles-crud.spec.ts (2)
apps/ui/tests/utils/project/setup.ts (1)
setupMockProjectWithProfiles(777-882)apps/ui/tests/utils/components/toasts.ts (1)
waitForSuccessToast(69-85)
apps/ui/tests/git/worktree-integration.spec.ts (1)
apps/ui/tests/utils/git/worktree.ts (6)
createTempDirPath(52-55)TestRepo(19-22)createTestGitRepo(72-108)cleanupTempDir(149-153)setupProjectWithPath(316-360)waitForBoardView(477-495)
apps/ui/tests/agent/start-new-chat-session.spec.ts (4)
apps/ui/tests/utils/git/worktree.ts (2)
createTempDirPath(52-55)cleanupTempDir(149-153).github/scripts/upload-to-r2.js (1)
fs(2-2)apps/ui/tests/utils/project/setup.ts (1)
setupRealProject(97-160)apps/ui/tests/utils/navigation/views.ts (1)
navigateToAgent(77-84)
apps/ui/tests/context/context-file-management.spec.ts (3)
apps/ui/tests/utils/project/fixtures.ts (3)
resetContextDirectory(46-51)setupProjectWithFixture(73-114)getFixturePath(119-121)apps/ui/tests/utils/navigation/views.ts (1)
navigateToContext(22-41)apps/ui/tests/utils/views/context.ts (4)
waitForContextFile(118-125)waitForFileContentToLoad(152-158)switchToEditMode(164-177)getContextEditorContent(32-35)
apps/ui/tests/features/edit-feature.spec.ts (3)
apps/ui/tests/utils/git/worktree.ts (2)
createTempDirPath(52-55)cleanupTempDir(149-153)apps/ui/tests/utils/project/setup.ts (1)
setupRealProject(97-160)apps/ui/tests/utils/views/board.ts (3)
clickAddFeature(98-103)fillAddFeatureDialog(108-153)confirmAddFeature(158-164)
apps/ui/tests/projects/new-project-creation.spec.ts (2)
apps/ui/tests/utils/git/worktree.ts (2)
createTempDirPath(52-55)cleanupTempDir(149-153)apps/ui/tests/utils/project/setup.ts (1)
setupWelcomeView(39-86)
🪛 GitHub Actions: Format Check
apps/ui/scripts/update-version.mjs
[error] 1-1: Prettier formatting check failed for 'apps/ui/scripts/update-version.mjs'. Run 'prettier --write' to fix code style issues.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e
🔇 Additional comments (26)
apps/ui/tests/projects/open-existing-project.spec.ts (1)
14-14: LGTM! Import path correctly updated to shared utilities.The import path change consolidates test utilities from a local module to a shared parent location (../utils), aligning with the PR's test reorganization. All required functions (createTempDirPath, cleanupTempDir, setupWelcomeView) are properly exported from the utils module index.
apps/ui/tests/context/context-file-management.spec.ts (1)
32-66: Test logic is well-structured.The test properly covers the happy path for creating and editing a markdown context file with appropriate waits and assertions.
apps/ui/tests/context/delete-context-file.spec.ts (1)
64-73: Verification logic is robust.The test properly verifies both UI state (file removed from list) and filesystem state (file deleted from disk) with appropriate timeout handling.
apps/ui/tests/context/add-context-image.spec.ts (1)
135-144: Verification approach is solid.The test properly verifies both UI state and filesystem state with appropriate timeout handling for eventual consistency.
apps/ui/tests/utils/git/worktree.ts (1)
356-356: Consistent version alignment across setup functions.The version changes are consistent across all three setup functions. This maintains uniformity in test initialization. The same verification requested for fixtures.ts applies here: confirm that setup-store.ts defaults to version 0.
Also applies to: 407-407, 462-462
apps/ui/tests/git/worktree-integration.spec.ts (1)
25-60: Well-structured E2E test with proper lifecycle management.The test follows good practices:
- Proper setup and teardown with beforeAll/afterAll hooks
- Cleanup of temporary resources
- Appropriate use of test utilities
- Reasonable timeout values for UI element visibility
apps/ui/tests/profiles/profiles-crud.spec.ts (1)
19-43: Clean profile creation test with good validation.The test effectively validates the profile creation flow:
- Starts with zero custom profiles
- Uses utility functions for form interactions
- Verifies success via toast notification
- Confirms profile count increment
apps/ui/tests/utils/project/setup.ts (1)
878-878: Version update aligns with STORE_VERSIONS constant.The change is consistent with the STORE_VERSIONS.SETUP_STORE constant defined at lines 7-10, which already documents that setup-store defaults to 0. This maintains consistency across the test utilities.
apps/ui/tests/agent/start-new-chat-session.spec.ts (1)
27-89: Thorough test setup with realistic project structure.The test demonstrates good practices:
- Creates a complete project directory structure with all necessary subdirectories
- Includes configuration files (package.json, categories.json, app_spec.txt)
- Uses real project setup for more realistic testing
- Validates multiple UI elements to ensure proper session initialization
- Properly cleans up temporary resources
apps/ui/tests/utils/project/fixtures.ts (1)
110-110: Verify that setup-store.ts defaults to version 0.The comment is correct. The setup-store.ts persist middleware does not specify a version parameter (lines 173-180 show only
nameandpartializeoptions), which means zustand defaults to version 0. The fixture correctly setsversion: 0to match this behavior.apps/ui/scripts/update-version.mjs (4)
1-13: LGTM!The shebang, imports, and ES module
__dirnamesetup follow standard patterns.
15-21: LGTM!Argument validation is clear and provides helpful error messages with proper exit codes.
23-24: LGTM!The logic to strip the 'v' prefix is correct and handles the expected input format.
33-46: LGTM!The file I/O logic is solid:
- Path construction correctly resolves to
apps/ui/package.json- Proper error handling with try-catch
- Pretty-prints JSON with trailing newline
- Logs the version change
.github/workflows/release.yml (7)
1-12: LGTM!The workflow trigger and matrix strategy are correctly configured for cross-platform Electron builds.
14-26: LGTM!Version extraction logic correctly strips the 'v' prefix and uses the modern
GITHUB_OUTPUTapproach for setting step outputs.
28-31: LGTM!The script invocation correctly passes the extracted version to
update-version.mjs.
33-36: LGTM!The setup step appropriately enables lockfile checking for reproducible release builds.
38-53: LGTM!Build steps are correctly conditioned per OS with appropriate environment variables (e.g., disabling macOS code signing in CI).
55-77: LGTM!Artifact upload steps correctly use glob patterns and appropriate retention policies. The artifact names are unique per OS, preventing collisions.
79-111: LGTM!The upload job correctly:
- Depends on the build job completion
- Skips draft releases
- Downloads all OS artifacts
- Publishes to the GitHub Release using standard patterns
apps/ui/tests/projects/new-project-creation.spec.ts (1)
1-52: Well-structured E2E test for project creation.The test follows good practices with proper setup/teardown, unique naming for test isolation, and comprehensive verification of both UI state and file system artifacts.
apps/ui/tests/features/feature-skip-tests-toggle.spec.ts (1)
63-106: Test logic is correct and well-structured.The test properly verifies that the skip tests badge appears by default for newly created features.
apps/ui/tests/features/feature-manual-review-flow.spec.ts (1)
83-118: Manual review flow test is well-implemented.The test correctly verifies the manual verification button appears for features in
waiting_approvalstatus and that clicking it moves the feature to theverifiedcolumn. Good use of polling assertions withtoPass()for handling async UI updates.apps/ui/tests/features/edit-feature.spec.ts (1)
60-138: Edit feature test is well-structured and comprehensive.The test properly handles potential click interception issues by collapsing the sidebar and using JavaScript click when needed. Good verification of both the UI update and dialog lifecycle.
apps/ui/tests/features/add-feature-to-backlog.spec.ts (1)
59-84: Add feature to backlog test is clean and effective.The test follows a straightforward flow and properly verifies the feature appears in the backlog using polling assertions. Well done!
| // Validate version format (basic semver check) | ||
| if (!/^\d+\.\d+\.\d+/.test(cleanVersion)) { | ||
| console.error(`Error: Invalid version format: ${cleanVersion}`); | ||
| console.error('Expected format: X.Y.Z (e.g., 1.2.3)'); | ||
| process.exit(1); | ||
| } |
There was a problem hiding this comment.
Strengthen semver validation to reject invalid suffixes.
The regex /^\d+\.\d+\.\d+/ only checks if the version starts with the pattern but doesn't anchor to the end of the string. This would incorrectly accept invalid versions like "1.2.3abc" or "1.2.3-".
🔎 Proposed fix
// Validate version format (basic semver check)
-if (!/^\d+\.\d+\.\d+/.test(cleanVersion)) {
+if (!/^\d+\.\d+\.\d+$/.test(cleanVersion)) {
console.error(`Error: Invalid version format: ${cleanVersion}`);
console.error('Expected format: X.Y.Z (e.g., 1.2.3)');
process.exit(1);
}🤖 Prompt for AI Agents
In apps/ui/scripts/update-version.mjs around lines 26 to 31, the current regex
only anchors the start and will accept invalid suffixes like "1.2.3abc"; update
the validation to anchor the end and use a stricter semver pattern — replace the
regex with a full semver-aware one (for example
/^\d+\.\d+\.\d+(?:-[0-9A-Za-z-.]+)?(?:\+[0-9A-Za-z-.]+)?$/) so versions with
invalid trailing characters are rejected, keep the existing error messages and
exit behavior.
| test.describe('Feature Skip Tests Badge', () => { | ||
| let projectPath: string; | ||
| const projectName = `test-project-${Date.now()}`; | ||
|
|
||
| test.beforeAll(async () => { | ||
| if (!fs.existsSync(TEST_TEMP_DIR)) { | ||
| fs.mkdirSync(TEST_TEMP_DIR, { recursive: true }); | ||
| } | ||
|
|
||
| projectPath = path.join(TEST_TEMP_DIR, projectName); | ||
| fs.mkdirSync(projectPath, { recursive: true }); | ||
|
|
||
| fs.writeFileSync( | ||
| path.join(projectPath, 'package.json'), | ||
| JSON.stringify({ name: projectName, version: '1.0.0' }, null, 2) | ||
| ); | ||
|
|
||
| const automakerDir = path.join(projectPath, '.automaker'); | ||
| fs.mkdirSync(automakerDir, { recursive: true }); | ||
| fs.mkdirSync(path.join(automakerDir, 'features'), { recursive: true }); | ||
| fs.mkdirSync(path.join(automakerDir, 'context'), { recursive: true }); | ||
|
|
||
| fs.writeFileSync( | ||
| path.join(automakerDir, 'categories.json'), | ||
| JSON.stringify({ categories: [] }, null, 2) | ||
| ); | ||
|
|
||
| fs.writeFileSync( | ||
| path.join(automakerDir, 'app_spec.txt'), | ||
| `# ${projectName}\n\nA test project for e2e testing.` | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract duplicated project setup into shared helper.
The project structure setup in beforeAll (lines 26-57) is nearly identical across multiple test files (feature-skip-tests-toggle.spec.ts, feature-manual-review-flow.spec.ts, edit-feature.spec.ts, add-feature-to-backlog.spec.ts). This violates DRY principles and creates a maintenance burden.
💡 Recommended refactor
Extract this setup logic into a shared utility function in apps/ui/tests/utils/project/setup.ts:
export async function createTestProject(
tempDir: string,
projectName: string,
options?: {
features?: Array<{ id: string; status: string; description: string }>;
}
): Promise<string> {
const projectPath = path.join(tempDir, projectName);
fs.mkdirSync(projectPath, { recursive: true });
fs.writeFileSync(
path.join(projectPath, 'package.json'),
JSON.stringify({ name: projectName, version: '1.0.0' }, null, 2)
);
const automakerDir = path.join(projectPath, '.automaker');
fs.mkdirSync(automakerDir, { recursive: true });
fs.mkdirSync(path.join(automakerDir, 'features'), { recursive: true });
fs.mkdirSync(path.join(automakerDir, 'context'), { recursive: true });
fs.writeFileSync(
path.join(automakerDir, 'categories.json'),
JSON.stringify({ categories: [] }, null, 2)
);
fs.writeFileSync(
path.join(automakerDir, 'app_spec.txt'),
`# ${projectName}\n\nA test project for e2e testing.`
);
// Add features if provided
if (options?.features) {
for (const feature of options.features) {
const featureDir = path.join(automakerDir, 'features', feature.id);
fs.mkdirSync(featureDir, { recursive: true });
const featureData = {
id: feature.id,
description: feature.description,
category: 'test',
status: feature.status,
skipTests: true,
model: 'sonnet',
thinkingLevel: 'none',
createdAt: new Date().toISOString(),
branchName: '',
priority: 2,
};
fs.writeFileSync(
path.join(featureDir, 'feature.json'),
JSON.stringify(featureData, null, 2)
);
}
}
return projectPath;
}Then simplify each test's beforeAll to:
test.beforeAll(async () => {
if (!fs.existsSync(TEST_TEMP_DIR)) {
fs.mkdirSync(TEST_TEMP_DIR, { recursive: true });
}
-
- projectPath = path.join(TEST_TEMP_DIR, projectName);
- fs.mkdirSync(projectPath, { recursive: true });
-
- fs.writeFileSync(
- path.join(projectPath, 'package.json'),
- JSON.stringify({ name: projectName, version: '1.0.0' }, null, 2)
- );
-
- const automakerDir = path.join(projectPath, '.automaker');
- fs.mkdirSync(automakerDir, { recursive: true });
- fs.mkdirSync(path.join(automakerDir, 'features'), { recursive: true });
- fs.mkdirSync(path.join(automakerDir, 'context'), { recursive: true });
-
- fs.writeFileSync(
- path.join(automakerDir, 'categories.json'),
- JSON.stringify({ categories: [] }, null, 2)
- );
-
- fs.writeFileSync(
- path.join(automakerDir, 'app_spec.txt'),
- `# ${projectName}\n\nA test project for e2e testing.`
- );
+ projectPath = await createTestProject(TEST_TEMP_DIR, projectName);
});Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/ui/tests/features/feature-skip-tests-toggle.spec.ts around lines 26-57,
the project setup logic is duplicated across multiple test files; extract this
logic into a shared helper (e.g. apps/ui/tests/utils/project/setup.ts) named
createTestProject(tempDir, projectName, options?) that creates the temp project
dir, .automaker tree, package.json, categories.json, app_spec.txt and optionally
creates feature subfolders with feature.json; then replace the beforeAll block
in this file (and the other tests) with a call to
createTestProject(TEST_TEMP_DIR, projectName, { features: [...] }) so tests
reuse the single utility and avoid duplication.
- Removed unnecessary blank lines in the update-version.mjs script for improved readability.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.