fix: re-enable ci tests #156
Conversation
This reverts commit 0b8d826.
|
WalkthroughThe changes involve updates to the CI workflow configuration and modifications to the Changes
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
apps/registry/app/components/Button/Button.jsOops! Something went wrong! :( ESLint: 8.57.1 TypeError: Failed to load plugin 'turbo' declared in 'apps/registry/.eslintrc.js » @repo/eslint-config-custom » eslint-config-turbo': Cannot convert undefined or null to object Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@levino I just reverted the commit where I disabled the tests and it works. If you approve this I can merge it back in. I disabled it, because one night it was just catastrophically failing, and I spent 6 hours before rage commenting it out. (old commits, latest of master, or branches) It must of been a problem with the github CI |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
apps/registry/app/components/Button/Button.js (1)
113-117: Consider migrating to TypeScript for better type safety.The defaultProps implementation is correct, but the component could benefit from TypeScript's type system to catch prop-related issues earlier in development.
Example TypeScript implementation:
interface ButtonProps { size?: 'small' | 'medium' | 'large'; color?: 'default' | 'success' | 'danger'; href?: string | null; children: React.ReactNode; disabled?: boolean; } const Button: React.FC<ButtonProps> = ({ size = 'medium', color = 'default', href = null, children, disabled, ...props }) => { // ... existing implementation };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
.github/workflows/ci.yml(2 hunks)apps/registry/app/components/Button/Button.js(2 hunks)
🔇 Additional comments (2)
apps/registry/app/components/Button/Button.js (2)
Line range hint 83-117: Verify that this change resolves the CI test failures.
Since this PR aims to fix CI tests, let's ensure this component change addresses specific test issues.
#!/bin/bash
# Description: Check for recent test failures related to Button default props
# Look for CI logs or test reports
fd -e log -e xml -e json . .github/
# Search for Button-related test failures in recent commits
git log -p --grep="test.*fail.*button" --grep="button.*test.*fail" -i83-83: LGTM! Moving defaults to defaultProps improves maintainability.
The change follows React's best practices by centralizing default values in defaultProps instead of parameter destructuring.
Let's verify the component's test coverage:
| - name: Run Playwright tests | ||
| run: pnpm turbo test:e2e --concurrency 1000 # The high concurrency is due to a bug: https://github.com/vercel/turbo/issues/4291 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add timeout and consider concurrency settings
The Playwright test step should have a timeout, and the high concurrency setting might cause stability issues.
Add timeout and adjust concurrency:
- name: Run Playwright tests
- run: pnpm turbo test:e2e --concurrency 1000 # The high concurrency is due to a bug: https://github.com/vercel/turbo/issues/4291
+ run: pnpm turbo test:e2e --concurrency 100 # Lower concurrency for stability
+ timeout-minutes: 30📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Run Playwright tests | |
| run: pnpm turbo test:e2e --concurrency 1000 # The high concurrency is due to a bug: https://github.com/vercel/turbo/issues/4291 | |
| - name: Run Playwright tests | |
| run: pnpm turbo test:e2e --concurrency 100 # Lower concurrency for stability | |
| timeout-minutes: 30 |
| - uses: actions/setup-node@v3 | ||
| with: | ||
| node-version: 20 | ||
| - uses: pnpm/action-setup@v2 | ||
| with: | ||
| version: 8 | ||
| - uses: actions/setup-node@v3 | ||
| with: | ||
| node-version: 20 | ||
| cache: pnpm |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove duplicate Node.js setup
There are two identical Node.js setup steps which is redundant.
Remove the duplicate setup:
- uses: actions/checkout@v3
- - uses: actions/setup-node@v3
- with:
- node-version: 20
- uses: pnpm/action-setup@v2
with:
version: 8
- uses: actions/setup-node@v3
with:
node-version: 20
cache: pnpm📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - uses: actions/setup-node@v3 | |
| with: | |
| node-version: 20 | |
| - uses: pnpm/action-setup@v2 | |
| with: | |
| version: 8 | |
| - uses: actions/setup-node@v3 | |
| with: | |
| node-version: 20 | |
| cache: pnpm | |
| - uses: actions/setup-node@v3 | |
| with: | |
| node-version: 20 | |
| - uses: pnpm/action-setup@v2 | |
| with: | |
| version: 8 | |
| - uses: actions/setup-node@v3 | |
| with: | |
| node-version: 20 | |
| cache: pnpm |
| dev-script: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v3 | ||
| with: | ||
| fetch-depth: 2 | ||
| - uses: pnpm/action-setup@v2 | ||
| with: | ||
| version: 8 | ||
| - uses: actions/setup-node@v3 | ||
| with: | ||
| node-version: 20 | ||
| cache: 'pnpm' | ||
| - run: pnpm install | ||
| - run: pnpm start-server-and-test 'turbo dev --filter=registry' 3000 'exit 0;' | ||
| working-directory: apps/registry |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Review the dev-script job implementation
The dev-script job has several potential issues:
- Using
exit 0forces a success status without actual validation - The server startup validation could be more robust
- The hardcoded working directory might make the workflow less maintainable
Consider this improved implementation:
dev-script:
runs-on: ubuntu-latest
+ defaults:
+ run:
+ working-directory: apps/registry
steps:
- uses: actions/checkout@v3
with:
fetch-depth: 2
- uses: pnpm/action-setup@v2
with:
version: 8
- uses: actions/setup-node@v3
with:
node-version: 20
cache: 'pnpm'
+ cache-dependency-path: '**/pnpm-lock.yaml'
- run: pnpm install
- - run: pnpm start-server-and-test 'turbo dev --filter=registry' 3000 'exit 0;'
- working-directory: apps/registry
+ - run: pnpm start-server-and-test 'turbo dev --filter=registry' 3000 'curl -f http://localhost:3000'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| dev-script: | |
| runs-on: ubuntu-latest | |
| steps: | |
| - uses: actions/checkout@v3 | |
| with: | |
| fetch-depth: 2 | |
| - uses: pnpm/action-setup@v2 | |
| with: | |
| version: 8 | |
| - uses: actions/setup-node@v3 | |
| with: | |
| node-version: 20 | |
| cache: 'pnpm' | |
| - run: pnpm install | |
| - run: pnpm start-server-and-test 'turbo dev --filter=registry' 3000 'exit 0;' | |
| working-directory: apps/registry | |
| dev-script: | |
| runs-on: ubuntu-latest | |
| defaults: | |
| run: | |
| working-directory: apps/registry | |
| steps: | |
| - uses: actions/checkout@v3 | |
| with: | |
| fetch-depth: 2 | |
| - uses: pnpm/action-setup@v2 | |
| with: | |
| version: 8 | |
| - uses: actions/setup-node@v3 | |
| with: | |
| node-version: 20 | |
| cache: 'pnpm' | |
| cache-dependency-path: '**/pnpm-lock.yaml' | |
| - run: pnpm install | |
| - run: pnpm start-server-and-test 'turbo dev --filter=registry' 3000 'curl -f http://localhost:3000' |
| - uses: actions/upload-artifact@v3 | ||
| if: always() | ||
| with: | ||
| name: playwright-report | ||
| path: playwright-report/ | ||
| retention-days: 30 |
There was a problem hiding this comment.
💡 Codebase verification
Artifact path configuration mismatch
The artifact upload path playwright-report/ does not match the actual test output location apps/registry/playwright-report/.
- Update the
pathtoapps/registry/playwright-report/
🔗 Analysis chain
Verify the artifact path configuration
The artifact upload step uses a hardcoded path that might not match the actual test output location.
Let's verify the Playwright report path:
Consider making the path more specific:
- uses: actions/upload-artifact@v3
if: always()
with:
name: playwright-report
- path: playwright-report/
+ path: apps/registry/playwright-report/
retention-days: 30🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for Playwright configuration files to verify the correct report path
rg -l "reporter.*html" . --type yaml --type javascript
Length of output: 91
Script:
#!/bin/bash
# Check for Playwright configuration files to verify the correct report path
rg -l "reporter.*html" . --type yaml --type js
Length of output: 85
Summary by CodeRabbit
New Features
dev-scriptjob in the CI workflow for improved development server testing.Bug Fixes
Buttoncomponent by centralizing default prop definitions for better clarity and maintainability.Chores