Skip to content
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.

Sign up for GitHub

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

fix: re-enable ci tests #156

Merged
merged 1 commit into from
Nov 12, 2024
Merged

fix: re-enable ci tests #156

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 42 additions & 42 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,22 +29,22 @@ jobs:
cache: pnpm
- run: pnpm install
- run: pnpm turbo lint prettier
# 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
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
Comment on lines +32 to +47
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Review the dev-script job implementation

The dev-script job has several potential issues:

  1. Using exit 0 forces a success status without actual validation
  2. The server startup validation could be more robust
  3. 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.

Suggested change
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'

build:
runs-on: ubuntu-latest
steps:
Expand All @@ -60,29 +60,29 @@ jobs:
cache: pnpm
- run: pnpm install
- run: pnpm turbo build
# test:
# runs-on: ubuntu-latest
# steps:
# - 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
# - run: pnpm install
# - name: Install Playwright Browsers
# run: pnpm exec playwright install --with-deps
# working-directory: apps/registry
# - 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
# - uses: actions/upload-artifact@v3
# if: always()
# with:
# name: playwright-report
# path: playwright-report/
# retention-days: 30
test:
runs-on: ubuntu-latest
steps:
- 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
Comment on lines +67 to +76
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
- 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

- run: pnpm install
- name: Install Playwright Browsers
run: pnpm exec playwright install --with-deps
working-directory: apps/registry
- 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
Comment on lines +81 to +82
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
- 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/upload-artifact@v3
if: always()
with:
name: playwright-report
path: playwright-report/
retention-days: 30
Comment on lines +83 to +88
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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 path to apps/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

15 changes: 7 additions & 8 deletions apps/registry/app/components/Button/Button.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,14 +80,7 @@ const StyledLink = styled.a`
${buttonStyles}
`;

const Button = ({
size = 'medium',
color = 'default',
href = null,
children,
disabled,
...props
}) => {
const Button = ({ size, color, href, children, disabled, ...props }) => {
if (href) {
return (
<StyledLink
Expand Down Expand Up @@ -117,4 +110,10 @@ Button.propTypes = {
children: PropTypes.node.isRequired,
};

Button.defaultProps = {
size: 'medium',
color: 'default',
href: null,
};

export default Button;
Loading