vatsrahul1001 commented on code in PR #59990:
URL: https://github.com/apache/airflow/pull/59990#discussion_r2657445464
##########
airflow-core/src/airflow/ui/tests/e2e/specs/asset.spec.ts:
##########
@@ -0,0 +1,44 @@
+import { test, expect } from "@playwright/test";
Review Comment:
Missing Apache License Headers
##########
airflow-core/src/airflow/ui/tests/e2e/pages/AssetListPage.ts:
##########
@@ -0,0 +1,53 @@
+import type { Locator, Page } from "@playwright/test";
Review Comment:
Missing Apache License Headers
##########
airflow-core/src/airflow/ui/tests/e2e/pages/AssetListPage.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 { Locator, Page } from "@playwright/test";
+
+import { BasePage } from "./BasePage";
+
+export class AssetListPage extends BasePage {
+ public readonly emptyState: Locator;
+ public readonly heading: Locator;
+ public readonly nextButton: Locator;
+ public readonly prevButton: Locator;
+ public readonly rows: Locator;
+ public readonly searchInput: Locator;
+ public readonly table: Locator;
+
+ public constructor(page: Page) {
+ super(page);
+
+ this.heading = page.getByRole("heading", { level: 2 });
Review Comment:
This is Fragile
We can try
`this.heading = page.getByRole("heading", { name: /assets/i });`
##########
airflow-core/src/airflow/ui/tests/e2e/specs/asset.spec.ts:
##########
@@ -0,0 +1,78 @@
+/*!
+ * 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 { test, expect } from "@playwright/test";
+
+import { AssetListPage } from "../pages/AssetListPage";
+
+test.describe("Assets Page", () => {
+ let assets: AssetListPage;
+
+ test.beforeEach(async ({ page }) => {
+ assets = new AssetListPage(page);
+ await assets.navigate();
+ await assets.waitForLoad();
+ });
+
+ test("renders assets page heading", async () => {
+ await expect(assets.heading).toBeVisible();
+ });
+
+ test("renders assets table", async () => {
+ await expect(assets.table).toBeVisible();
+ });
+
+ test("shows asset rows when data exists", async () => {
+ const count = await assets.assetCount();
+
+ expect(count).toBeGreaterThanOrEqual(0);
+ });
+
+ test("each asset has a visible name link", async () => {
+ const names = await assets.assetNames();
+
+ for (const name of names) {
+ expect(name.trim().length).toBeGreaterThan(0);
+ }
+ });
+
+ test("filters assets using search", async () => {
Review Comment:
Current test is not testing filtering correctly. You can try below
```
test("verify search filters assets", async () => {
const initialCount = await assets.assetCount();
expect(initialCount).toBeGreaterThan(1);
await assets.search("dag1"); // dag1 is just example, use proper asset
name which filters
await expect.poll(() => assets.assetCount(), { timeout: 20_000
}).toBeLessThan(initialCount);
const names = await assets.assetNames();
expect(names.length).toBeGreaterThan(0);
for (const name of names) {
expect(name.toLowerCase()).toContain("dag1");
}
});
```
##########
airflow-core/src/airflow/ui/tests/e2e/specs/asset.spec.ts:
##########
@@ -0,0 +1,78 @@
+/*!
+ * 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 { test, expect } from "@playwright/test";
+
+import { AssetListPage } from "../pages/AssetListPage";
+
+test.describe("Assets Page", () => {
+ let assets: AssetListPage;
+
+ test.beforeEach(async ({ page }) => {
+ assets = new AssetListPage(page);
+ await assets.navigate();
+ await assets.waitForLoad();
+ });
+
+ test("renders assets page heading", async () => {
+ await expect(assets.heading).toBeVisible();
+ });
+
+ test("renders assets table", async () => {
+ await expect(assets.table).toBeVisible();
+ });
+
+ test("shows asset rows when data exists", async () => {
+ const count = await assets.assetCount();
+
+ expect(count).toBeGreaterThanOrEqual(0);
+ });
+
+ test("each asset has a visible name link", async () => {
+ const names = await assets.assetNames();
+
+ for (const name of names) {
+ expect(name.trim().length).toBeGreaterThan(0);
+ }
+ });
+
+ test("filters assets using search", async () => {
+ const initialCount = await assets.assetCount();
+
+ await assets.search("dag");
+ await expect.poll(() => assets.assetCount(), { timeout: 20_000
}).toBeLessThanOrEqual(initialCount);
+
+ if ((await assets.assetCount()) === 0) {
+ await expect(assets.emptyState).toBeVisible();
+ }
+ });
+
+ test("supports pagination when multiple pages exist", async () => {
Review Comment:
You can try something like below
test("verify pagination controls navigate between pages", async () => {
// Force pagination with small limit
await assets.navigateTo("/assets?limit=5&offset=0");
await assets.waitForLoad();
const page1Assets = await assets.getAssetNames();
expect(page1Assets.length).toBeGreaterThan(0);
await assets.page.getByRole("button", { name: "2" }).click();
await assets.page.waitForURL(/offset=5/);
await assets.waitForLoad();
const page2Assets = await assets.getAssetNames();
expect(page2Assets.length).toBeGreaterThan(0);
expect(page2Assets).not.toEqual(page1Assets);
await assets.page.getByRole("button", { name: "1" }).click();
await assets.page.waitForURL(/offset=0|(?!.*offset)/);
await assets.waitForLoad();
const backToPage1Assets = await assets.getAssetNames();
expect(backToPage1Assets).toEqual(page1Assets);
});
##########
airflow-core/src/airflow/ui/tests/e2e/pages/AssetListPage.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 { Locator, Page } from "@playwright/test";
+
+import { BasePage } from "./BasePage";
+
+export class AssetListPage extends BasePage {
+ public readonly emptyState: Locator;
+ public readonly heading: Locator;
+ public readonly nextButton: Locator;
+ public readonly prevButton: Locator;
+ public readonly rows: Locator;
+ public readonly searchInput: Locator;
+ public readonly table: Locator;
+
+ public constructor(page: Page) {
+ super(page);
+
+ this.heading = page.getByRole("heading", { level: 2 });
+ this.table = page.getByTestId("table-list");
+ this.rows = this.table.locator("tbody tr").filter({
+ has: page.locator("td"),
+ });
+
+ this.nextButton = page.getByTestId("next");
+ this.prevButton = page.getByTestId("prev");
+
+ this.searchInput = page.getByRole("textbox");
Review Comment:
We can use `this.searchInput = page.getByTestId("search-dags");`
##########
airflow-core/src/airflow/ui/tests/e2e/specs/asset.spec.ts:
##########
@@ -0,0 +1,78 @@
+/*!
+ * 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 { test, expect } from "@playwright/test";
+
+import { AssetListPage } from "../pages/AssetListPage";
+
+test.describe("Assets Page", () => {
+ let assets: AssetListPage;
+
+ test.beforeEach(async ({ page }) => {
+ assets = new AssetListPage(page);
+ await assets.navigate();
+ await assets.waitForLoad();
+ });
+
+ test("renders assets page heading", async () => {
+ await expect(assets.heading).toBeVisible();
+ });
+
+ test("renders assets table", async () => {
+ await expect(assets.table).toBeVisible();
+ });
+
+ test("shows asset rows when data exists", async () => {
+ const count = await assets.assetCount();
+
+ expect(count).toBeGreaterThanOrEqual(0);
+ });
+
+ test("each asset has a visible name link", async () => {
+ const names = await assets.assetNames();
+
+ for (const name of names) {
+ expect(name.trim().length).toBeGreaterThan(0);
+ }
+ });
+
+ test("filters assets using search", async () => {
+ const initialCount = await assets.assetCount();
+
+ await assets.search("dag");
+ await expect.poll(() => assets.assetCount(), { timeout: 20_000
}).toBeLessThanOrEqual(initialCount);
+
+ if ((await assets.assetCount()) === 0) {
+ await expect(assets.emptyState).toBeVisible();
+ }
+ });
+
+ test("supports pagination when multiple pages exist", async () => {
Review Comment:
Also we need to add below to AssetListPage
```
async getAssetNames(): Promise<string[]> {
const names: string[] = [];
const count = await this.rows.count();
for (let i = 0; i < count; i++) {
const text = await this.rows.nth(i).locator("a").first().textContent();
if (text) names.push(text.trim());
}
return names;
}
async clickPageNumber(pageNum: number): Promise<void> {
await this.page.getByRole("button", { name: String(pageNum) }).click();
}
```
--
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]
