[vogievetsky]

Note from the human

So the e2e tests in the web console have not been touched in a long time. On a whim I asked Claude "If I was added e2e tests today would they be written differently"... and boy did it have a lot to say.

While Claude was clauding I described what I was doing to @gianm and he said: "It would be cool if web console e2e tests used the embedded tests framework" and the rest is history...

Description

The web console's end-to-end tests ran in CI by building a full Druid distribution, starting it with script/druid, running the Playwright specs against it and stopping it. That was slow, the specs shared one long-lived cluster (so leftovers from one test could break another), and the console flows lived apart from the Druid features they exercise.

This PR runs the specs on the embedded clusters that the rest of Druid's tests use (embedded-tests), modernizes the specs themselves, and adds console coverage for features that the embedded tests already cover through the API. Each console test lives next to the embedded tests of its feature (for example s3/S3WebConsoleTest next to S3StorageTest), so the "load data from S3" flow is tested through the console too.

Running the console e2e specs on embedded clusters

• WebConsoleTestBase (in embedded-tests, package org.apache.druid.testing.embedded.console) starts an embedded cluster with a Router, which serves the console from the web-console jar, and runs a Playwright spec of web-console/e2e-tests on it with npx playwright test <spec>. A subclass adds the resources the spec needs (S3 or Kafka containers, extensions, properties) in addResources / configureCluster and passes their settings to the spec as environment variables. The JUnit failure message carries Playwright's output; each spec writes its screenshots and traces to its own web-console/test-results/<spec>/.
• The tests are tagged web-console: excluded from the default surefire run and run by failsafe with the new web-console-tests profile:

  ./mvnw -pl web-console install -DskipTests          # with the console built, for the Router to serve it
  ./mvnw -pl embedded-tests verify -Pweb-console-tests [-Dit.test=BatchIndexingWebConsoleTest]

• -Dweb.console.keepAlive=true keeps a test's cluster up and prints the command to run its spec against it (with --ui / --debug as you like). -Dweb.console.port=18081 runs the specs against the dev server (npm start) instead of the bundled console.
• Every test class gets a fresh cluster, so the specs no longer clean up after themselves there (DRUID_E2E_TEST_CLUSTER_IS_DISPOSABLE).
• WebConsoleSpecsTest (a plain unit test) fails if a spec of web-console/e2e-tests isn't run by any *WebConsoleTest, or if one runs a spec that doesn't exist.
• The specs still run against a quickstart cluster with npm run test-e2e (script/druid stays for that); the ones that need a resource (S3, Kafka, extra extensions, a lookup table) skip themselves there.

CI

.github/scripts/web-checks.sh installs web-console (with the console built) and then runs ./mvnw verify -pl 'embedded-tests,!web-console' -am -Pweb-console-tests -DskipUTs, which builds only the modules embedded-tests needs: no distribution, no extensions download, no separate JVMs to start. worker.yml uploads embedded-tests/target/failsafe-reports/ (the Druid and Playwright logs) and web-console/test-results/ instead of the distribution's logs, and the existing JUnit report step now shows each console test.

Modernized e2e specs

The specs were written in a Puppeteer style; they now use Playwright as intended:

• The @playwright/test runner (playwright.config.ts) instead of Jest: fixtures instead of per-spec browser setup, a globalSetup that waits for the cluster, expect.poll / toPass instead of a home-made retry, screenshots, traces and an error-context.md on failure.
• Locators (getByRole, form fields found by their label) instead of XPath and element handles.
• The cluster state (tasks finishing, segments loading, supervisor and Dart query states) is checked through SQL on the system tables rather than by reading view columns by position; views are read by their column headers.
• Test data is set up through the HTTP API rather than by shelling out to post-index-task.
• Page objects are plain functions and types (no Object.assign data classes), with loadData(page, config) for the data loader.
• E2E_SHOW_STEPS=true saves a screenshot of each step a test takes to e2e-tests/steps/<spec>-NNN.png, a quick way to see what a test does.
• web-console/e2e-tests/README.md describes the tests, how to run them both ways, and how to write one.

Running them on embedded clusters also surfaced three latent races in the specs that the quickstart's timing had hidden (reading a view's table before it loaded, cancelling a query before it was sent, reading the Query view's previous result), now fixed.

New console coverage

Spec	Run by	What it covers
s3-ingestion.spec.ts	s3.S3WebConsoleTest	Data loader from S3 (an S3 container that is also deep storage)
input-formats.spec.ts	indexer.InputFormatsWebConsoleTest	Data loader with CSV, TSV, Parquet, ORC and Avro OCF files: the format it picks and the columns it parses
kafka-ingestion.spec.ts	indexing.KafkaWebConsoleTest	A Kafka supervisor through the data loader (a Kafka container), then suspend, resume and terminate from the Supervisors view
sql-ingestion.spec.ts	msq.SqlIngestionWebConsoleTest	The SQL data loader, and REPLACE from external data and from a datasource in the Query view
datasource-actions.spec.ts	indexing.DatasourceActionsWebConsoleTest	Mark a datasource's segments unused and used again, then delete them with a kill task
retention-rules.spec.ts	server.RetentionRulesWebConsoleTest	Editing retention rules (loadForever, then dropForever)
jdbc-lookup.spec.ts	lookup.JdbcLookupWebConsoleTest	Initializing lookups and adding a JDBC lookup (on a table in the Derby metadata store), then querying it
dart.spec.ts	msq.DartWebConsoleTest	Running a query with the Dart engine, the current Dart queries panel and its details, cancelling a running Dart query

