This is an automated email from the ASF dual-hosted git repository.
tbonelee pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/zeppelin.git
The following commit(s) were added to refs/heads/master by this push:
new 63609aa30d [ZEPPELIN-6560] Lint the new UI e2e suite and document its
conventions
63609aa30d is described below
commit 63609aa30df368daecfcbc075a04c76fb5c34479
Author: YONGJAE LEE (이용재) <[email protected]>
AuthorDate: Mon Jul 27 23:26:06 2026 +0900
[ZEPPELIN-6560] Lint the new UI e2e suite and document its conventions
### What is this PR for?
The new UI e2e suite has never been linted. `angular.json` sets
`lintFilePatterns` to `src/**/*.ts` and `src/**/*.html`, so nothing under
`e2e/` is checked beyond what the TypeScript compiler catches.
`zeppelin-web-angular` has no unit or component test target either: no `test`
target in `angular.json`, no `npm test` script, no spec files under `src/`. The
e2e suite is the app's only automated coverage, so a check in it that quietly
passes goes unnoticed. Two are in the tree today. `e [...]
```ts
await expect(page.locator('.paragraph-control .fa-spin')).not.toBeVisible({
timeout: 15000 });
```
`.fa-spin` is defined in the vendored FontAwesome stylesheet but applied by
no Angular template, so the locator matches no element, and `not.toBeVisible`
passes when a locator resolves to no DOM node. Separately,
`e2e/tests/notebook/paragraph/copy-to-clipboard.spec.ts:25` attributes coverage
to `PAGES.SHARE.SHARE_RESULT`, a key that does not exist: `SHARE_RESULT` lives
under `PAGES.WORKSPACE`. Playwright transpiles without type checking, so the
expression is `undefined` and that descr [...]
This PR adds `e2e/**/*.ts` to `lintFilePatterns` and applies
`eslint-plugin-playwright`'s recommended flat config to that path. The
pre-commit hook runs `eslint --fix` through lint-staged, and `--fix` applies
fixes at any severity including warnings, so an autofixable rule rewrites test
code without review. `playwright/no-useless-not` rewrites `.not.toBeVisible()`
into `.toBeHidden()`; both forms pass when the locator matches nothing, but the
rewritten one reads as a deliberate hidden [...]
Auditing the tree against the rules the doc states turned up a few more,
fixed here: an unused catch binding in `login-page.util.ts`, `eslint-disable`
markers carrying the tracking key on the four ZEPPELIN-6379 skips, reasons on
the `waitForTimeout` calls in `utils.ts` and `react-footer.spec.ts`, reasons on
the two `eslint-disable` lines in `notebook-keyboard-page.ts`, a message on the
unexplained WebKit skip in `dark-mode.spec.ts`, and the missing `PAGES`
annotation in `user-menu-nav [...]
`e2e/AGENTS.md` is updated too. It drops the `Tooling: Use e2e-skills`
section, which pointed contributors at an external personal repository, along
with the vendor-specific setup notes in the preamble. In their place it gains
an escape-hatch contract (`// JUSTIFIED:` and `// eslint-disable-next-line ...
-- <why>`) and a locator hierarchy that explains why `exact: true` matters and
when a CSS selector is acceptable. Four rules that claimed more than the tree
delivers were brought back [...]
### What type of PR is it?
Improvement
### Todos
* [x] Run `npm run lint` with the e2e patterns added
* [x] Check every autofixable rule for one that could change a test's
meaning through the pre-commit hook
* [x] Compare each override against the preset severity so the config
states only real deviations
* [x] Confirm every `PAGES` reference resolves
### What is the Jira issue?
ZEPPELIN-6560
### How should this be tested?
This is a lint and documentation change; no test behaviour changes.
```bash
cd zeppelin-web-angular
npm run lint
```
Expect 0 errors. The warnings are the debt described above. To see the new
coverage on its own, run `npx eslint 'e2e/**/*.ts'`; before this PR `ng lint`
reported nothing for `e2e/` because the path was not in `lintFilePatterns`.
Adding a violation of any error-level rule, for example
`test.describe.skip('x', () => {})` in a spec, should now fail the lint.
The coverage-key fix is visible in the run's annotations: `npm run e2e:fast
-- tests/notebook/paragraph/copy-to-clipboard.spec.ts` attributes that describe
to the `share/result` component instead of `undefined`. That spec needs a
running shell interpreter, so it skips on CI.
### Screenshots (if appropriate)
### Questions:
* Does the license files need to update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No
Closes #5348 from voidmatcha/fix/e2e-locator-conventions.
Signed-off-by: ChanHo Lee <[email protected]>
---
zeppelin-web-angular/angular.json | 2 +-
zeppelin-web-angular/e2e/AGENTS.md | 187 +++++++--------------
zeppelin-web-angular/e2e/models/login-page.util.ts | 2 +-
.../e2e/models/notebook-keyboard-page.ts | 4 +-
.../keyboard/notebook-keyboard-shortcuts.spec.ts | 10 +-
.../notebook/paragraph/copy-to-clipboard.spec.ts | 2 +-
.../tests/notebook/paragraph/react-footer.spec.ts | 2 +
.../e2e/tests/theme/dark-mode.spec.ts | 4 +-
.../tests/workspace/user-menu-navigation.spec.ts | 4 +-
zeppelin-web-angular/e2e/utils.ts | 1 +
zeppelin-web-angular/eslint.config.js | 21 +++
zeppelin-web-angular/package-lock.json | 30 ++++
zeppelin-web-angular/package.json | 1 +
13 files changed, 133 insertions(+), 137 deletions(-)
diff --git a/zeppelin-web-angular/angular.json
b/zeppelin-web-angular/angular.json
index 089cc07ce6..fa7d20ec48 100644
--- a/zeppelin-web-angular/angular.json
+++ b/zeppelin-web-angular/angular.json
@@ -151,7 +151,7 @@
"lint": {
"builder": "@angular-eslint/builder:lint",
"options": {
- "lintFilePatterns": ["src/**/*.ts", "src/**/*.html"]
+ "lintFilePatterns": ["src/**/*.ts", "src/**/*.html", "e2e/**/*.ts"]
}
}
}
diff --git a/zeppelin-web-angular/e2e/AGENTS.md
b/zeppelin-web-angular/e2e/AGENTS.md
index 900756358a..af81bb2303 100644
--- a/zeppelin-web-angular/e2e/AGENTS.md
+++ b/zeppelin-web-angular/e2e/AGENTS.md
@@ -17,81 +17,67 @@ limitations under the License.
# AGENTS.md
-> E2E (Playwright) conventions for `zeppelin-web-angular/e2e/`. A scoped
companion
-> to the repository-root AGENTS.md, loaded only when working under `e2e/`.
-> See [AGENTS.md specification](https://github.com/agentsmd/agents.md).
+> E2E (Playwright) conventions for `zeppelin-web-angular/e2e/`. A scoped
companion to the repository-root AGENTS.md, loaded only when working under
`e2e/`. See [AGENTS.md specification](https://github.com/agentsmd/agents.md).
-Config: `zeppelin-web-angular/playwright.config.js` (Angular UI) and
-`playwright.classic.config.js` (legacy classic UI), sharing
`playwright.shared.js`.
-This document is the shared source of truth for E2E conventions; Codex and
-agents.md-native tools read it directly.
-Claude Code / Gemini users can symlink `CLAUDE.md` / `GEMINI.md` to it locally
-(both gitignored, personal, not committed).
-
-## Tooling: Use e2e-skills
-
-Generate, review, and debug with
[e2e-skills](https://github.com/voidmatcha/e2e-skills)
-instead of ad-hoc prompts. It encodes the rules below and adds a deterministic
-silent-pass scanner.
-
-```bash
-npx skills add voidmatcha/e2e-skills -g --all # or -a <agent>
-```
-
-| Task | Skill |
-| --- | --- |
-| Generate new Playwright coverage | `playwright-test-generator` |
-| Review specs for silent-pass smells | `e2e-reviewer` |
-| Debug a failed Playwright report | `playwright-debugger` |
-| Deterministic local scan | `bash skills/e2e-reviewer/scripts/scan.sh e2e/` |
-
-Always run `e2e-reviewer` on generated specs. It catches always-passing
-assertions (`toBeDefined()`, `not.toBeNull()`) that pass while the feature is
broken.
+Config: `zeppelin-web-angular/playwright.config.js` (Angular UI) and
`playwright.classic.config.js` (legacy classic UI), sharing
`playwright.shared.js`. This document is the source of truth for E2E
conventions, for contributors and for coding agents alike.
## Layout
-- Specs: `e2e/tests/<area>/<feature>.spec.ts` (areas: `authentication`, `home`,
- `login`, `notebook`, `share`, `theme`, `workspace`).
+- Specs: `e2e/tests/<area>/[<group>/]<feature>.spec.ts` (areas:
`authentication`, `home`, `login`, `notebook`, `share`, `theme`, `workspace`).
Larger areas group specs one level deeper, as in `notebook/keyboard/` and
`workspace/notebook-repos/`. `tests/app.spec.ts` covers the app shell and sits
outside any area.
- Page Objects (POM), split by role:
- `e2e/models/<name>.ts`: locators + primitive actions (click, fill,
navigate, simple state checks).
- `e2e/models/<name>.util.ts`: workflows, composite verification, scenario
helpers.
+ - Most existing POMs are a single file. Split a new one by role, and split
an existing one when its workflow code outgrows its locators.
- Shared helpers: `e2e/utils.ts`.
## Style
- English only. No unnecessary comments.
-- BDD via `test.step('Given/When/Then …', …)`, as in existing specs.
-- One `test.describe` per feature; construct the POM in `beforeEach`.
+- BDD via `test.step('Given/When/Then …', …)`. Steps show up in traces and
reports; `// Given:` comments do not. Some specs still use comments; migrate a
test's comments to steps when you touch it.
+- One `test.describe` per feature; construct the feature's own POM in
`beforeEach`. A secondary POM that only one test needs, such as the second
viewer in a collaboration test or a page reached mid-test, can be built in the
test body.
+- `test.describe.serial` is a last resort: one failure skips every later test
in the group, which hides the rest instead of reporting them. Playwright
recommends against it (https://playwright.dev/docs/test-parallel#serial-mode).
Prefer making each test set up its own state.
+
+## Escape hatches
+
+Two comment forms mark a deliberate rule violation, and both require a reason:
+
+- `// JUSTIFIED: <why>` for the conventions in this document, either trailing
the offending line or in the comment block directly above it. It is a contract
with the reviewer: the marker says the deviation is deliberate and the reason
says why. A `test.describe.serial` group needs one too.
+- `// eslint-disable-next-line <rule> -- <why>` for a lint rule. Give a reason
after the `--`; if the violation is tracked elsewhere, the ticket key is that
reason.
+
+When a rule is both a convention here and a lint rule, `// JUSTIFIED:` is the
one to use: an `eslint-disable` silences the linter but leaves the convention
unmet.
+
+Neither hatch is a way to opt out of thinking. One without a concrete reason
will be challenged in review.
## Locators
Prefer user-facing, in this order:
-1. `getByRole('button' | 'link' | 'textbox', { name })`, `getByLabel`,
`getByText`.
-2. Last resort: `data-testid` (attribute selector) when a role/label is
unavailable
- and a CSS chain would be brittle.
-3. Forbidden: raw CSS chains and XPath.
+1. `getByRole('button' | 'link' | 'textbox', { name })`, `getByLabel`,
`getByText`. Pass `exact: true` alongside `name`. The default matches the
accessible name as a case-insensitive substring, which collides with note
titles and other page content and fails strict mode.
+2. `data-testid` (attribute selector) when a role or label is unavailable.
Adding one to the Angular or React template is allowed, and is better than
reaching into component internals with a CSS selector.
+3. A CSS selector only when the element offers neither, which is common for
ng-zorro internals and icon-only controls. It belongs in the Page Object, named
for what it does (`cancelButton`, not `.cancel-para`), never inline in a spec.
An accessible name that is really an icon glyph (`pause-circle`) is not an
improvement; it rots on the next icon swap.
+
+XPath is forbidden outright.
+
+Much of the suite predates this section: it inlines CSS and mostly omits
`exact: true`. The ratchet is that new or modified code complies. When you
touch a test that inlines a selector, move it into the Page Object as part of
that change.
## Assertions
-- Web-first, auto-waiting assertions only: `toBeVisible`, `toHaveURL`,
- `toHaveText`, `toHaveCount`.
-- No `waitForTimeout`. When waiting on a count, use `toHaveCount`.
-- No one-shot boolean checks (`expect(await el.isVisible())`) and no
- always-true assertions (`toBeDefined`, `not.toBeNull`).
+- Web-first, auto-waiting assertions only: `toBeVisible`, `toHaveURL`,
`toHaveText`, `toHaveCount`. The suite still asserts on values it extracted
first in a few places, which `playwright/prefer-web-first-assertions` reports;
the ratchet applies here too.
+- No `waitForTimeout` without a `// JUSTIFIED:` rationale. When waiting on a
count, use `toHaveCount`.
+- No one-shot boolean checks (`expect(await el.isVisible())`) and no
always-true assertions on a locator (`toBeDefined`, `not.toBeNull`). A Locator
is always a defined, non-null object, so those pass whether or not the element
exists. Asserting a non-locator value is not this smell:
`expect.poll(...).not.toBeNull()` and a null-guard on a regex match are both
legitimate.
+- A conditional may gate a setup action on dual-mode UI (auth vs anonymous,
the optional welcome modal), which is why `playwright/no-conditional-in-test`
is off. Do not put an `expect` inside one: an assertion that runs on only one
branch passes by skipping the check it exists to make.
`playwright/no-conditional-expect` reports those and the suite still carries
some, so the ratchet applies here too.
+- A network wait is synchronization, not proof.
`waitForLoadState('networkidle')` is discouraged by Playwright and the suite
still has several, one of them inside `waitForZeppelinReady`; in new code wait
on a user-visible signal instead. When you do wait on the network, assert the
rendered result afterwards.
+- The lint config covers part of this section, not all of it.
`eslint-plugin-playwright` has no rule for always-true assertions, so those are
a review responsibility.
## Readiness & Auth
-- After navigation, wait with `waitForZeppelinReady(page)` from `e2e/utils.ts`
- (not fixed sleeps).
-- Auth is programmatic: the `setup` project logs in once and writes
- `playwright/.auth/user.json`; browser projects consume it via `storageState`.
- Do not add per-test login races. For logged-out scenarios use a fresh
context.
+- After navigation, wait with `waitForZeppelinReady(page)` from `e2e/utils.ts`
(not fixed sleeps).
+- Auth is programmatic: the `setup` project logs in once and writes
`playwright/.auth/user.json`; browser projects consume it via `storageState`.
Do not add per-test login races. For logged-out scenarios use a fresh context.
+- A skip says why it skipped. `playwright/no-skipped-test` errors on the
declaration forms (`test.skip('title', fn)`, `test.describe.skip`) and on a
bare `test.skip()` outside an `if`; those need the `eslint-disable` hatch and a
tracking key. Every other skip passes lint whatever its message says, so the
message is a convention, not a gate: name the missing capability (auth mode,
interpreter, environment feature) or the tracking key.
## Coverage Annotation (Required)
-Every `describe` must declare the page/component it exercises so coverage is
-attributed:
+Every `describe` must declare the page/component it exercises so coverage is
attributed:
```ts
import { addPageAnnotationBeforeEach, PAGES } from '../../utils';
@@ -102,23 +88,20 @@ test.describe('Home Page - Core Elements', () => {
});
```
-Use an existing key from the `PAGES` object in `e2e/utils.ts`; add a new one
-there if the page is missing. `PAGES` is also the coverage-instrumentation set
-(`getCoverageTransformPaths`), so it defines the coverage denominator. Purely
-structural / non-page components (lifecycle hooks, shared UI primitives like
the
-spinner or resize handle) are intentionally omitted from `PAGES`. They are
-exercised transitively and are not counted.
+Use an existing key from the `PAGES` object in `e2e/utils.ts`; add a new one
there if the page is missing. `PAGES` is also the coverage-instrumentation set
(`getCoverageTransformPaths`), so it defines the coverage denominator. Purely
structural / non-page components (lifecycle hooks, shared UI primitives like
the spinner or resize handle) are intentionally omitted from `PAGES`. They are
exercised transitively and are not counted.
## Running
-- Node: `nvm use` (pinned in `.nvmrc`, currently 22.21.1).
-- Dev server: `npm run start` at `http://localhost:4200` (Playwright reuses a
- running one via `webServer.reuseExistingServer`).
+- Node: `nvm use` (version pinned in `.nvmrc`).
+- Dev server: `npm run start` at `http://localhost:4200` (Playwright reuses a
running one via `webServer.reuseExistingServer`).
| Command | Purpose |
| --- | --- |
| `npm run e2e` | Full suite |
| `npm run e2e:fast` | Chromium only (fast) |
+| `npm run e2e:fast -- tests/<area>/<feature>.spec.ts` | One spec (path is
relative to `e2e/`) |
+| `npm run e2e:fast -- -g '<test title>'` | One test, matched by title |
+| `npx eslint e2e/tests/<area>/<feature>.spec.ts` | Lint one file; `npm run
lint` covers the whole app |
| `npm run e2e:classic` | Classic `/classic` UI suite against `:8080` (needs
`-Pweb-classic`) |
| `npm run e2e:ui` | Playwright Test UI |
| `npm run e2e:headed` | Headed run |
@@ -132,90 +115,40 @@ exercised transitively and are not counted.
## Adding a Test (Agents Start Here)
1. Pick/confirm the target route and the `PAGES` key.
-2. Copy the shape of an existing spec in the same `<area>`; reuse or extend the
- matching POM (`models/<name>.ts` + `.util.ts`). Do not inline selectors the
- POM already owns.
-3. Annotate the page (`addPageAnnotationBeforeEach`), navigate, then
- `waitForZeppelinReady`.
-4. Run `npm run e2e:fast` and iterate until green; then run `e2e-reviewer`.
+2. Copy the shape of an existing spec in the same `<area>`; reuse or extend
the matching POM (`models/<name>.ts` + `.util.ts`). Do not inline selectors the
POM already owns.
+3. Annotate the page (`addPageAnnotationBeforeEach`), navigate, then
`waitForZeppelinReady`.
+4. Run `npm run e2e:fast` and iterate until green.
## Migration (Angular to React Microfrontend)
-Pages are moving from Angular to React fragments incrementally. Today this is
-narrow: the published paragraph route reads a `?react=true` flag
-(`published/paragraph/paragraph.component`), and the notebook footer swaps via
a
-`?reactFooter=true` flag (read into the notebook component's `useReactFooter`
-input). Both are query params inside the hash. There is no app-wide "flip this
-route to React" flag, and
-no cross-framework parity project in this config. Write specs so they survive a
-route being reimplemented, but do not build parity infrastructure ahead of
need.
+Pages are moving from Angular to React fragments incrementally. Today this is
narrow: the published paragraph route reads a `?react=true` flag
(`published/paragraph/paragraph.component`), and the notebook footer swaps via
a `?reactFooter=true` flag (read into the notebook component's `useReactFooter`
input). Both are query params inside the hash. There is no app-wide "flip this
route to React" flag, and no cross-framework parity project in this config.
Write specs so they survive a route [...]
### Write Framework-Neutral Specs
-- Assert observable behavior only: what the user sees, the URL, network
effects.
- Avoid asserting framework internals (`[ng-version]`, Angular component
classes,
- `zeppelin-*` custom-element tags) except in a deliberate feature-flag test.
-- Keep the locator order from the Locators section (role/label/text first). At
a
- seam that will flip frameworks, prefer a shared `data-testid` that both
- implementations render.
-- Never use fixed waits at a fragment seam. Wait on a user-visible post-mount
- signal or the specific remote response (`page.waitForResponse` on the
fragment
- chunk), then assert the rendered result. `react-footer.spec.ts` shows the
- fallback pattern (`page.route('**/remoteEntry.js', route => route.abort())`).
+- Assert observable behavior only: what the user sees, the URL, network
effects. Avoid asserting framework internals (`[ng-version]`, Angular component
classes, `zeppelin-*` custom-element tags) except in a deliberate feature-flag
test.
+- Keep the locator order from the Locators section (role/label/text first). At
a seam that will flip frameworks, prefer a shared `data-testid` that both
implementations render.
+- Never use fixed waits at a fragment seam. Wait on a user-visible post-mount
signal or the specific remote response (`page.waitForResponse` on the fragment
chunk), then assert the rendered result. `react-footer.spec.ts` shows the
fallback pattern (`page.route('**/remoteEntry.js', route => route.abort())`).
### When a Route Gains a React Flag
-- The flag is a route query param read via `ActivatedRoute.queryParams`, so
with
- the hash router it goes INSIDE the hash:
`/#/notebook/<id>/paragraph/<id>?react=true`,
- not before the `#`. Popups opened by app code (`window.open`) will not carry
a
- flag added only to `page.goto`.
-- To exercise both frameworks, follow the existing precedent and toggle the
flag
- in-spec: navigate the same spec with and without the flag across tests, as
- `published-paragraph.spec.ts` does. A separate flag-appending Playwright
project
- is an alternative, but scope it (its own `testMatch`) to routes that read the
- flag rather than running the whole suite twice.
+- The flag is a route query param read via `ActivatedRoute.queryParams`, so
with the hash router it goes INSIDE the hash:
`/#/notebook/<id>/paragraph/<id>?react=true`, not before the `#`. Popups opened
by app code (`window.open`) will not carry a flag added only to `page.goto`.
+- To exercise both frameworks, follow the existing precedent and toggle the
flag in-spec: navigate the same spec with and without the flag across tests, as
`published-paragraph.spec.ts` does. A separate flag-appending Playwright
project is an alternative, but scope it (its own `testMatch`) to routes that
read the flag rather than running the whole suite twice.
### Coverage
-- Coverage is tracked by `PAGES` key, not source file. The key is the stable
- identity; the path behind it is an implementation detail. When a page moves
to
- React, update its path in `PAGES` rather than deleting the key (deleting
drops
- it from the coverage denominator). Specs keep the same
- `addPageAnnotationBeforeEach(PAGES.KEY)` call across the migration.
+- Coverage is tracked by `PAGES` key, not source file. The key is the stable
identity; the path behind it is an implementation detail. When a page moves to
React, update its path in `PAGES` rather than deleting the key (deleting drops
it from the coverage denominator). Specs keep the same
`addPageAnnotationBeforeEach(PAGES.KEY)` call across the migration.
### Suite Shape
-- Keep the composed suite focused on real cross-seam user flows. Behavior that
- lives entirely inside one fragment belongs in that fragment's own tests; do
not
- grow the composed suite into a per-fragment unit suite.
+- Keep the composed suite focused on real cross-seam user flows. Behavior that
lives entirely inside one fragment belongs in that fragment's own tests; do not
grow the composed suite into a per-fragment unit suite.
## Classic UI Tests (`e2e/tests/classic/`)
-`e2e/tests/classic/` runs Playwright against the legacy AngularJS app served at
-`/classic`, ported from the retired `zeppelin-web` Protractor suite. Treat it
as
-a frozen legacy surface: keep it at parity coverage and test new features only
in
-the Angular/React suites.
-
-- **Locators (classic exception):** the classic templates predate roles and
- `data-testid`, so the role/label/text-first rule cannot apply. Sanctioned
here:
- element ids (`#findInput`), `ng-click="..."` / `ng-controller="..."`
attribute
- selectors, class selectors the legacy templates already expose (`.username`,
- `.interpreterHead`), and Ace/Select2 internals. Do not add `data-testid` to
the
- frozen `zeppelin-web` sources.
-- **Readiness:** `waitForZeppelinReady` is Angular-specific (`[ng-version]`)
and
- does not resolve on `/classic`; gate on a classic-visible signal instead
(e.g.
- the first `ParagraphCtrl` paragraph, or `.ace_text-input` attached).
-- **Coverage:** `PAGES` is the Angular coverage denominator; classic pages are
- intentionally outside it, so `addPageAnnotationBeforeEach` is not used here.
-- **Running:** the classic suite has its own config,
`playwright.classic.config.js`
- (Desktop Chrome only, targets `http://localhost:8080`), and needs a Zeppelin
- server built with `-Pweb-classic` — the `:4200` dev server does not serve
- `/classic`, so a plain `npm run e2e` never includes it. Run it with
- `npm run e2e:classic` (single spec: `npm run e2e:classic --
tests/classic/<spec>`).
- In CI the workflow enables it on the anonymous matrix leg only
- (`-Dweb.e2e.classic.disabled=false`), matching the anonymous-only legacy
- Protractor suite.
-- **POM:** inlining locators/helpers is acceptable while the suite is this
small;
- if it grows, move them behind `models/classic-*.ts` / `*.util.ts`.
-- The React-migration / framework-neutral-spec guidance does not apply to
- `tests/classic/`.
+`e2e/tests/classic/` runs Playwright against the legacy AngularJS app served
at `/classic`, ported from the retired `zeppelin-web` Protractor suite. Treat
it as a frozen legacy surface: keep it at parity coverage and test new features
only in the Angular/React suites.
+
+- **Locators (classic exception):** the classic templates predate roles and
`data-testid`, so the role/label/text-first rule cannot apply. Sanctioned here:
element ids (`#findInput`), `ng-click="..."` / `ng-controller="..."` attribute
selectors, class selectors the legacy templates already expose (`.username`,
`.interpreterHead`), and Ace/Select2 internals. Do not add `data-testid` to the
frozen `zeppelin-web` sources.
+- **Readiness:** `waitForZeppelinReady` is Angular-specific (`[ng-version]`)
and does not resolve on `/classic`; gate on a classic-visible signal instead
(e.g. the first `ParagraphCtrl` paragraph, or `.ace_text-input` attached).
+- **Coverage:** `PAGES` is the Angular coverage denominator; classic pages are
intentionally outside it, so `addPageAnnotationBeforeEach` is not used here.
+- **Running:** the classic suite has its own config,
`playwright.classic.config.js` (Desktop Chrome only, targets
`http://localhost:8080`), and needs a Zeppelin server built with
`-Pweb-classic`. The `:4200` dev server does not serve `/classic`, so a plain
`npm run e2e` never includes it. Run it with `npm run e2e:classic` (single
spec: `npm run e2e:classic -- tests/classic/<spec>`). In CI the workflow
enables it on the anonymous matrix leg only
(`-Dweb.e2e.classic.disabled=false`), match [...]
+- **POM:** inlining locators/helpers is acceptable while the suite is this
small; if it grows, move them behind `models/classic-*.ts` / `*.util.ts`.
+- The React-migration / framework-neutral-spec guidance does not apply to
`tests/classic/`.
diff --git a/zeppelin-web-angular/e2e/models/login-page.util.ts
b/zeppelin-web-angular/e2e/models/login-page.util.ts
index e5cce7fee5..825c7111b0 100644
--- a/zeppelin-web-angular/e2e/models/login-page.util.ts
+++ b/zeppelin-web-angular/e2e/models/login-page.util.ts
@@ -42,7 +42,7 @@ export class LoginTestUtil {
try {
await access(this.SHIRO_CONFIG_PATH);
this._isShiroEnabled = true;
- } catch (error) {
+ } catch {
this._isShiroEnabled = false;
}
diff --git a/zeppelin-web-angular/e2e/models/notebook-keyboard-page.ts
b/zeppelin-web-angular/e2e/models/notebook-keyboard-page.ts
index fb4a24a3a6..2ffedd0b64 100644
--- a/zeppelin-web-angular/e2e/models/notebook-keyboard-page.ts
+++ b/zeppelin-web-angular/e2e/models/notebook-keyboard-page.ts
@@ -574,7 +574,7 @@ export class NotebookKeyboardPage extends BasePage {
.waitForFunction(
// waitForFunction executes in browser context, not Node.js context.
// Browser cannot access Node.js variables like
PARAGRAPH_RESULT_SELECTOR.
- // eslint-disable-next-line @typescript-eslint/no-explicit-any
+ // eslint-disable-next-line @typescript-eslint/no-explicit-any -- args
cross the browser boundary untyped
([index, selector]: any[]) => {
const paragraphs =
document.querySelectorAll('zeppelin-notebook-paragraph'); // JUSTIFIED:
index-based paragraph lookup with sub-element checks not expressible via
Playwright locator API
const targetParagraph = paragraphs[index];
@@ -622,7 +622,7 @@ export class NotebookKeyboardPage extends BasePage {
.waitForFunction(
// waitForFunction executes in browser context, not Node.js context.
// Browser cannot access Node.js variables like
PARAGRAPH_RESULT_SELECTOR.
- // eslint-disable-next-line @typescript-eslint/no-explicit-any
+ // eslint-disable-next-line @typescript-eslint/no-explicit-any -- args
cross the browser boundary untyped
([index, selector]: any[]) => {
const paragraphs =
document.querySelectorAll('zeppelin-notebook-paragraph'); // JUSTIFIED:
index-based paragraph lookup with sub-element checks not expressible via
Playwright locator API
const targetParagraph = paragraphs[index];
diff --git
a/zeppelin-web-angular/e2e/tests/notebook/keyboard/notebook-keyboard-shortcuts.spec.ts
b/zeppelin-web-angular/e2e/tests/notebook/keyboard/notebook-keyboard-shortcuts.spec.ts
index 6f3029fa51..f4e584d215 100644
---
a/zeppelin-web-angular/e2e/tests/notebook/keyboard/notebook-keyboard-shortcuts.spec.ts
+++
b/zeppelin-web-angular/e2e/tests/notebook/keyboard/notebook-keyboard-shortcuts.spec.ts
@@ -25,7 +25,7 @@ import {
* (src/app/key-binding/shortcuts-map.ts). The page object gates on Monaco's
`focused`
* class before dispatching shortcuts; effects are asserted with web-first
expectations.
*/
-// Serial ordering prevents cross-test editor state corruption within the
shared notebook.
+// JUSTIFIED: serial ordering prevents cross-test editor state corruption
within the shared notebook.
test.describe.serial('Comprehensive Keyboard Shortcuts (ShortcutsMap)', () => {
addPageAnnotationBeforeEach(PAGES.WORKSPACE.NOTEBOOK);
addPageAnnotationBeforeEach(PAGES.SHARE.SHORTCUT);
@@ -89,7 +89,9 @@ test.describe.serial('Comprehensive Keyboard Shortcuts
(ShortcutsMap)', () => {
// TODO: Fix the previously skipped tests - ZEPPELIN-6379
test.describe('ParagraphActions.RunAbove: Control+Shift+ArrowUp', () => {
+ // eslint-disable-next-line playwright/no-skipped-test -- tracked by
ZEPPELIN-6379
test.skip();
+
test('should run all paragraphs above current with Control+Shift+ArrowUp',
async () => {
// Given: Multiple paragraphs
await keyboardPage.tryFocusCodeEditor(0);
@@ -121,7 +123,9 @@ test.describe.serial('Comprehensive Keyboard Shortcuts
(ShortcutsMap)', () => {
// TODO: Fix the previously skipped tests - ZEPPELIN-6379
test.describe('ParagraphActions.RunBelow: Control+Shift+ArrowDown', () => {
+ // eslint-disable-next-line playwright/no-skipped-test -- tracked by
ZEPPELIN-6379
test.skip();
+
test('should run current and all paragraphs below with
Control+Shift+ArrowDown', async () => {
// Given: Multiple paragraphs with content
await keyboardPage.tryFocusCodeEditor(0);
@@ -666,7 +670,9 @@ test.describe.serial('Comprehensive Keyboard Shortcuts
(ShortcutsMap)', () => {
// TODO: Fix the previously skipped tests - ZEPPELIN-6379
test.describe('ParagraphActions.CutLine: Control+K', () => {
+ // eslint-disable-next-line playwright/no-skipped-test -- tracked by
ZEPPELIN-6379
test.skip();
+
test('should cut line with Control+K', async () => {
// Given: Code editor with content
await keyboardPage.tryFocusCodeEditor();
@@ -708,7 +714,9 @@ test.describe.serial('Comprehensive Keyboard Shortcuts
(ShortcutsMap)', () => {
// TODO: Fix the previously skipped tests - ZEPPELIN-6379
test.describe('ParagraphActions.PasteLine: Control+Y', () => {
+ // eslint-disable-next-line playwright/no-skipped-test -- tracked by
ZEPPELIN-6379
test.skip();
+
test('should paste line with Control+Y', async () => {
// Given: Content in the editor
await keyboardPage.tryFocusCodeEditor();
diff --git
a/zeppelin-web-angular/e2e/tests/notebook/paragraph/copy-to-clipboard.spec.ts
b/zeppelin-web-angular/e2e/tests/notebook/paragraph/copy-to-clipboard.spec.ts
index dc13297dd4..93a9b7029e 100644
---
a/zeppelin-web-angular/e2e/tests/notebook/paragraph/copy-to-clipboard.spec.ts
+++
b/zeppelin-web-angular/e2e/tests/notebook/paragraph/copy-to-clipboard.spec.ts
@@ -22,7 +22,7 @@ import {
} from '../../../utils';
test.describe('Copy table result to clipboard', () => {
- addPageAnnotationBeforeEach(PAGES.SHARE.SHARE_RESULT);
+ addPageAnnotationBeforeEach(PAGES.WORKSPACE.SHARE_RESULT);
let paragraphPage: NotebookParagraphPage;
let testNotebook: { noteId: string; paragraphId: string };
diff --git
a/zeppelin-web-angular/e2e/tests/notebook/paragraph/react-footer.spec.ts
b/zeppelin-web-angular/e2e/tests/notebook/paragraph/react-footer.spec.ts
index d76ff4438b..fd102b36d4 100644
--- a/zeppelin-web-angular/e2e/tests/notebook/paragraph/react-footer.spec.ts
+++ b/zeppelin-web-angular/e2e/tests/notebook/paragraph/react-footer.spec.ts
@@ -102,6 +102,8 @@ test.describe('React Paragraph Footer', () => {
await page.goto('/#/');
await waitForZeppelinReady(page);
+ // JUSTIFIED: outlives the 1500 ms stub so a destroy-time error has room
to surface.
+ // The assertion is that nothing happened, so there is no UI state to wait
on.
await page.waitForTimeout(2500);
expect(consoleErrors).toEqual([]);
diff --git a/zeppelin-web-angular/e2e/tests/theme/dark-mode.spec.ts
b/zeppelin-web-angular/e2e/tests/theme/dark-mode.spec.ts
index 49bbdf92cb..25f69daeb4 100644
--- a/zeppelin-web-angular/e2e/tests/theme/dark-mode.spec.ts
+++ b/zeppelin-web-angular/e2e/tests/theme/dark-mode.spec.ts
@@ -20,9 +20,7 @@ test.describe('Dark Mode Theme Switching', () => {
test.beforeEach(async ({ page, browserName }) => {
// TODO: This crash occurs only on WebKit. The root cause should be
investigated and addressed.
- if (browserName === 'webkit') {
- test.skip();
- }
+ test.skip(browserName === 'webkit', 'The theme toggle crashes the page on
WebKit');
darkModePage = new DarkModePage(page);
await page.goto('/#/');
await waitForZeppelinReady(page);
diff --git
a/zeppelin-web-angular/e2e/tests/workspace/user-menu-navigation.spec.ts
b/zeppelin-web-angular/e2e/tests/workspace/user-menu-navigation.spec.ts
index c4ee882cf1..656dcd5c61 100644
--- a/zeppelin-web-angular/e2e/tests/workspace/user-menu-navigation.spec.ts
+++ b/zeppelin-web-angular/e2e/tests/workspace/user-menu-navigation.spec.ts
@@ -12,7 +12,7 @@
import { expect, test } from '@playwright/test';
import { HeaderPage } from '../../models/header-page';
-import { waitForZeppelinReady } from '../../utils';
+import { addPageAnnotationBeforeEach, waitForZeppelinReady, PAGES } from
'../../utils';
/**
* Regression guard for the header user-menu navigation.
@@ -35,6 +35,8 @@ const MENU_ITEMS = [
];
test.describe('Header user menu - full-row navigation', () => {
+ addPageAnnotationBeforeEach(PAGES.SHARE.HEADER);
+
let header: HeaderPage;
test.beforeEach(async ({ page }) => {
diff --git a/zeppelin-web-angular/e2e/utils.ts
b/zeppelin-web-angular/e2e/utils.ts
index 25e6d4304b..7ccae56156 100644
--- a/zeppelin-web-angular/e2e/utils.ts
+++ b/zeppelin-web-angular/e2e/utils.ts
@@ -517,6 +517,7 @@ export const createTestNotebookWithName = async (
if (attempt === 3 || !isRetryableError(message)) {
throw new Error(`Failed to create test notebook: ${message}. Current
URL: ${page.url()}`);
}
+ // JUSTIFIED: backoff between REST create retries; no UI state to wait
on.
await page.waitForTimeout(1000 * attempt);
}
}
diff --git a/zeppelin-web-angular/eslint.config.js
b/zeppelin-web-angular/eslint.config.js
index ba289b6519..b6bb00f695 100644
--- a/zeppelin-web-angular/eslint.config.js
+++ b/zeppelin-web-angular/eslint.config.js
@@ -23,6 +23,7 @@ const prettier = require('eslint-config-prettier');
// from the former TSLint rule (ZEPPELIN-6372).
const localRules = require('./eslint-rules');
const perfectionist = require('eslint-plugin-perfectionist');
+const playwright = require('eslint-plugin-playwright');
module.exports = tseslint.config(
{
@@ -166,6 +167,26 @@ module.exports = tseslint.config(
'perfectionist/sort-exports': 'error'
}
},
+ {
+ files: ['e2e/**/*.ts'],
+ ...playwright.configs['flat/recommended'],
+ rules: {
+ ...playwright.configs['flat/recommended'].rules,
+ // Conditionals here gate setup actions for dual-mode UI (auth vs
anonymous, an optional welcome modal).
+ // no-conditional-expect stays on: an assertion reached on only one
branch passes without running.
+ 'playwright/no-conditional-in-test': 'off',
+ // Interpreter-backed specs skip on process.env.CI deliberately; only an
unconditional skip is a leak.
+ 'playwright/no-skipped-test': ['error', { allowConditional: true }],
+ // Autofixable, and the pre-commit hook applies fixes without review.
+ // It rewrites `.not.toBeVisible()` into `.toBeHidden()`; both pass when
the locator matches nothing,
+ // but the rewrite reads as a deliberate check, so a dead selector gets
harder to spot.
+ 'playwright/no-useless-not': 'off',
+ // Lowered from the preset's error: 13 networkidle waits and 9
extracted-value assertions already exist,
+ // and `ng lint` exits 0 on warnings.
+ 'playwright/prefer-web-first-assertions': 'warn',
+ 'playwright/no-networkidle': 'warn'
+ }
+ },
{
files: ['**/*.html'],
// == legacy `plugin:@angular-eslint/template/recommended`
diff --git a/zeppelin-web-angular/package-lock.json
b/zeppelin-web-angular/package-lock.json
index 9d0ccbc33e..544270d737 100644
--- a/zeppelin-web-angular/package-lock.json
+++ b/zeppelin-web-angular/package-lock.json
@@ -70,6 +70,7 @@
"eslint-plugin-import": "^2.32.0",
"eslint-plugin-jsdoc": "^50.8.0",
"eslint-plugin-perfectionist": "^5.10.0",
+ "eslint-plugin-playwright": "^2.10.5",
"eslint-plugin-prefer-arrow": "^1.2.3",
"https-proxy-agent": "^2.2.1",
"husky": "9.1.7",
@@ -12562,6 +12563,35 @@
"url": "https://opencollective.com/eslint";
}
},
+ "node_modules/eslint-plugin-playwright": {
+ "version": "2.10.5",
+ "resolved":
"https://registry.npmjs.org/eslint-plugin-playwright/-/eslint-plugin-playwright-2.10.5.tgz";,
+ "integrity":
"sha512-4k+ml4cEd55kePUIW1WsCiOLDwZkAdPZPKMFulR83XpplCaVOTKHF6yvXLYixLtdVbkMjmKV5F3BaGuI3aH8aQ==",
+ "dev": true,
+ "license": "MIT",
+ "dependencies": {
+ "globals": "^17.3.0"
+ },
+ "engines": {
+ "node": ">=16.9.0"
+ },
+ "peerDependencies": {
+ "eslint": ">=8.40.0"
+ }
+ },
+ "node_modules/eslint-plugin-playwright/node_modules/globals": {
+ "version": "17.7.0",
+ "resolved": "https://registry.npmjs.org/globals/-/globals-17.7.0.tgz";,
+ "integrity":
"sha512-Czmyns5dUsq4seFBR/Kdydhmo8y9kC79hiSkPn0YcGtNnYWnrgt0vjrSjx9tspoDGWm2CMarffRuLjM4xUz8xg==",
+ "dev": true,
+ "license": "MIT",
+ "engines": {
+ "node": ">=18"
+ },
+ "funding": {
+ "url": "https://github.com/sponsors/sindresorhus";
+ }
+ },
"node_modules/eslint-plugin-prefer-arrow": {
"version": "1.2.3",
"resolved":
"https://registry.npmjs.org/eslint-plugin-prefer-arrow/-/eslint-plugin-prefer-arrow-1.2.3.tgz";,
diff --git a/zeppelin-web-angular/package.json
b/zeppelin-web-angular/package.json
index e9d2b2ac15..756dab3127 100644
--- a/zeppelin-web-angular/package.json
+++ b/zeppelin-web-angular/package.json
@@ -97,6 +97,7 @@
"eslint-plugin-import": "^2.32.0",
"eslint-plugin-jsdoc": "^50.8.0",
"eslint-plugin-perfectionist": "^5.10.0",
+ "eslint-plugin-playwright": "^2.10.5",
"eslint-plugin-prefer-arrow": "^1.2.3",
"https-proxy-agent": "^2.2.1",
"husky": "9.1.7",
