AP-638: Implements e2e tests using Playwright by danschmidt5189 · Pull Request #34 · BerkeleyLibrary/mokelumne

danschmidt5189 · 2026-04-14T22:25:19Z

Implements end-to-end browser-based testing using Playwright w/Chromium. The initial tests cover the OIDC login flow, ensuring that:

Anonymous users are forced to login.
Admin/Basic users can view the homepage after logging in, but unauthz'd users are forbidden.
Only Admin users can view an admin page (/security/users).

Playwright runs in a new playwright service in the Compose File, executed over-the-wire by pytest running from AirFlow. This introduces some complications around reconciling AirFlow/Keycloak URLs when accessing as a developer (on localhost) vs. as Playwright in the stack. See Slack for a rundown on the approaches I considered. I ultimately settled on extending the Playwright image to include socat, and using that for forward localhost:8080 -> airflow and localhost:8180 -> keycloak within the playwright service.

Copilot

Pull request overview

Adds a Playwright-based end-to-end (E2E) auth test suite for the Airflow UI (via Keycloak OIDC), plus Docker/CI wiring to run the browser remotely in a sidecar container.

Changes:

Introduce test/e2e/ Playwright tests + helpers for the Airflow→Keycloak login flow.
Add a Playwright sidecar service (Dockerfile + entrypoint) and enable it under a Compose test profile.
Update pytest configuration/dependencies and CI to start the stack with the test profile and retain Playwright artifacts on failures.

Reviewed changes

Copilot reviewed 9 out of 11 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
`test/e2e/test_auth.py`	New E2E tests asserting login is required and role-based access to homepage/admin pages.
`test/e2e/helpers.py`	Helper to drive the Airflow→Keycloak OIDC login flow in Playwright.
`test/e2e/conftest.py`	Pytest fixtures to connect to a remote Playwright server and set `base_url`.
`test/e2e/__init__.py`	Package marker for the E2E tests.
`requirements.txt`	Adds Playwright + pytest-playwright dependency pins/hashes.
`pyproject.toml`	Adds pytest-playwright to test extras; configures pytest Playwright artifact options and marker.
`playwright/entrypoint.sh`	Starts TCP forwarders (socat) for localhost port parity inside the sidecar container.
`playwright/Dockerfile`	Builds the Playwright sidecar image (extends MS Playwright image + installs socat).
`example.env`	Documents E2E env vars for running tests via the Playwright sidecar.
`docker-compose.yml`	Adds a `playwright` service under the `test` profile to run the remote Playwright server.
`.github/workflows/build.yml`	Starts Compose with `--profile test ... --wait` so the sidecar is available in CI.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-14T23:42:41Z

+    page.locator("#username").fill(username)
+    page.locator("#password").fill(password)
+    page.locator("#kc-login").click()
+    page.wait_for_url(lambda url: not KEYCLOAK_AUTH_URL_RE.search(url))


page.wait_for_url(lambda url: not KEYCLOAK_AUTH_URL_RE.search(url)) can return as soon as navigation leaves /protocol/openid-connect/auth, even if the browser is still on a different Keycloak URL (e.g., login-actions endpoints), which can make the helper return before the post-login redirect back to Airflow completes. Consider waiting until the URL is no longer on the Keycloak base host/port (or matches the Airflow base URL), and/or broadening the Keycloak regex to cover all realm paths so the wait cannot finish while still in Keycloak.

Copilot · 2026-04-14T23:42:42Z

+        expect(page.locator(ADMIN_USERS_TABLE_HEADER)).to_be_visible()
+    else:
+        expect(page.locator(ADMIN_USERS_TABLE_HEADER)).not_to_be_visible()


In the denied case, this test only asserts that the admin users table header is not visible. That can pass for many unintended outcomes (redirect to an error page, a blank page, a 500, etc.). To make the authorization check meaningful and reduce false positives/flakiness, assert the expected denied behavior as well (e.g., URL is the login chooser, or an explicit 403/"Forbidden" marker is shown).

Copilot · 2026-04-14T23:42:42Z

+  # wire protocol is not stable across versions.
+  playwright:
+    build: ./playwright
+    command: ["npx", "-y", "playwright@1.58.0", "run-server", "--port", "3000", "--host", "0.0.0.0", "--path", "/ws"]


The Playwright service hard-codes playwright@1.58.0 in the npx command, duplicating the version already pinned via ARG PLAYWRIGHT_VERSION in playwright/Dockerfile. This can drift and also tends to force an npx download on container start. Prefer invoking the Playwright CLI already present in the base image (e.g., playwright run-server ...) and/or eliminate the duplicated version so a single source of truth controls the server/client protocol version.

Suggested change

