getodk / getodk/web-forms

Address E2E issues and shortcomings

Open
#211 0 comments 0 reactions 0 assignees View on GitHub
test-automation
Dominant language
TypeScript
Stars
38
Forks
22
PR merge metrics
No merged PRs in 30d

Description

While reviewing #183, I was preparing a suggested change which would require a corresponding change to the `vue.test.ts` E2E test. This led to issues actually getting E2E tests to complete locally at all, and most of a day debugging the issue.

## Local system

I've concluded that it's **highly likely** part of the problem I encountered is related to the fact that I'm running macOS 13 (aka Ventura; specifically 13.5, build 22G74 if for some reason even that level of detail turns out to be pertinent).

I reached this conclusion after finding [this Playwright issue](https://github.com/microsoft/playwright/issues/30070) and [this PR](https://github.com/microsoft/playwright/pull/30079) fixing it

## Steps to reproduce

1. `yarn workspace @getodk/web-forms test:e2e`

## Observed _symptomatic behavior_

In WebKit, `vue.test.ts` consistently fails with a timeout. For clarity, since I think we should consider renaming this test module to something more descriptive of what it does: this contains one test, which:

1. Iterates through all demo fixtures, and for each fixture...
2. Loads the fixture
3. Attempts to fill all of the form's questions
4. If the form has a language menu, attempts to select the last language menu item

The failure occurs on the `01-itext-basic.xml` form, timing out on step 4 ([this line specifically](https://github.com/getodk/web-forms/blob/6f1ffe3456910c7ef4699c7794c7a40ec105b521/packages/web-forms/e2e/vue.test.ts#L80)).

**Importantly**, this is _only a symptom of the issue_.

## Root cause

While it may seem unrelated, the failure actually occurs earlier in the rendering process of `01-itext-basic.xml`.

- **In the current WebKit build, on my local macOS version:** a runtime error is produced when attempting to access the form-global validation popover's `hidePopover` method ([this line specifically](https://github.com/getodk/web-forms/blob/6f1ffe3456910c7ef4699c7794c7a40ec105b521/packages/web-forms/src/components/OdkWebForm.vue#L58)).

- **Regardless of OS, browser:**

- The E2E mechanism to detect and report errors [counts error/warning logs that may be issued](https://github.com/getodk/web-forms/blob/6f1ffe3456910c7ef4699c7794c7a40ec105b521/packages/web-forms/e2e/vue.test.ts#L5-L11) (notably, it does not collect any other detail about those logs, such as their text or where the logs occur). The test passes if _all test steps **for all form fixtures**_ complete without stalling or `throw`ing, and only after that there is an [assertion](https://github.com/getodk/web-forms/blob/6f1ffe3456910c7ef4699c7794c7a40ec105b521/packages/web-forms/e2e/vue.test.ts#L86) that no such error/warning logs were issued. This assertion is not reached if any earlier aspect of the test stalls or fails.

- If _any unhandled runtime error_ occurs at any point in rendering, the UI implementation does not currently provide any mechanism for E2E tests to detect that or invoke any assertions around it. And neither do the the E2E tests attempt to do so.

In this specific case, this is the root cause of the observed symptomatic behavior:

1. A runtime error occurs during a reactive effect.
2. Rendering scheduled past that point is either interrupted or delayed.
3. The test attempts to access a DOM structure which does not exist (at least at call time).
4. DOM access being achieved by Playwright's "wait for match and resolve" approach (also common in many other browser test runners/frameworks) never finds a match, waiting indefinitely until interrupted by test timeout.

### Likely, tangentially related, flakiness observed/root cause

I also observed and debugged a confounding factor, which I believe is partly responsible for _many of our flaky E2E test woes_. Specifically: the test's step to load each form fixture is [just a click call](https://github.com/getodk/web-forms/blob/6f1ffe3456910c7ef4699c7794c7a40ec105b521/packages/web-forms/e2e/vue.test.ts#L23). Form loading itself is asynchronous, but the test does not wait for any observable condition after initiating load of each fixture by click. (This is in contrast with the logic to wait for an [observable _page load_ condition](https://github.com/getodk/web-forms/blob/6f1ffe3456910c7ef4699c7794c7a40ec105b521/packages/web-forms/e2e/vue.test.ts#L16).

As I was debugging the `popover`/WebKit/macOS 13 issue, I also observed that flakiness _improves drastically_ by introducing a similar observable condition check for each fixture's form load.

## Followup actions to consider

- Introduce programmatically detectable runtime error reporting in the UI:

- Catch-all, for unknown/unhandled cases
- Specific handling for known/knowable fallible cases (few in the current engine API, but will soon be growing; many in the UI's other API accesses, albeit currently fall more into the "knowable" subcategory)

Reporting some or all of these errors may or may not _also_ be user-facing. But the purpose is to be able to identify, in tests, when and what errors occur **at all**. Reporting should accommodate known/expected points of potential failure (e.g. form load, specific fallible user interactions). **Importantly** reporting should also accommodate programmatically detecting _unknown/unexpected_ points of failure. (I don't want to be too prescriptive here, but think events.)

- Include assertions in tests around those programmatically detectable error reports; _at least_ in E2E tests, possibly also some unit/integration tests where pertinent.

- Since Vue and other dependencies seem to like reporting errors/warnings via `console` API, we should improve the aspects of the E2E test which observe and assert log activity:

- Collect more detail about the logs. At least their text, preferably also their file/URL, line number. All of these details are available from the same Playwright APIs already in use.

- Perform log assertions much more frequently:

- _At least_ at the end of each fixture's test steps
- _Preferably_ more frequently than that, e.g. after specific test steps (or even between each step!)
- _Ideally_ in a way that might be reported cumulatively in test run failures (e.g. `expect.soft` assertions, some means of deferring test interruption by collecting assertion failures and dumping them later)

- Break up `vue.test.ts` so there is a single test per form fixture it exercises, likely as an `each`/table test to start. I'm not sure whether this will require substantial tooling/build change (i.e. to make the fixture list available at test definition time), but I'm **hoping** it's possible with just the runtime information already available (i.e. `import.meta.glob`, Vite `define` config, something along those lines).

- While we're at it, let's rename `vue.test.ts to something more descriptive of what it actually tests! Does `fixture-form-filling.test.ts` sound good?

- As I mentioned in the tangentially related flakiness concern: introduce an observable condition check for each fixture's form load. This could be as simple as a class on the form root node which is only added once the form `RootNode` becomes non-null (i.e. when `Promise` resolves), and a corresponding Playwright locator assertion for that class.

## Consider E2E _approach_ with more intent

Lastly, I think we may want to re-evaluate the value of these very general E2E fixture tests. In theory, many or all of the above followup actions could make them more valuable than they are right now. But without some or all of these improvements, I think the value they provide is heavily outweighed by wasting a whole lot of time (CPU time in CI, our time in debugging and mitigation).

This isn't to say that we won't get value out of having E2E tests! I just think it's worth considering whether we'll get _more value_ out of testing _more specific functionality_. The fixture filling exercise is still compelling _as a smoketest_, but I think it's pretty far from reaching its potential.

The build-specific E2E test is valuable because it serves as a regression test against a recurring build issue.

Otherwise, I think we'll get the most value out of E2E tests which exercise distinct user-facing scenarios, and either:

- Make lower level execution challenging due to e.g. interactivity
- Involve browser-level features like persistence, workers, and other conventionally difficult-to-unit-test scenarios

Contributor guide

No contributing guide indexed for this repository

Research direction

Start by running `yarn workspace @getodk/web-forms test:e2e`, then read `packages/web-forms/e2e/vue.test.ts` and `packages/web-forms/src/components/OdkWebForm.vue`. The issue lists several possible changes, including runtime error reporting, richer log assertions, fixture test splitting, and load readiness checks; completion would require selecting and defining one bounded approach.

Written by the indexing model from the issue text.

Assessment

Tech stack
playwright, typescript
Domain
frontend, testing-qa
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.