mrc-4333 e2e tests #179

EmmaLRussell · 2025-02-17T14:09:25Z

This branch adds some basic e2e tests, and the framework for running the tests, both locally and against any Packit server. Please see the changes to the top level README for details on how this works.

The index page, packet group page and parts of the packet page are tested.

Some other changes to note:

./scripts/run-e2e-tests-on-server will run generic e2e tests on the specified server with specified auth method (basic or github). If github auth is required, it calls out to a python module, e2e_github_auth.py, which in turn uses pyorderly to manage the github auth, and obtain a packit token. We then need to return this token to the bash script so it can set it as an environment variable which the playwright tests will use. Python scripts cannot set environment variables themselves (outside of the scope of their own copy of the environment) and also cannot return values to the calling script (other than return codes or values written to stdout, on which there is a lot of other chat during the auth process). So here the token is kludgily passed back to the calling script via writing out to a file.
app/e2e/auth.setup.ts does authentication according to the required method and credentials provided through environment variables. , or using default basic superuser and password. It is configured as a dependency of the main test projects in playwright.config.ts.

This branch (which includes a couple of markup changes used by the tests) is deployed to packit-dev using the packit-infra PR here. You should be able to run the e2e tests against packit-dev by running:

hatch shell 
pip install pyorderly
./scripts/run-e2e-tests-on-server https://packit-dev.dide.ic.ac.uk/reside-dev/ github

There are separate tickets under the epic for testing:

managing roles
running packets
downloading files and zips
other auth paths e.g enter wrong password

We will also need to ensure that any future tests which change the state of the server e.g. running packets or changing user roles are not run on prod servers, but only on localhost or dev. (The tests in this branch are all read-only.).

Codecov is grumbling but there isn't any new untested code.

codecov · 2025-02-17T14:13:34Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.16%. Comparing base (2383016) to head (0eb3b65).
Report is 88 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #179      +/-   ##
==========================================
- Coverage   97.29%   97.16%   -0.14%     
==========================================
  Files         150      150              
  Lines        1477     1479       +2     
  Branches      426      428       +2     
==========================================
  Hits         1437     1437              
- Misses         39       41       +2     
  Partials        1        1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

EmmaLRussell · 2025-03-10T09:41:51Z

Updates following last review

The major changes were abandoning the run-e2e-tests-on-server script and associated python file. Instead, all authentication happens in auth.setup.ts which now:

Makes an auth config call to the server to figure out whether basic or github auth is required
If basic, requires PACKIT_E2E_BASiC_USER and PACKIT_E2E_BASIC_PASSWORD env vars to be set, and logs in with those
If github, checks if GITHUB_ACCESS_TOKEN env var is set. If not, does device flow to get a github token using @octokit/auth-oauth-device. Uses the github token to login to packit.

The other main change was to refactor the division of "local" and generic tests, which were previously matching on file name. Now there are tags for @demoPackets (tests which use the local demo dataset) and @stateMutate (for tests which will change the state of the server by running packets or changing permissions. We don't have any of these yet, but I've added a placeholder test which has this tag). We can include or exclude these tags when running playwright - test runs on dev or prod will probably fail if @demoPackets is included. @stateMutate should never be included when running against prod. I've added a fixture, tagCheckFixture which all tests should use. This checks if we're running tests with unexpected tags given the baseURL, and prints warning messages or throws an error (in the case of @stateMutate tag on non-local or non-dev).

There are now three npm scripts in packit json:

test:e2e which allows playwright to use default localhost server, and sets basic username and password
test:e2e-dev which runs against https://packit-dev.dide.ic.ac.uk/reside-dev/ and excludes the two tags
test:e2e-prod which runs against https://packit.dide.ic.ac.uk/reside/ and excludes the two tags

I was going to include functionality to allow user to enter basic username and password interactively if not provided via environment variables, but stdin doesn't seem to be available in the auth.setup script (although I could get it to read input if I made a universal global setup). I presume something in Playwright is tampering with stdin. If it's useful, I can add that, in global setup, or in a separate node script but it's probably not worth it when we can just ask the user to set the variables!

Various other changes from the review comments. Some detailed in replies, and also:

Replaced Firefox with Safari, since people insist on using it..
renamed workflow file and some of the job names
upload test-results, should delete auth.json (but not a big deal if it doesn't)

Testing

The dev and prod scripts script are currently failing because the deployed main branch doesn't include the breadcrumb test id.

To test running non-interactively with github login, run the test:e2e-dev script with env var GITHUB_ACCESS_TOKEN set to your PAT - the tests should run successfully without needing to authenticate interactively: export GITHUB_ACCESS_TOKEN=[your PAT] && npm run test:e2e-dev

You can test triggering the tagCheckFixture by running without tag exclusion:
export PACKIT_E2E_BASE_URL=https://packit-dev.dide.ic.ac.uk/reside-dev/ && npx playwright test or export PACKIT_E2E_BASE_URL=https://packit.dide.ic.ac.uk/reside/ && npx playwright test

david-mears-2

I haven't re-read all of the test files themselves, only the surrounding infrastructure things. My main comment is to suggest a further bit of defence-in-depth to make triply sure we won't be mutating state on prod.

david-mears-2 · 2025-03-12T12:06:21Z

app/e2e/auth.setup.ts

@@ -0,0 +1,93 @@
+import { test as setup, expect, Page } from "@playwright/test";


Should this import test and expect from tagCheckFixture instead?

I am uncomfortable relying on developers to remember to check that they are importing 'test' from the correct place. Especially since IDEs often write imports for you. We could do something like the following, in a global setup script akin to auth.setup.js that is listed as a dependency for all test projects in playwright config:

function checkImports(filePath) { const content = fs.readFileSync(filePath, 'utf-8'); const importRegex = /import\s+{[^}]*\btest\b[^}]*}\s+from\s+".\/tagCheckFixture";/; if (!importRegex.test(content)) { throw new Error(`Test file ${filePath} does not import 'test' from 'tagCheckFixture'.`); } }

Don't think it really matters for the auth setup test but yeah, for the purposes of setting a good example let's do that!

checkImports is a great idea. Actually, I'd be inclined to do that in auth.setup itself (possibly with a rename to reflect more generic concerns) as further defence, since people could still forget to include checkImports.setup.ts as a dependency, but their tests aren't going to get anywhere if they don't include the dep that does auth...

README.md

app/e2e/packetPage.demo.spec.ts

app/playwright.config.ts

Co-authored-by: David Mears <[email protected]>

EmmaLRussell · 2025-03-17T14:58:18Z

I've implemented the suggested checkImports method in a globalSetup script rather than within the auth setup (it runs first before any other project). It uses a generic glob search for test files under the test root dir, then loads all the file contents and checks for the suggested regex match of loading test from tagCheckFixture. Test by changing one of the test files to import test from @playwright/test

The fs glob search is new and experimental in node 22 - I've updated the node version in CI.

I've also updated the dev and prod urls now they're on proper subdomains, and also made sure the BASIC_AUTH variable is set in the dev-start script (it caused an error if running in github mode).

plietar

LGTM, just some minor bits

plietar · 2025-03-17T17:19:41Z

app/e2e/packetGroupPage.demo.spec.ts

+    await expect(link).toHaveText(packetId);
+    const expectedHref = getInstanceRelativePath(baseURL, `parameters/${packetId}`);
+    await expect(await link.getAttribute("href")).toBe(expectedHref);
+    // We expect servers we're testing on to have either GB or US locale.


Suggested change

// We expect servers we're testing on to have either GB or US locale.

// We expect browsers we're testing on to have either GB or US locale.

No? I'd be surprised if we did any date rendering server side. The JS should pick up the browser defaults.

Actually you can set the locale in the playwright config, that should make this more reliable..

plietar · 2025-03-17T17:29:06Z

app/e2e/utils.ts

