Skip to content

POC: Refactor tests, increase speed, use API methods to create user#3427

Draft
anshulxyz wants to merge 2 commits intoonline-go:mainfrom
anshulxyz:refactor_e2e_tests
Draft

POC: Refactor tests, increase speed, use API methods to create user#3427
anshulxyz wants to merge 2 commits intoonline-go:mainfrom
anshulxyz:refactor_e2e_tests

Conversation

@anshulxyz
Copy link
Contributor

This is a Proof-of-Concept. Fixes slowness of the e2e tests to some degree.

Proposed Changes

In every test we don't have to test the user registration flow. That is taking a lot of time of the test. For e2e tests we can register users directly with the API instead of the UI. For speed.

We can have a single separate test for user registration through UI.

I have tested this with the4 demo board related tests, and have found that the newer tests are 60% faster.

Results:

With new user registration
Time: 11 + 9.6 = 20.6

With old user registration
Time: 18.5 + 14.1 = 32.6

I am all ears. Open to all suggestions.

@github-actions
Copy link

github-actions bot commented Mar 7, 2026

Review Notes

The speedup approach is interesting, but there are a few issues worth addressing before this lands.


1. Direct API calls conflict with project e2e policy

e2e-tests/CLAUDE.md explicitly states:

Avoid direct API calls - the intent of e2e testing is to test by driving the system as a user does.

atomicRegisterNewUser and atomicPrepareNewUser bypass the UI entirely for registration and onboarding. This is acknowledged in the PR description as a deliberate trade-off, but worth a conscious team decision before merging.


2. Silent CSRF token failure

In atomicRegisterNewUser:

const csrftoken = cookies.find((c) => c.name === "csrftoken")?.value || "";

If the CSRF cookie is not present after GET /, csrftoken silently becomes an empty string. The subsequent POST /api/v0/register will then fail with a 403, but the error message will only say "API Registration failed" with the server response body — making it non-obvious that the root cause was a missing CSRF token. A guard like if (!csrftoken) throw new Error("Failed to obtain CSRF token from session") would give much clearer diagnostics.


3. starting_rank API response is not checked

await request.put("/api/v1/me/starting_rank", {
    data: { choice: "basic" },
    headers: { "X-CSRFToken": csrftoken },
});

Unlike the registration call, this response is never checked. If it fails (e.g. due to an endpoint change or auth issue), the onboarding prompt will still appear during subsequent test actions and cause hard-to-diagnose failures downstream.


4. Wrong localStorage key — dynamic help suppression will silently fail

atomicPrepareNewUser sets:

localStorage.setItem("rdh-system-state", '{"enabled":false}');

However, OgsHelpProvider stores the RDH state via data.set("rdh-system-state", ..., data.Replication.REMOTE_OVERWRITES_LOCAL). The OGS data module reads local values under the ogs. prefix (i.e. ogs.rdh-system-state), and for remote-replicated keys the authoritative value comes from the server after sync. Writing to the bare key rdh-system-state in localStorage is neither the local key (ogs.rdh-system-state) nor the remote key (ogs-remote-storage-store.<user_id>.rdh-system-state), so the setting will be silently ignored and dynamic help popups will still appear, potentially interfering with tests.

The existing turnOffDynamicHelp() works because it interacts through the UI and persists the preference server-side.

Add error handling for missing CSRF token
Add error handling for setting starting rank
Correct the localStorage key Disable dynamic help popups
@github-actions
Copy link

Thanks for the POC — the speed improvement is significant. A few issues to address before this approach can be adopted:

Conflicts with stated e2e test principles

e2e-tests/CLAUDE.md explicitly says:

Avoid direct API calls - the intent of e2e testing is to test by driving the system as a user does.

atomicRegisterNewUser skips the UI entirely and calls /api/v0/register and /api/v1/me/starting_rank directly, and injects preferences via localStorage rather than navigating to /settings/help. This means the registration and onboarding flows are now completely untested by any test that switches to atomicPrepareNewUser. If those UI paths break, no test will catch it.

The PR description mentions adding a dedicated UI registration test to compensate, but that test is not included here, leaving a coverage gap.

ebi uniqueness in parallel test execution

In atomicRegisterNewUser, the device fingerprint is generated using only Date.now() for uniqueness. Unlike generateUniqueTestIPv6, which uses an incrementing per-process counter to handle same-millisecond collisions, the ebi has no such counter. Two parallel workers executing in the same millisecond will produce identical values. If the server treats ebi as part of rate-limiting or deduplication, this could cause flaky failures under parallel execution.

@anoek
Copy link
Member

anoek commented Mar 10, 2026

Conflicts with stated e2e test principles

e2e-tests/CLAUDE.md explicitly says:

Avoid direct API calls - the intent of e2e testing is to test by driving the system as a user does.

I feel like if we're doing something that makes the testing process notably faster, so long as we've exercised those parts (such as user registration) with an actual e2e test, we don't need to always drive it through the whole browser method necessarily. That said, I defer to @GreenAsJade , if he feels it's better to always go through that whole process then I'm good with it too.

@GreenAsJade
Copy link
Collaborator

Yeah I mentioned to Anshul - it's cool to deliberately to it. I suggested we make sure that we have one test in each "class" of test where the users are "real" not "pre-fab", just in case something depends on the signup flow.

@GreenAsJade
Copy link
Collaborator

Maybe we should tell CLAUDE that we've decided this, in the README/CLAUDE, so it doesn't complain all the time, and checks that it's true!

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