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.

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

[Issue #358]: Add e2e tests against PR preview env #85

Open
wants to merge 51 commits into
base: main
Choose a base branch
from

Conversation

rylew1
Copy link

@rylew1 rylew1 commented Jul 8, 2024

Ticket

Resolves #358

Changes

  • add playwright e2e tests against preview env

Context

Preview environment

@rylew1 rylew1 requested review from lorenyu and aligg July 8, 2024 16:58
@rylew1 rylew1 marked this pull request as draft July 8, 2024 16:59
Copy link
Collaborator

@lorenyu lorenyu left a comment

Choose a reason for hiding this comment

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

Amazing. So excited to have e2e tests!

.github/workflows/pr-environment-update.yml Outdated Show resolved Hide resolved
e2e/tests/index.spec.js Outdated Show resolved Hide resolved
e2e/tests/index.spec.js Outdated Show resolved Hide resolved
e2e/tests/index.spec.js Outdated Show resolved Hide resolved
e2e/playwright.config.js Show resolved Hide resolved
e2e/playwright.config.js Outdated Show resolved Hide resolved
e2e/playwright.config.js Outdated Show resolved Hide resolved
e2e/playwright.config.js Outdated Show resolved Hide resolved
e2e/package.json Outdated Show resolved Hide resolved
.github/workflows/e2e-tests.yml Show resolved Hide resolved
e2e/package.json Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
Copy link
Collaborator

@lorenyu lorenyu left a comment

Choose a reason for hiding this comment

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

in .github/workflows/ci-app-pr-environment-update.yml, rename the workflow from "CI App PR Environment Update" to "CI App PR Environment Checks" and rename the file to .github/workflows/ci-app-pr-environment-checks.yml

Comment on lines 26 to 44
- name: Set up Node.js
uses: actions/setup-node@v4
with:
node-version: '20'

- name: Install system dependencies
run: |
sudo apt-get update
sudo apt-get install -y libwoff1 libopus0 libvpx7 libevent-2.1-7 libopus0 libgstreamer1.0-0 \
libgstreamer-plugins-base1.0-0 libgstreamer-plugins-good1.0-0 libharfbuzz-icu0 libhyphen0 \
libenchant-2-2 libflite1 libgles2 libx264-dev

- name: Install Node.js dependencies
run: npm ci
working-directory: ./e2e

- name: Install Playwright browsers
run: npx playwright install --with-deps
working-directory: ./e2e
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok.. there are a lot more dependencies here than I expected. Installing linux packages, node libraries, and browsers. Given how much there is here we should containerize this and run it all in a container, like we do with unit tests. Makes me think that we should consider Dockerizing this like we do for unit tests in the Flask template. Does Playwright run ok in a Docker container?

cc @rocketnova what do you think?

That said I'd be open to doing that in a separate PR.

Copy link
Author

Choose a reason for hiding this comment

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

I would vote separate pr as this is getting large

Comment on lines +33 to +36
sudo apt-get update
sudo apt-get install -y libwoff1 libopus0 libvpx7 libevent-2.1-7 libopus0 libgstreamer1.0-0 \
libgstreamer-plugins-base1.0-0 libgstreamer-plugins-good1.0-0 libharfbuzz-icu0 libhyphen0 \
libenchant-2-2 libflite1 libgles2 libx264-dev
Copy link
Collaborator

Choose a reason for hiding this comment

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

What are these for? Does it merit a comment or link to documentation that explains the need for these packages and/or what they do?

Copy link
Author

@rylew1 rylew1 Jul 12, 2024

Choose a reason for hiding this comment

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

If it helps, you can see the initial deps failure here: https://github.com/navapbc/platform-test-nextjs/actions/runs/9885358319/job/27303354260

.github/workflows/e2e-tests.yml Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
.github/workflows/e2e-tests.yml Outdated Show resolved Hide resolved
e2e/app/playwright.config.js Show resolved Hide resolved
.github/workflows/e2e-tests.yml Outdated Show resolved Hide resolved
@rylew1 rylew1 marked this pull request as ready for review July 12, 2024 22:01
@rylew1
Copy link
Author

rylew1 commented Jul 16, 2024

Moved over to template-infra repo navapbc/template-infra#694

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.

2 participants