+};
+
+// Get a relative path which includes the packit instance, if any
+export const getInstanceRelativePath = (baseURL: string, path: string) => {


So as of last Friday we're now running all instances at the root path, meaning we could actually remove this function.

Might just leave it in for now just in case we run any old style multi-tenancies again!

README.md

plietar · 2025-03-17T17:31:28Z

app/e2e/tagCheckFixture.ts

+export const TAG_DEMO_PACKETS = "@demoPackets";
+
+const isLocal = (baseURL: string) => baseURL.startsWith("http://localhost");
+const isDev = (baseURL: string) => baseURL.startsWith("https://packit-dev.dide.ic.ac.uk");


I think this is broken now that the URL has changed. You would need to parse the URL, extract the domain name and check if that part ends in packit-dev.dide.ic.ac.uk

Something like

Suggested change

const isDev = (baseURL: string) => baseURL.startsWith("https://packit-dev.dide.ic.ac.uk");

const isDev = (baseURL: string) => new URL(baseURL).hostname.endsWith("https://packit-dev.dide.ic.ac.uk");

Thanks, have added that.

Co-authored-by: Paul Liétar <[email protected]>

…3-e2e

…ult e2e test script

david-mears-2

Thank you!

Perhaps the version of "@types/node" (package.json) wants to be kept in sync with the node version - it's still on 16.

david-mears-2

Sorry, just noticed app/Dockerfile has node 18 still as well. Also line 6 of app/README.md cites Node 18.

EmmaLRussell · 2025-03-18T14:18:55Z

Sorry, just noticed app/Dockerfile has node 18 still as well. Also line 6 of app/README.md cites Node 18.

😱 So many node versions! Great spot!

run backend with basicauth for smoke test

d9030c4

EmmaLRussell added 25 commits February 17, 2025 17:04

add playwright config

401f806

add e2e to ci, first part of server script

f4c8cee

can run e2e tests with github auth

44810f1

format

999dfc1

add filter test

858c3b7

add packet page summary tests (local and generic)

1b70507

packetPage section tests

47b2c73

lint

9110d07

create super user in ci

24d37e7

lint

f96ff73

install playwright

65e5a64

add Platform test

5fb99dc

Merge branch 'main' into mrc-4333-e2e

d1fc962

lint

a433d1d

export base url var in ci

78b79cd

add local parameter group row test

51c7f14

packet group page tests

131f07e

add parameter column test

45ed78f

update readme and run app in dev on ci

b659293

test fixes for ci

c33c16e

comments

46c86a1

fixes for dev run

72c2ae9

format

65470ce

docker run does not need basic auth

f88ea33

revert previous change

42fa7b9

EmmaLRussell marked this pull request as ready for review February 26, 2025 09:37

EmmaLRussell requested review from david-mears-2 and plietar February 26, 2025 09:37

updates for prod

c59b700

EmmaLRussell requested review from plietar and david-mears-2 March 10, 2025 09:42

david-mears-2 reviewed Mar 12, 2025

View reviewed changes

EmmaLRussell and others added 6 commits March 17, 2025 09:44

check that test files import "test" from the tag test fixture

caf28a0

Update README.md

66a606f

Co-authored-by: David Mears <[email protected]>

Update app/e2e/packetPage.demo.spec.ts

746c90c

Co-authored-by: David Mears <[email protected]>

Update app/playwright.config.ts

cf9be0b

Co-authored-by: David Mears <[email protected]>

lint

d168b21

update dev and prod urls

005330f

EmmaLRussell added 3 commits March 17, 2025 15:26

CI timeout increase :(

df25e94

try checkbox click

15dee7e

reset timeout

ef187e3

plietar approved these changes Mar 17, 2025

View reviewed changes

EmmaLRussell and others added 6 commits March 18, 2025 10:13

Update README.md

a2d8065

Co-authored-by: Paul Liétar <[email protected]>

set locale in config

12dbc8f

Merge branch 'mrc-4333-e2e' of github.com:mrc-ide/packit into mrc-433…

63c64f1

…3-e2e

update README

da88cfd

update tagCheckFixture for dev subdomains

7cb2e42

use tagTestFixure from auth.setup and explicitly set base url in defa…

ed09eb4

…ult e2e test script

plietar approved these changes Mar 18, 2025

View reviewed changes

david-mears-2 approved these changes Mar 18, 2025

View reviewed changes

david-mears-2 requested changes Mar 18, 2025

View reviewed changes

update node versions

0eb3b65

EmmaLRussell requested a review from david-mears-2 March 18, 2025 14:34

david-mears-2 approved these changes Mar 18, 2025

View reviewed changes

EmmaLRussell merged commit 9937f93 into main Mar 18, 2025
5 of 6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mrc-4333 e2e tests #179

mrc-4333 e2e tests #179

EmmaLRussell commented Feb 17, 2025 •

edited

Loading

codecov bot commented Feb 17, 2025 •

edited

Loading

EmmaLRussell commented Mar 10, 2025

david-mears-2 left a comment

david-mears-2 Mar 12, 2025

david-mears-2 Mar 12, 2025

EmmaLRussell Mar 13, 2025

EmmaLRussell commented Mar 17, 2025 •

edited

Loading

plietar left a comment

plietar Mar 17, 2025

EmmaLRussell Mar 18, 2025

plietar Mar 17, 2025

EmmaLRussell Mar 18, 2025

plietar Mar 17, 2025

plietar Mar 17, 2025

EmmaLRussell Mar 18, 2025

david-mears-2 left a comment

david-mears-2 left a comment

EmmaLRussell commented Mar 18, 2025

		@@ -0,0 +1,93 @@
		import { test as setup, expect, Page } from "@playwright/test";

	// We expect servers we're testing on to have either GB or US locale.
	// We expect browsers we're testing on to have either GB or US locale.

	const isDev = (baseURL: string) => baseURL.startsWith("https://packit-dev.dide.ic.ac.uk");
	const isDev = (baseURL: string) => new URL(baseURL).hostname.endsWith("https://packit-dev.dide.ic.ac.uk");

mrc-4333 e2e tests #179

mrc-4333 e2e tests #179

Conversation

EmmaLRussell commented Feb 17, 2025 • edited Loading

codecov bot commented Feb 17, 2025 • edited Loading

Codecov Report

EmmaLRussell commented Mar 10, 2025

david-mears-2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EmmaLRussell commented Mar 17, 2025 • edited Loading

plietar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-mears-2 left a comment

Choose a reason for hiding this comment

david-mears-2 left a comment

Choose a reason for hiding this comment

EmmaLRussell commented Mar 18, 2025

EmmaLRussell commented Feb 17, 2025 •

edited

Loading

codecov bot commented Feb 17, 2025 •

edited

Loading

EmmaLRussell commented Mar 17, 2025 •

edited

Loading