Skip to content

House keeping (05-04-2023) #131

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.

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

Merged
merged 8 commits into from
Apr 17, 2023

Conversation

orama254
Copy link
Member

@orama254 orama254 commented Apr 5, 2023

Changes proposed

Fixes the remaining parts of #118 plus also adds additional information to the contribution guidelines.
Also added some updated workflow for end to end playwright tests

@vercel
Copy link

vercel bot commented Apr 5, 2023

@orama254 is attempting to deploy a commit to a Personal Account owned by @reactdeveloperske on Vercel.

@reactdeveloperske first needs to authorize it.

@vercel
Copy link

vercel bot commented Apr 5, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
reactdevske-website ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 14, 2023 8:05pm

Copy link
Member

@antosan antosan left a comment

Choose a reason for hiding this comment

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

@orama254 The tests seem to be failing because they do not have access to the environment variables. I usually prefer creating a .env file in CI similar to how we have it locally. Here is how I would suggest the e2e.yml workflow to be written instead to include the additional step to Prepare .env file (the top-level env: key-value pair can be removed):

name: End-to-end Tests
on:
  push:
    branches: [main, develop]
  pull_request:
    branches: [main, develop]
jobs:
  e2e:
    timeout-minutes: 60
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v3
      - name: Prepare .env file
        run: |
          rm -f .env && touch .env
          echo "NEXT_PUBLIC_FORMSPREE_ID=${{ secrets.NEXT_PUBLIC_FORMSPREE_ID }}" >> .env
          echo "NEXT_PUBLIC_RECAPTCHA_SITE_KEY=${{ secrets.NEXT_PUBLIC_RECAPTCHA_SITE_KEY }}" >> .env
      - uses: actions/setup-node@v3
        with:
          node-version: '18.x'
      - name: Install dependencies
        run: npm ci
      - name: Install Playwright Browsers
        run: npx playwright install --with-deps
      - name: Run Playwright tests
        run: npm run test
      - uses: actions/upload-artifact@v3
        if: always()
        with:
          name: playwright-report
          path: playwright-report/
          retention-days: 5

Another thing I have noticed is that we have the word "Events" appearing severally on the page and this will cause one of the tests to be ambiguous and fail. This test in home.spec.ts should be adjusted to be an exact match by adding exact: true:

test('should show Events section', async ({ page }) => {
  await expect(
    page.getByRole('heading', { name: 'Events', exact: true })
  ).toBeVisible();
});

Copy link
Member

@antosan antosan left a comment

Choose a reason for hiding this comment

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

@orama254 The trace file still shows this error You must provide a form key or hashid (e.g. useForm("myForm") or useForm("123xyz") which means that the value of secrets.NEXT_PUBLIC_FORMSPREE_ID and secrets.NEXT_PUBLIC_RECAPTCHA_SITE_KEY are undefined. Since the values do not necessarily need to be the exact keys, can you try using fake values hardcoded in the workflow file instead, like this:

      - name: Prepare .env file
        run: |
          rm -f .env && touch .env
          echo "NEXT_PUBLIC_FORMSPREE_ID=fake" >> .env
          echo "NEXT_PUBLIC_RECAPTCHA_SITE_KEY=fake" >> .env

@orama254
Copy link
Member Author

Another thing I have noticed is that we have the word "Events" appearing severally on the page and this will cause one of the tests to be ambiguous and fail. This test in home.spec.ts should be adjusted to be an exact match by adding exact: true:

test('should show Events section', async ({ page }) => {
  await expect(
    page.getByRole('heading', { name: 'Events', exact: true })
  ).toBeVisible();
});

@antosan Very nice, the tests are passing.

on the requested changes above, I'm getting an error with typescript that these are the expected parameters for that:

{ checked?: boolean | undefined; disabled?: boolean | undefined; expanded?: boolean | undefined; includeHidden?: boolean | undefined; level?: number | undefined; name?: string | RegExp | undefined; pressed?: boolean | undefined; selected?: boolean | undefined; }

exact: boolean seems to be missing

@orama254 orama254 requested a review from antosan April 14, 2023 20:14
Copy link

@kharioki kharioki left a comment

Choose a reason for hiding this comment

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

All tests are running and passing

@orama254 orama254 merged commit 90451bf into reactdeveloperske:develop Apr 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants