FrankChen021 commented on code in PR #20544:
URL: https://github.com/apache/druid/pull/20544#discussion_r4237633384
##########
web-console/e2e-tests/component/workbench/overview.ts:
##########
@@ -16,43 +16,181 @@
* limitations under the License.
*/
-import type * as playwright from 'playwright-chromium';
+import type { Locator, Page, Request } from '@playwright/test';
-import { clickButton, setQueryInput } from '../../util/playwright';
+import { expect } from '../../util/fixtures';
+import { clickButton, clickMenuItem, openView, setQueryInput } from
'../../util/playwright';
+import { showStep } from '../../util/steps';
import { extractTable } from '../../util/table';
/**
- * Represents the workbench tab.
+ * How to run a query.
+ */
+export interface QueryOptions {
+ /** The engine to pick in the view's engine menu, by its name there (like
"SQL (Dart)"), rather than "Auto" */
+ readonly engine?: string;
+}
+
+/**
+ * What the view sent to run a query.
+ */
+export interface SubmittedQuery {
+ /** The engine of the query's context, which the view sets for the engine
picked (like "msq-dart") */
+ readonly engine?: string;
+ /** The ID that the view gives the query (SQL queries, natively or with
Dart), to cancel it */
+ readonly sqlQueryId?: string;
+}
+
+/**
+ * Represents the Query view (the workbench).
*/
export class WorkbenchOverview {
- private readonly page: playwright.Page;
- private readonly baseUrl: string;
+ private readonly page: Page;
+
+ /** What the view sent to run the last query run (or started) */
+ lastSubmittedQuery: SubmittedQuery | undefined;
- constructor(page: playwright.Page, unifiedConsoleUrl: string) {
+ constructor(page: Page) {
this.page = page;
- this.baseUrl = unifiedConsoleUrl + '#workbench';
}
- async runQuery(query: string): Promise<string[][]> {
- await this.page.goto(this.baseUrl);
- await this.page.reload({ waitUntil: 'networkidle' });
+ /**
+ * Runs a query and returns its results (as text, one array of cells per
row). Throws if the query fails.
+ */
+ async runQuery(query: string, options: QueryOptions = {}):
Promise<string[][]> {
+ const results = this.page.locator('.result-table-pane');
+ await this.run(query, options, results);
+ await showStep(this.page, 'Query view: the query results');
+ return await extractTable(results);
+ }
+
+ /**
+ * Runs an ingestion query (INSERT or REPLACE, run as an MSQ task) and
returns what the view says when it is done
+ * (like "39,244 rows inserted into ..."). Throws if the query fails.
+ */
+ async runIngestQuery(query: string, options: QueryOptions = {}):
Promise<string> {
+ const ingestSuccess = this.page.locator('.ingest-success-pane');
+ await this.run(query, options, ingestSuccess);
+ await showStep(this.page, 'Query view: the data ingested');
+ return await ingestSuccess.innerText();
+ }
+
+ /**
+ * Starts a query, without waiting for it to finish, and returns what the
view sent to run it.
+ */
+ async startQuery(query: string, options: QueryOptions = {}):
Promise<SubmittedQuery> {
+ await this.submit(query, options);
+ await showStep(this.page, 'Query view: the query running');
+ return this.lastSubmittedQuery!;
+ }
+
+ private async run(query: string, options: QueryOptions, done: Locator):
Promise<void> {
+ await this.submit(query, options);
+
+ const error = this.page.locator('.execution-error-pane');
+ await done.or(error).waitFor({ timeout: QUERY_TIMEOUT });
Review Comment:
[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.
##########
web-console/e2e-tests/component/load-data/sql-data-loader.ts:
##########
@@ -0,0 +1,87 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import type { Page } from '@playwright/test';
+
+import { clickButton, getLabeledInput, openView, setLabeledInput } from
'../../util/playwright';
+import { showStep } from '../../util/steps';
+import { waitForTaskSlots } from '../workbench/overview';
+
+import type { DataConnector } from './data-connector';
+import { connectorCardTitle, fillConnector } from './data-connector';
+
+/**
+ * What to set at each step of the SQL data loader ("Load data" > "Batch -
SQL"), which ingests with an MSQ task.
+ */
+export interface SqlDataLoaderConfig {
+ readonly connector: DataConnector;
+ /** Checks the input format that the data loader picked and the columns it
parsed */
+ readonly validateParseData?: (parsed: { inputFormat: string; columns:
string[] }) => void;
+ /** The new datasource to create */
+ readonly datasourceName: string;
+}
+
+/**
+ * Goes through each step of the SQL data loader, starts the ingestion and
waits for it to finish. Throws if it fails.
+ */
+export async function loadDataWithSql(page: Page, config:
SqlDataLoaderConfig): Promise<void> {
+ await openView(page, 'sql-data-loader');
+
+ // Select input type
+ await page
+ .locator('.input-source-step .bp6-card')
+ .filter({ has: page.locator('p', { hasText:
connectorCardTitle(config.connector) }) })
+ .click();
+ await fillConnector(page, config.connector);
+ await showStep(page, `SQL data loader:
${connectorCardTitle(config.connector)}`);
+ await clickButton(page, 'Connect data');
+
+ // Parse
+ const parseDataTable = page.locator('.input-format-step .parse-data-table');
+ await parseDataTable.locator('.column-name').first().waitFor();
+ if (config.validateParseData) {
+ config.validateParseData({
+ inputFormat: await getLabeledInput(page, 'Input format'),
+ columns: await parseDataTable.locator('.column-name').allTextContents(),
+ });
+ }
+ await showStep(page, 'SQL data loader: Parse');
+ await clickButton(page.locator('.input-format-step .prev-next-bar'), 'Next');
+
+ // Schema, with the destination (a new datasource)
+ const schemaStep = page.locator('.schema-step');
+ await schemaStep.locator('.destination-button').click();
+ const destinationDialog = page.locator('.destination-dialog');
+ await clickButton(destinationDialog, 'New table');
+ await setLabeledInput(destinationDialog, 'New table name',
config.datasourceName);
+ await showStep(page, 'SQL data loader: Destination');
+ await clickButton(destinationDialog, 'Save');
+ await destinationDialog.waitFor({ state: 'detached' });
+ await showStep(page, 'SQL data loader: Schema');
+ await clickButton(schemaStep.locator('.prev-next-bar'), 'Start loading
data');
Review Comment:
[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.
##########
web-console/src/utils/table-filters/table-filter.ts:
##########
@@ -64,14 +96,18 @@ export class TableFilter {
return new TableFilter(filter.id, mode, value);
}
+ /**
+ * @param value the values to filter on, or a needle (as typed in a filter
input) of values separated by `|` (see
+ * `splitNeedle`). To filter on one literal value, that may contain a `|`,
pass it as `[value]`.
+ */
constructor(key: string, mode: FilterMode, value: string | string[]) {
this.key = key;
this.mode = mode;
- this.values = typeof value === 'string' ? value.split('|') : value;
+ this.values = typeof value === 'string' ? TableFilter.splitNeedle(value) :
value;
Review Comment:
[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.
##########
embedded-tests/src/test/java/org/apache/druid/testing/embedded/console/WebConsoleTestBase.java:
##########
@@ -0,0 +1,231 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.testing.embedded.console;
+
+import org.apache.druid.guice.SleepModule;
+import org.apache.druid.java.util.common.StringUtils;
+import org.apache.druid.java.util.common.logger.Logger;
+import org.apache.druid.query.aggregation.datasketches.hll.HllSketchModule;
+import
org.apache.druid.query.aggregation.datasketches.quantiles.DoublesSketchModule;
+import org.apache.druid.query.aggregation.datasketches.theta.SketchModule;
+import org.apache.druid.testing.embedded.EmbeddedBroker;
+import org.apache.druid.testing.embedded.EmbeddedCoordinator;
+import org.apache.druid.testing.embedded.EmbeddedDruidCluster;
+import org.apache.druid.testing.embedded.EmbeddedHistorical;
+import org.apache.druid.testing.embedded.EmbeddedIndexer;
+import org.apache.druid.testing.embedded.EmbeddedOverlord;
+import org.apache.druid.testing.embedded.EmbeddedRouter;
+import org.apache.druid.testing.embedded.junit5.EmbeddedClusterTestBase;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Tag;
+
+import java.io.BufferedReader;
+import java.io.File;
+import java.io.IOException;
+import java.io.InputStreamReader;
+import java.nio.charset.StandardCharsets;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.TimeUnit;
+import java.util.regex.Pattern;
+import java.util.stream.Collectors;
+
+/**
+ * Base class for the web console end-to-end tests: a cluster with a Router
(which serves the web console), on which
+ * the tests run the Playwright specs of {@code web-console/e2e-tests}.
+ * <p>
+ * Steps to write a test:
+ * <ul>
+ * <li>Write the Playwright spec in {@code web-console/e2e-tests}.</li>
+ * <li>Write a {@code *WebConsoleTest} class that extends this class, in the
package of the functionality the spec
+ * covers (like {@code s3} for loading data from S3). Add the resources the
spec needs (S3, Kafka, etc.) in
+ * {@link #addResources}, and the extensions and properties in {@link
#configureCluster}.</li>
+ * <li>Write a {@code @Test} method that calls {@link #runSpec}, passing the
settings of the resources to the spec
+ * through environment variables.</li>
+ * </ul>
+ * The tests run only with the {@code web-console-tests} profile, all of them
or one class:
+ * <pre>
+ * mvn -pl embedded-tests verify -Pweb-console-tests
+ * mvn -pl embedded-tests verify -Pweb-console-tests
-Dit.test=BatchIndexingWebConsoleTest
+ * </pre>
+ * With {@code -Dweb.console.keepAlive=true}, {@link #runSpec} doesn't run the
spec. It prints the command to run it
+ * and waits, keeping the cluster up, so that the spec can be run (and
debugged) from {@code web-console} against it.
+ * With {@code -Dweb.console.port}, the specs run against the console on that
port (such as the dev server of
+ * {@code npm start}, which proxies to the Router on port 8888) rather than
the console that the Router serves.
+ */
+@Tag("web-console")
+public abstract class WebConsoleTestBase extends EmbeddedClusterTestBase
+{
+ private static final Logger log = new Logger(WebConsoleTestBase.class);
+
+ private static final long SPEC_TIMEOUT_MINUTES = 15;
+ private static final Pattern NOT_FILE_NAME_CHARS =
Pattern.compile("[^A-Za-z0-9.-]");
+
+ protected final EmbeddedCoordinator coordinator = new EmbeddedCoordinator();
+ protected final EmbeddedOverlord overlord = new EmbeddedOverlord();
+ protected final EmbeddedIndexer indexer = new EmbeddedIndexer();
+ protected final EmbeddedHistorical historical = new EmbeddedHistorical();
+ protected final EmbeddedBroker broker = new EmbeddedBroker();
+ protected final EmbeddedRouter router = new EmbeddedRouter();
+
+ @Override
+ protected final EmbeddedDruidCluster createCluster()
+ {
+ final EmbeddedDruidCluster cluster =
EmbeddedDruidCluster.withEmbeddedDerbyAndZookeeper();
+ // Resources start in the order they are added, and must start before the
Druid servers that use them
+ addResources(cluster);
+
+ // SleepModule has the sleep() function, for queries that run long enough
to be canceled
+ cluster.addExtensions(SketchModule.class, HllSketchModule.class,
DoublesSketchModule.class, SleepModule.class)
+ .addCommonProperty("druid.msq.dart.enabled", "true")
+ // Keep the reports of finished Dart queries, for the console to
show them (as script/druid build does)
+
.addCommonProperty("druid.msq.dart.controller.maxRetainedReportCount", "100")
+
.addCommonProperty("druid.msq.dart.controller.maxRetainedReportDuration",
"PT3600S")
+ .addCommonProperty("druid.sql.planner.enableSysQueriesTable",
"true")
+ // The console waits for the cluster state to change, so make it
change quickly
+ .addCommonProperty("druid.manager.segments.pollDuration", "PT1S")
+ .addServer(coordinator.addProperty("druid.coordinator.period",
"PT1S"))
+ .addServer(overlord)
+ // Enough task slots for an MSQ query (a controller and a worker)
next to an ingestion task
+ .addServer(indexer.addProperty("druid.worker.capacity",
"4").setServerMemory(1_000_000_000L))
+ .addServer(historical)
+ .addServer(broker)
+ .addServer(router);
+
+ configureCluster(cluster);
+ return cluster;
+ }
+
+ /**
+ * Adds the resources (such as an S3 container) that the specs of this test
need.
+ */
+ protected void addResources(EmbeddedDruidCluster cluster)
+ {
+ }
+
+ /**
+ * Adds the extensions and properties that the specs of this test need.
+ */
+ protected void configureCluster(EmbeddedDruidCluster cluster)
+ {
+ }
+
+ /**
+ * Runs the Playwright spec (a file name or a pattern of {@code
web-console/e2e-tests}) against this cluster and
+ * fails if it fails.
+ */
+ protected void runSpec(String spec) throws Exception
+ {
+ runSpec(spec, Map.of());
+ }
+
+ /**
+ * Runs the Playwright spec (a file name or a pattern of {@code
web-console/e2e-tests}) against this cluster and
+ * fails if it fails.
+ *
+ * @param env environment variables to pass to the spec, such as the
settings of the resources
+ */
+ protected void runSpec(String spec, Map<String, String> env) throws Exception
+ {
+ final File webConsoleDir = webConsoleDir();
+ final Map<String, String> specEnv = new HashMap<>(env);
+ specEnv.put(
+ "DRUID_E2E_TEST_UNIFIED_CONSOLE_PORT",
+ System.getProperty("web.console.port",
String.valueOf(router.bindings().selfNode().getPlaintextPort()))
+ );
+ // Each spec has its own output directory, as Playwright empties it on
every run
+ final String outputDir = "test-results/" +
NOT_FILE_NAME_CHARS.matcher(spec).replaceAll("_");
+ final List<String> command = List.of("npx", "playwright", "test", spec,
"--output", outputDir);
+
+ if (Boolean.getBoolean("web.console.keepAlive")) {
+ keepAlive(webConsoleDir, specEnv, command);
+ return;
+ }
+
+ // The cluster is thrown away after this class, so the specs don't need to
delete what they create. (A kept-alive
+ // cluster is reused for runs of the specs by hand, so they do.)
+ specEnv.put("DRUID_E2E_TEST_CLUSTER_IS_DISPOSABLE", "true");
+
+ log.info("Running spec[%s] in [%s] with env[%s].", spec, webConsoleDir,
specEnv);
+ final ProcessBuilder processBuilder = new
ProcessBuilder(command).directory(webConsoleDir)
+
.redirectErrorStream(true);
+ processBuilder.environment().putAll(specEnv);
+ final Process process = processBuilder.start();
+
+ // Log the output as it comes (so that a hung spec can be seen) and keep
it for the failure message
+ final List<String> output = new ArrayList<>();
+ try (BufferedReader reader = new BufferedReader(
+ new InputStreamReader(process.getInputStream(), StandardCharsets.UTF_8)
+ )) {
+ String line;
+ while ((line = reader.readLine()) != null) {
Review Comment:
[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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
