-
Notifications
You must be signed in to change notification settings - Fork 462
E2E infra #4875
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
base: develop
Are you sure you want to change the base?
E2E infra #4875
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR establishes comprehensive E2E testing infrastructure for both local development and CI environments. It introduces Docker Compose-based testing setup, improves backend configuration flexibility, and updates GitHub Actions to leverage the new infrastructure.
- Enhanced
run.sh
script with configurable database files and improved error handling - Added Docker Compose services for E2E testing (backend-e2e, frontend-e2e, playwright-e2e)
- Updated GitHub Actions workflow to use Docker Compose for E2E tests
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
application/ui/tests/e2e/new-project.spec.ts | Enhanced test selectors and re-enabled project creation test |
application/ui/tests/e2e/existing-project.spec.ts | Temporarily disabled existing project test |
application/ui/tests/e2e/README.md | Comprehensive E2E testing documentation |
application/ui/playwright.config.ts | Updated configuration for Docker and local E2E testing |
application/ui/package.json | Added E2E-specific build and Docker test scripts |
application/docker/nginx-e2e.conf | Nginx configuration for E2E frontend service |
application/docker/docker-compose.yaml | Added E2E services with health checks and watch support |
application/docker/Dockerfile | Added run.sh script to Docker image |
application/backend/run.sh | Enhanced with database file configuration and improved error handling |
application/backend/readme.md | Added comprehensive backend documentation |
application/.dockerignore | Included run.sh in Docker build |
.github/workflows/ui-lint-and-test.yaml | Updated to use Docker Compose for E2E tests |
Comments suppressed due to low confidence (2)
application/ui/playwright.config.ts:1
- The Playwright image version v1.54.0 appears to be a future version that may not exist yet. Current knowledge cutoff is January 2025, and the latest known stable version is around v1.48.x. Please verify this version exists or use a known stable version.
// Copyright (C) 2025 Intel Corporation
.github/workflows/ui-lint-and-test.yaml:310
- The Playwright image version v1.54.0 appears to be a future version that may not exist yet. Current knowledge cutoff is January 2025, and the latest known stable version is around v1.48.x. Please verify this version exists or use a known stable version.
image: mcr.microsoft.com/playwright:v1.54.0-noble@sha256:18d6adb6aaccf1b0f30eba890069972e089138e4a59ddb5303d7e7290e4e38b6
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
📊 Test coverage report
|
- name: Build UI | ||
working-directory: "application/ui" | ||
env: | ||
PUBLIC_API_BASE_URL: "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to avoid setting http://localhost:7680/api/...
to our baseUrl, so the UI use relative urls
env: | ||
E2E_ASSETS_S3_URL: ${{ vars.E2E_ASSETS_S3_URL }} | ||
run: | | ||
docker compose --profile e2e up \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the important change is this one.
|
||
# Backend files | ||
!backend/app | ||
!backend/run.sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because we need to copy this bad boy now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, we shouldn't copy it. Can you motivate the need for the copy?
ports: | ||
- "8081:80" | ||
|
||
backend-e2e: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if this is overkill
@@ -0,0 +1,46 @@ | |||
server { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if we need a second nginx but the flow goes like this:
- build:e2e builds UI with PUBLIC_API_BASE_URL=""
- Docker Compose starts
- backend-e2e: Runs run.sh, seeds DB, starts FastAPI on port 7860
frontend-e2e: Nginx serves dist/, proxies /api/* to backend
playwright-e2e: Runs tests against http://frontend-e2e:3000
Tests make API calls: http://frontend-e2e:3000/api/projects → nginx proxies → http://backend-e2e:7860/api/projects
}, | ||
"scripts": { | ||
"build": "rsbuild build", | ||
"build:e2e": "PUBLIC_API_BASE_URL='' rsbuild build", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
running the build here and not on docker-compose avoids having to install aaaaaaaalllll node dependencies to just build the UI
import { expect, test } from '@playwright/test'; | ||
|
||
test('[E2E] Existing project', async ({ page }) => { | ||
test.skip('[E2E] Existing project', async ({ page }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
skipping for now since the test step with the STREAM is not working.
uv sync --frozen --no-editable | ||
|
||
COPY backend/app ./app | ||
COPY backend/run.sh ./run.sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
run.sh
should not be copied in the Docker image, it is not meant for production.
|
||
# Backend files | ||
!backend/app | ||
!backend/run.sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, we shouldn't copy it. Can you motivate the need for the copy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the current pace of development, I'm afraid this readme.md
will be largely obsolete in a couple of weeks. I intentionally kept it short so that we don't have to constantly update it.
- name: Checkout code | ||
uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5.0.0 | ||
with: | ||
persist-credentials: false | ||
|
||
- name: Download UI build | ||
uses: actions/download-artifact@634f93cb2916e3fdff6788551b99b062d0335ce0 # v5.0.0 | ||
with: | ||
name: ui-dist | ||
path: application/ui | ||
|
||
- name: Unpack build | ||
working-directory: application/ui | ||
run: tar -xzf dist.tar.gz | ||
|
||
- name: Install dependencies | ||
working-directory: application/ui | ||
run: npm ci |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All this is unnecessary imho. We already have a Dockerfile
that includes both the server and the UI, and a GHA workflow build.yaml
to build it. I would expect the e2e workflow to use that image to spin up a container and run the tests on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree
action: rebuild | ||
|
||
frontend-e2e: | ||
profiles: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to understand the architecture choice here. we're using a separate frontend-e2e
container with nginx to serve the UI, but in production we have a single geti-tune
container that includes both the backend and frontend (nginx + FastAPI).
A couple of questions:
Since e2e tests are meant to validate our production setup, wouldn't it be more valuable to test against the actual production container that includes both services? This would catch any integration issues between nginx and FastAPI in the same container.
The current approach requires 3 containers + health checks + inter-service networking. Have you considered using the existing production image from the build.yaml
workflow instead and running playwright tests directly in the runner's container?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally agree, we should leverage the existing geti-tune
. The issues i had were related with the networking inside docker containers. We want to test webrtc streams and it wasnt working.
|
||
```bash | ||
# Full E2E setup with database seeding and test file downloads | ||
DATABASE_FILE=geti_tune_e2e.db SEED_DB=true DOWNLOAD_FILES=true ./run.sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wondering why a db_file name should be different for e2e tests setup?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The goal was to also be able to run these locally, and since some tests require seeding, we need a temporary DB that we can setup/destroy after every run
Summary
run.sh
DATABASE_FILE
as a param allowing us a to have different DB's, one for e2e tests and other for developmentdocker-compose
to include E2E stepsHow to test
Checklist