The existing specs (tutorial-batch, reindexing, auto-compaction, multi-stage-query, cancel-query) run from indexing.BatchIndexingWebConsoleTest, compact.AutoCompactionWebConsoleTest, msq.MultiStageQueryWebConsoleTest and query.SqlQueryCancelWebConsoleTest.

Web console fixes found on the way

• Table filters in the URL (#datasources/datasource=...) couldn't hold values with # or ? (which end the hash route) or | (which separates values), so going from a datasource with one of those in its name to its tasks or segments filtered on the wrong thing. #, ?, &, / and % are now encoded, and \| / \\ escape | / \ in a value.
• Api.encodePath left ^ and | as is, which Jetty rejects (400 Illegal Path Character), so every action on a datasource with one of those in its name (mark unused, kill, retention rules, compaction config) failed.

Release note

Fixed the web console's handling of datasource names containing #, ?, |, ^ or \: links between views now filter on the right datasource, and actions such as marking segments unused, killing data, and editing retention rules or compaction configs no longer fail for such names.

---

Key changed/added classes in this PR

• WebConsoleTestBase, WebConsoleSpecsTest and the *WebConsoleTest classes (embedded-tests)
• embedded-tests/pom.xml (web-console-tests profile), .github/scripts/web-checks.sh, .github/workflows/worker.yml
• web-console/e2e-tests/**, web-console/playwright.config.ts
• TableFilter, TableFilters, Api.encodePath (web console)

---

This PR has:

• been self-reviewed.
• added documentation for new or modified features or behaviors (web-console/e2e-tests/README.md).
• a release note entry in the PR description.
• added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
• added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
• added unit tests or modified existing tests to cover new code paths (table-filter.spec.ts, table-filters.spec.ts, hash-routing.spec.ts, api.spec.ts).
• added integration tests.

[FrankChen021]

🟡 Changes recommended

The current head cannot pass the new SQL ingestion E2E because the helper searches for a button label the application does not render. Additional issues can produce stale query assertions, indefinite E2E hangs, and incorrect filtering for datasource names containing |.

Reviewed 79 of 79 changed files.

Severity	Findings
P0	0
P1	2
P2	2
P3	0
Total	4

---

This is an automated review by Codex GPT-5.6-Luna(max)

After addressing the findings or replying to the comments, you can request another review from me to trigger a new automated review.

[FrankChen021]

[P1] Use the SQL loader's rendered submit label

Finding: The schema step renders the exact button name Start ingesting data, but this helper still searches for Start loading data. Because clickButton uses an exact accessible-name match, sql-ingestion.spec.ts times out before starting ingestion, so SqlIngestionWebConsoleTest cannot pass at this head.

Suggestion: Update the helper to the rendered label, or revert the temporary UI rename before merging.

[vogievetsky]

That commit was a deliberate, temporary failure to see how an e2e failure shows in CI; it's reverted in a20cb85.

[FrankChen021]

[P1] Wait for the current ingestion result

Finding: The workbench keeps the previous successful execution visible while a new query loads, so done.or(error).waitFor() can resolve on the old .ingest-success-pane. The second runIngestQuery in sql-ingestion.spec.ts can therefore return the first query's text before the new task finishes.

Suggestion: Wait for the current execution to complete or verify a query-specific state transition before reading the result pane.

[vogievetsky]

Right, the view can show the last output while the next query runs; fixed in 09872dc by clearing the output before each query.

[FrankChen021]

[P2] Enforce the spec timeout while consuming output

Finding: The runner drains process.getInputStream() to EOF before calling waitFor with SPEC_TIMEOUT_MINUTES. A hung Playwright process can keep stdout open, so the timeout branch is never reached and CI can hang until an external timeout.

Suggestion: Consume output asynchronously while waiting, then destroy the process and join or close the reader when the timeout expires.

[vogievetsky]

Fixed in d8a3312: the output is read on its own thread, and a timed-out spec's process tree is killed.

[FrankChen021]

[P2] Preserve literal pipes in datasource filters

Finding: String values are now parsed with splitNeedle, where an unescaped | means OR. Existing datasource links in segments-view.tsx and datasources-view.tsx still pass datasource names as strings, so a valid datasource named a|b becomes two filter values instead of one literal datasource.

Suggestion: Use the constructor's literal-value form for those datasource links and add a pipe-containing datasource regression case.

[vogievetsky]

Good catch: fixed in 6be4e1f; the segment timeline's two links now pass the datasource as a literal value.

[FrankChen021]

🟡 Changes recommended

This follow-up review covers the current head after rechecking all four prior review threads. Those prior findings appear resolved at the current head.

One remaining P2 should be addressed before merging: the new literal table-filter serialization is still passed as a datasource identifier to the Datasources and Segments segment-timeline views. A datasource containing | or \\ therefore cannot be loaded correctly in the timeline.

Reviewed all 80 changed files in the full diff, the 5 files in the incremental diff since the previous review, and the relevant surrounding call sites.

Validation: the exact GitHub base/head mergeability check and git diff --check passed. No builds or tests were run; this was a static review.

Severity	Findings
P0	0
P1	0
P2	1
P3	0
Total	1

---

This is an automated review by Codex GPT-5.6 Luna(Max)

After addressing the findings or replying to the comments, you can request another review from me to trigger a new automated review.

[FrankChen021]

[P2] Keep literal datasource names raw for timeline consumers

Finding: The new getter serializes literal values for the table-filter grammar (a|b becomes a\|b). However, the existing Datasources and Segments views still pass filter.value directly to SegmentTimeline as a datasource name. With the new literal representation, a datasource containing | or \ is sent to the timeline with its escape characters, so the SQL/API timeline query cannot find the real datasource.

Suggestion: Use the raw single value (filter.values[0]) at those timeline call sites, or expose separate raw-value and serialized-needle accessors so consumers do not treat the escaped grammar value as a datasource identifier.

[vogievetsky]

Good catch, fixed in e7bc667: the segment timeline now gets the filter's raw value (values[0]), only when there's exactly one.

[FrankChen021]

🟡 Changes recommended

Reviewed the full 84-file merge-base diff for apache/druid#20544. I rechecked the prior review thread: the datasource and segments timeline issue is resolved at e7bc667 by passing the raw datasource filter value when there is exactly one literal datasource filter.

Coverage included the CI and Maven integration, the embedded JUnit harness, all new WebConsoleTest classes, the Playwright specs and shared utilities/page objects, table-filter/API serialization changes, and the processing async-resource changes plus production consumers. The remaining issue is the removal of fail-fast completion from CollectAsyncResource, which regresses current master behavior for failed asynchronous downloads.

Validation: git diff --check 165e9b9 e7bc667 passed. No test suite was run; this review used static inspection of the prepared checkout.

Severity	Findings
P0	0
P1	1
P2	0
P3	0
Total	1

Findings that could not be attached inline:

• processing/src/main/java/org/apache/druid/common/asyncresource/CollectAsyncResource.java:83 - [P1] Preserve fail-fast completion for collected resources

  Finding: This change removes the fail-fast path that is present on current master: each source callback now only increments readyCount, so if one source fails while another remains pending, the collected resource stays pending and its consumer does not observe the failure until every source completes. PartialQueryableIndexCursorFactory relies on the collected resource callback to call failHolder and release the query's download state; a failed deep-storage range can therefore leave the query waiting until timeout and keep other downloads running.

  Suggestion: Retain the failed-source get()/setException path, guarded so only the first failure completes the target, and keep a regression test that verifies a failed source completes the collection without waiting for pending sources.

---

This is an automated review by Codex GPT-5.6 Luna(Max)

After addressing the findings or replying to the comments, you can request another review from me to trigger a new automated review.

[vogievetsky]

@FrankChen021 re the CollectAsyncResource P1 from the latest review: this PR never touched processing/. #20504 landed on master after this branch's base, so comparing master's tip against the PR head made the fix look removed. I've merged master into the branch (5154498), so the diff no longer touches processing/ and master's CollectAsyncResource is unchanged.

The CodeQL resource-leak alert on KafkaWebConsoleTest was already handled in the current code: the FileInputStream is its own try-with-resources resource. With that, I believe all the review findings are resolved.

[FrankChen021]

🟢 Approval recommended

The current head is functionally ready to merge. One non-blocking documentation issue remains: the embedded-cluster table mixes runner mappings with long test descriptions under the wrong column.

Reviewed 80 of 80 changed files; one non-blocking finding remains.

Validation: git diff --check 4c4ad2c376fc66fa5d9de200caa663f551919f8e...HEAD passed. Tests, build, typecheck, and formatter were not run; the prepared worktree has no node_modules.

Severity	Findings
P0	0
P1	0
P2	0
P3	1
Total	1

---

This is an automated review by Codex GPT-5.6-Luna(max)

After addressing the findings or replying to the comments, you can request another review from me to trigger a new automated review.

[FrankChen021]

[P3] Keep the embedded-test table columns consistent

Finding: The table beginning at line 66 declares its second column as Run by, but lines 80-86 append long What it does descriptions as additional rows without a new header. These rows render in the wrong column and duplicate several spec entries, making the embedded-test mapping misleading.

Suggestion: Move the descriptions under a separate Spec | What it does table or remove the duplicate block so the embedded-cluster table contains only runner mappings.