command: ["npx", "-y", "playwright@1.58.0", "run-server", "--port", "3000", "--host", "0.0.0.0", "--path", "/ws"]

command: ["playwright", "run-server", "--port", "3000", "--host", "0.0.0.0", "--path", "/ws"]

danschmidt5189 · 2026-04-14T23:45:29Z

      - name: Start the stack
        run: |
-          docker compose up --detach
+          docker compose --profile test up --detach --wait


Playwright is pretty lightweight, so I'm not sure we need to gate this behind a profile.

awilfox

Some discussion items, but overall a good direction.

awilfox · 2026-04-15T16:57:14Z

+    page.locator("#username").fill(username)
+    page.locator("#password").fill(password)
+    page.locator("#kc-login").click()
+    page.wait_for_url(lambda url: not KEYCLOAK_AUTH_URL_RE.search(url))


I'm in agreement with Copilot and feel like this is an excellent use of KEYCLOAK_BASE_URL.

Suggested change

page.wait_for_url(lambda url: not KEYCLOAK_AUTH_URL_RE.search(url))

page.wait_for_url(lambda url: not url.startswith(KEYCLOAK_BASE_URL))

awilfox · 2026-04-15T16:57:45Z

@@ -0,0 +1,23 @@
+import re


Missing module docstring.

awilfox · 2026-04-15T16:59:47Z

+KEYCLOAK_AUTH_URL_RE = re.compile(r"/realms/berkeley-local/protocol/openid-connect/auth")
+
+
+def login(page: Page, username: str, password: str) -> None:


I feel like it would be good to have a decorator for admin and user. Perhaps public too, though that shouldn't be generally useful.

This way, tests could have @as_admin or @as_user later on. Otherwise, there will be a lot of code duplication, and hardcoding the test credentials which we may not want to do.

awilfox · 2026-04-15T17:00:07Z

+    if allowed:
+        expect(page.locator(ADMIN_USERS_TABLE_HEADER)).to_be_visible()
+    else:
+        expect(page.locator(ADMIN_USERS_TABLE_HEADER)).not_to_be_visible()


Agree with Copilot, this should be explicitly testing for a 403, not just the lack of presence of a table.

Unfortunately AirFlow makes this much, much harder than it needs to be. A public user is actually redirected to the login page; a regular non-admin user gets an HTTP 200 with a page that says "404"… I'll see what I can do to clean it up but so long as we're actually browsing like a human user the assertion will be messy.

danschmidt5189 · 2026-04-15T21:58:07Z

@jason-raitz @awilfox Re-requesting review after the following major changes:

I reverted from table-based access tests (parametrized) to simple tests per user. The table-based tests are seriously complicated by the interaction between the user's status (admin, regular, public) and how AirFlow renders the "auth error" (it doesn't—you get 200, 404, or 3xx depending on the situation). I think it's easier to read this way and it's certainly easier to write.
Merged helpers into conftest.py. There wasn't really a good reason to separate them IMO.
Added helpers for matching AirFlow and KeyCloak URLs.
Removed the --profile=test flag from playwright. As I mentioned in a comment it idles ~200MB and peaks ~500MB when running tests. This simplifies startup (you don't have to remember to allow it, which bit me a few times).

Copilot

Pull request overview

Copilot reviewed 7 out of 9 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-15T21:58:30Z

+#PYTEST_PLAYWRIGHT_WS_ENDPOINT=
+# Set these to run against non-localhost airflow or keycloak instances.
+#PYTEST_AIRFLOW_BASE_URL=
+#PYTEST_KEYCLOAK_BASE_URL


#PYTEST_KEYCLOAK_BASE_URL is missing the trailing = (unlike the other commented env vars). As-is, it can’t be uncommented/edited consistently and may confuse copy/paste into a real .env.

Suggested change

#PYTEST_KEYCLOAK_BASE_URL

#PYTEST_KEYCLOAK_BASE_URL=

Copilot · 2026-04-15T21:58:30Z

+
+import pytest
+
+from .conftest import airflow_login_url, airflow_url, login


login is imported from .conftest but never used in this test module. This will trip unused-import checks (e.g., pylint) and adds noise; remove the unused import or use it directly in a test.

Suggested change

from .conftest import airflow_login_url, airflow_url, login

from .conftest import airflow_login_url, airflow_url

awilfox

Style issues, but structurally sound.

awilfox · 2026-04-15T22:54:53Z

+
+
+def login(page: Page, username: str, password: str) -> None:
+    """Walk a fresh page through the Airflow -> Keycloak OIDC login flow.


Suggested change

"""Walk a fresh page through the Airflow -> Keycloak OIDC login flow.

"""Walk a fresh instance through the Airflow -> Keycloak OIDC login flow.

?

Suggested change

"""Walk a fresh page through the Airflow -> Keycloak OIDC login flow.

"""Walk a fresh browser through the Airflow -> Keycloak OIDC login flow.

?

"page" doesn't seem right here.

In Playwright terms it's a Page, meaning a fresh tab in a per-test isolated browser. That last part is not obvious (I originally thought pages shared a Browser context); see https://playwright.dev/python/docs/browser-contexts.

awilfox · 2026-04-15T22:59:30Z

+    expect(page.locator("#btn-signin-keycloak")).to_be_visible()
+
+
+def test_public_user_cannot_view_homepage(as_public_user) -> None:


I was thinking of a decorator when I made that example name. With the literate RSpec-like expect, something like public_browser makes more sense to me.

Suggested change

def test_public_user_cannot_view_homepage(as_public_user) -> None:

def test_public_user_cannot_view_homepage(public_browser) -> None:

I considered this, but Browser has a specific meaning and this is really a Page object. Agree that "as_{user}" doesn't feel perfect, but it does encapsulate the credentials used which is my only real goal for now.

I'm accustomed to using fixtures to inject behavior into pytests — what did you have in mind with a raw decorator?

anarchivist

rw+c; mine are minor, @awilfox's probably merit more discussion.

- Implements end-to-end (e2e) browser-based tests using Playwright. - Resolves issues running the stack as a human developer vs. tests from the container by using `socat` in the playwright container to forward localhost:<port> to the respective service. - Adds an initial set of tests for the OIDC login flow (admin, user, public, non-auth'd).

danschmidt5189 force-pushed the AP-638 branch from 9cdf262 to 760e5d4 Compare April 14, 2026 23:37

danschmidt5189 requested a review from Copilot April 14, 2026 23:38

Copilot started reviewing on behalf of danschmidt5189 April 14, 2026 23:38 View session

Copilot AI reviewed Apr 14, 2026

View reviewed changes

danschmidt5189 commented Apr 14, 2026

View reviewed changes

Comment thread example.env Outdated

danschmidt5189 marked this pull request as ready for review April 14, 2026 23:50

awilfox reviewed Apr 15, 2026

View reviewed changes

danschmidt5189 force-pushed the AP-638 branch from 34ebbe6 to 45f571d Compare April 15, 2026 21:39

danschmidt5189 requested review from awilfox, Copilot and jason-raitz April 15, 2026 21:54

Copilot started reviewing on behalf of danschmidt5189 April 15, 2026 21:55 View session

Copilot AI reviewed Apr 15, 2026

View reviewed changes

danschmidt5189 force-pushed the AP-638 branch from 3635647 to ac4a434 Compare April 15, 2026 22:14

danschmidt5189 changed the title ~~Ap 638~~ AP-638: Implements e2e tests using Playwright Apr 15, 2026

awilfox reviewed Apr 15, 2026

View reviewed changes

anarchivist approved these changes Apr 15, 2026

View reviewed changes

Comment thread test/e2e/conftest.py Outdated

Comment thread test/e2e/test_auth.py

Comment thread pyproject.toml Outdated

danschmidt5189 force-pushed the AP-638 branch from 96d76dd to 7fe96bf Compare April 16, 2026 00:43

	command: ["npx", "-y", "playwright@1.58.0", "run-server", "--port", "3000", "--host", "0.0.0.0", "--path", "/ws"]
	command: ["playwright", "run-server", "--port", "3000", "--host", "0.0.0.0", "--path", "/ws"]

	page.wait_for_url(lambda url: not KEYCLOAK_AUTH_URL_RE.search(url))
	page.wait_for_url(lambda url: not url.startswith(KEYCLOAK_BASE_URL))

		KEYCLOAK_AUTH_URL_RE = re.compile(r"/realms/berkeley-local/protocol/openid-connect/auth")


		def login(page: Page, username: str, password: str) -> None:


		import pytest

		from .conftest import airflow_login_url, airflow_url, login



		def login(page: Page, username: str, password: str) -> None:
		"""Walk a fresh page through the Airflow -> Keycloak OIDC login flow.

	"""Walk a fresh page through the Airflow -> Keycloak OIDC login flow.
	"""Walk a fresh instance through the Airflow -> Keycloak OIDC login flow.

	"""Walk a fresh page through the Airflow -> Keycloak OIDC login flow.
	"""Walk a fresh browser through the Airflow -> Keycloak OIDC login flow.

		expect(page.locator("#btn-signin-keycloak")).to_be_visible()


		def test_public_user_cannot_view_homepage(as_public_user) -> None:

Conversation

danschmidt5189 commented Apr 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

awilfox left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

danschmidt5189 commented Apr 15, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

awilfox left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anarchivist left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

danschmidt5189 commented Apr 14, 2026 •

edited

Loading