Skip to content

Commit 669e641

Browse files
nopcodernopcoder
committed
webui: simplify auth file handling and remove unused API methods
1 parent 3f65808 commit 669e641

13 files changed

+141
-128
lines changed

‎webui/.gitignore‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ node_modules
33
dist-ssr
44
*.local
55
/test-results/
6+
test/playwright/*.json
67

78
# Ignore built UI but keep .gitkeep
89
dist/*

‎webui/playwright.config.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import { defineConfig, devices } from "@playwright/test";
44
* https://github.com/motdotla/dotenv
55
*/
66
import "dotenv/config";
7-
import { COMMON_STORAGE_STATE_PATH } from "./test/e2e/consts";
7+
import { STORAGE_STATE_PATH } from "./test/e2e/credentialsFile";
88

99
const BASE_URL = process.env.BASE_URL || "http://localhost:8000";
1010
const SKIP_SETUP = !!process.env.SKIP_SETUP;
@@ -50,7 +50,7 @@ export default defineConfig({
5050
dependencies: SKIP_SETUP ? [] : ["common-setup"],
5151
use: {
5252
...devices["Desktop Chrome"],
53-
storageState: COMMON_STORAGE_STATE_PATH,
53+
storageState: STORAGE_STATE_PATH,
5454
},
5555
testMatch: "test/e2e/common/**/*.spec.ts",
5656
testIgnore: [...SETUP_SPECS, "test/e2e/common/quickstart.spec.ts"],
@@ -60,7 +60,7 @@ export default defineConfig({
6060
dependencies: SKIP_SETUP ? [] : ["common-setup"],
6161
use: {
6262
...devices["Desktop Chrome"],
63-
storageState: COMMON_STORAGE_STATE_PATH,
63+
storageState: STORAGE_STATE_PATH,
6464
},
6565
testMatch: "test/e2e/common/**/quickstart.spec.ts",
6666
testIgnore: SETUP_SPECS,

‎webui/test/README.md‎

Lines changed: 102 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ npx playwright test
2828

2929
### Skip setup (already-configured lakeFS)
3030

31-
If lakeFS is already set up and credentials are saved at `test/playwright/.auth/`:
31+
If lakeFS is already set up and credentials are saved at `test/playwright/`:
3232

3333
```bash
3434
SKIP_SETUP=true npx playwright test
@@ -55,12 +55,6 @@ Run tests with the browser visible:
5555
npx playwright test --headed
5656
```
5757

58-
Slow down execution to watch interactions:
59-
60-
```bash
61-
npx playwright test --headed --timeout=0
62-
```
63-
6458
## Debugging
6559

6660
### Playwright Inspector (step-through debugger)
@@ -117,9 +111,8 @@ test/e2e/
117111
├── fixtures.ts # Custom Playwright fixtures (POMs, API helper)
118112
├── lakeFSApi.ts # lakeFS REST API wrapper for test setup
119113
├── timeouts.ts # Named timeout constants
120-
├── credentialsFile.ts # Read/write saved credentials
114+
├── credentialsFile.ts # Read/write saved credentials, auth file paths
121115
├── types.ts # Shared TypeScript types
122-
├── consts.ts # File path constants
123116
├── poms/ # Page Object Models
124117
│ ├── repositoriesPage.ts # Repositories list page
125118
│ ├── repositoryPage.ts # Single repository (facade)
@@ -144,6 +137,106 @@ test/e2e/
144137
└── revertCommit.spec.ts # Revert commit tests
145138
```
146139

140+
## Design Principles
141+
142+
### Fixtures over manual construction
143+
144+
Tests receive POMs via [Playwright fixtures](https://playwright.dev/docs/test-fixtures) instead of constructing them manually. Import `test` and `expect` from `../fixtures`, not from `@playwright/test`:
145+
146+
```ts
147+
// Good
148+
import { test, expect } from "../fixtures";
149+
test("my test", async ({ repositoriesPage, repositoryPage }) => { ... });
150+
151+
// Bad — don't construct POMs manually in tests
152+
import { test } from "@playwright/test";
153+
import { RepositoriesPage } from "../poms/repositoriesPage";
154+
test("my test", async ({ page }) => {
155+
const repositoriesPage = new RepositoriesPage(page);
156+
});
157+
```
158+
159+
### Facade pattern for repository operations
160+
161+
`RepositoryPage` is a thin facade that composes sub-POMs by domain. Access operations through the appropriate sub-POM:
162+
163+
```ts
164+
repositoryPage.branches.createBranch("feature");
165+
repositoryPage.branches.switchBranch("feature");
166+
repositoryPage.objects.uploadFiles("file.txt");
167+
repositoryPage.changes.commitChanges("my commit");
168+
repositoryPage.changes.merge("merge msg");
169+
repositoryPage.revert.clickRevertButton();
170+
repositoryPage.commits.getCommitsCount();
171+
```
172+
173+
When adding new repository operations, add them to the relevant sub-POM (or create a new one) — don't add methods directly to `RepositoryPage`.
174+
175+
### Named timeouts
176+
177+
Use the constants from `timeouts.ts` instead of magic numbers:
178+
179+
```ts
180+
import { TIMEOUT_LONG_OPERATION, TIMEOUT_ELEMENT_VISIBLE, TIMEOUT_NAVIGATION } from "../timeouts";
181+
182+
await expect(element).toBeVisible({ timeout: TIMEOUT_ELEMENT_VISIBLE }); // 10s
183+
await page.waitForURL(/.*ref=.*/, { timeout: TIMEOUT_NAVIGATION }); // 5s
184+
await expect(result).toBeVisible({ timeout: TIMEOUT_LONG_OPERATION }); // 120s
185+
```
186+
187+
### Assertions, not boolean checks
188+
189+
Use Playwright's auto-retrying assertions. Never use `.isVisible()`, `.isEnabled()`, or `.isDisabled()` as assertions — they return a boolean instantly without waiting:
190+
191+
```ts
192+
// Good — auto-retries until timeout
193+
await expect(element).toBeVisible();
194+
await expect(button).toBeDisabled();
195+
196+
// Bad — returns true/false immediately, no retries
197+
await element.isVisible();
198+
```
199+
200+
Similarly, for counting elements, use `expect.poll()`:
201+
202+
```ts
203+
// Good — retries until count matches
204+
await expect.poll(() => locator.count()).toEqual(3);
205+
206+
// Bad — checks once, instantly
207+
expect(await locator.count()).toEqual(3);
208+
```
209+
210+
### REST API for test setup, UI for test assertions
211+
212+
Use `lakeFSApi` (via the fixture) to set up preconditions like creating repositories. Use the UI to verify behavior. This keeps tests fast and focused on what they're actually testing.
213+
214+
## Adding a New Test
215+
216+
1. Create a spec file in `common/` (e.g., `common/myFeature.spec.ts`).
217+
218+
2. Import from fixtures, not from `@playwright/test`:
219+
```ts
220+
import { test, expect } from "../fixtures";
221+
```
222+
223+
3. Destructure the fixtures you need:
224+
```ts
225+
test("should do something", async ({ repositoryPage, lakeFSApi }) => {
226+
// lakeFSApi for setup, repositoryPage for UI interaction
227+
});
228+
```
229+
230+
4. If you need new UI operations, add methods to the appropriate sub-POM under `poms/`. If the operations don't fit an existing sub-POM, create a new `*Operations.ts` file, add it as a property on `RepositoryPage`, and register it as a fixture in `fixtures.ts` if it needs to be used directly.
231+
232+
5. If you need new API operations, add methods to `LakeFSApi` in `lakeFSApi.ts`.
233+
234+
6. For features unsupported by the local block adapter, tag the test with `@exclude-local`:
235+
```ts
236+
test("import @exclude-local", async ({ repositoryPage }) => { ... });
237+
```
238+
These can be skipped with `npx playwright test --grep-invert @exclude-local`.
239+
147240
## Test Projects (playwright.config.ts)
148241

149242
| Project | Description | Dependencies |

‎webui/test/e2e/common/quickstart.spec.ts‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,7 @@ test.describe("Quickstart", () => {
8282
await expect(page.getByRole("button", { name: "Uncommitted Changes" })).toBeVisible({ timeout: TIMEOUT_ELEMENT_VISIBLE });
8383
await repositoryPage.changes.showOnlyChanges();
8484
await expect(page.getByText("Showing 1 change for branch")).toBeVisible();
85-
expect(await repositoryPage.changes.getUncommittedCount()).toEqual(1);
85+
await expect.poll(() => repositoryPage.changes.getUncommittedCount()).toEqual(1);
8686
await repositoryPage.changes.commitChanges("denmark");
8787
await expect(page.getByRole("button", { name: "Uncommitted Changes" })).toHaveCount(0);
8888
});
@@ -91,7 +91,7 @@ test.describe("Quickstart", () => {
9191
await repositoryPage.gotoCompareTab();
9292
await repositoryPage.branches.switchBaseBranch("main");
9393
await expect(page.getByText("Showing changes between")).toBeVisible();
94-
expect(await repositoryPage.changes.getUncommittedCount()).toEqual(1);
94+
await expect.poll(() => repositoryPage.changes.getUncommittedCount()).toEqual(1);
9595
await repositoryPage.changes.merge("merge commit");
9696
await expect(page.getByText("No changes")).toBeVisible();
9797
});
@@ -122,15 +122,15 @@ test.describe("Quickstart", () => {
122122
await test.step("commit changes", async () => {
123123
await repositoryPage.gotoObjectsTab();
124124
await repositoryPage.changes.showOnlyChanges();
125-
expect(await repositoryPage.changes.getUncommittedCount()).toEqual(1);
125+
await expect.poll(() => repositoryPage.changes.getUncommittedCount()).toEqual(1);
126126
await repositoryPage.changes.commitChanges("Commit for pull-1");
127127
await expect(page.getByRole("button", { name: "Uncommitted Changes" })).toHaveCount(0);
128128
});
129129

130130
await test.step("verify empty pull requests list", async () => {
131131
await repositoryPage.gotoPullRequestsTab();
132132
await expect(page.getByText("Create Pull Request")).toBeVisible();
133-
expect(await pullsPage.getPullsListCount()).toEqual(0);
133+
await expect.poll(() => pullsPage.getPullsListCount()).toEqual(0);
134134
});
135135

136136
const pullDetails = { title: "PR for branch 1", description: "A description for PR 1" };
@@ -173,7 +173,8 @@ test.describe("Quickstart", () => {
173173
}
174174

175175
await validateRow("Repository name", QUICKSTART_REPO_NAME);
176-
await validateRow("Storage namespace", "local://" + QUICKSTART_REPO_NAME);
176+
const storagePrefix = process.env.REPO_STORAGE_NAMESPACE_PREFIX || "local://";
177+
await validateRow("Storage namespace", storagePrefix + QUICKSTART_REPO_NAME);
177178
await validateRow("Default branch", "main");
178179
});
179180
});

‎webui/test/e2e/common/readOnlyRepository.spec.ts‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,12 +3,10 @@ import { test, expect } from "../fixtures";
33
const READ_ONLY_REPO_NAME = 'ro-test-repo';
44

55
test.describe("Read Only Repository", () => {
6-
test.beforeAll(async ({ lakeFSApiWorker }) => {
6+
test.describe.configure({ mode: "serial" });
7+
test("Read only indicator shown on repositories page", async ({ repositoriesPage, lakeFSApi }) => {
78
const storageNamespace = (process.env.REPO_STORAGE_NAMESPACE_PREFIX || 'local://') + 'ro_test_repo';
8-
await lakeFSApiWorker.createRepository(READ_ONLY_REPO_NAME, storageNamespace, { readOnly: true, ifNotExists: true });
9-
});
10-
11-
test("Read only indicator shown on repositories page", async ({ repositoriesPage }) => {
9+
await lakeFSApi.createRepository(READ_ONLY_REPO_NAME, storageNamespace, { readOnly: true, ifNotExists: true });
1210
await repositoriesPage.goto();
1311
await expect(repositoriesPage.readOnlyIndicatorLocator).toBeVisible();
1412
});

‎webui/test/e2e/common/revertCommit.spec.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ test.describe("Revert Commit", () => {
2727

2828
await test.step("commit the change", async () => {
2929
await repositoryPage.changes.showOnlyChanges();
30-
expect(await repositoryPage.changes.getUncommittedCount()).toEqual(1);
30+
await expect.poll(() => repositoryPage.changes.getUncommittedCount()).toEqual(1);
3131
await repositoryPage.changes.commitChanges("Delete image file");
3232
await expect(page.getByRole("button", { name: "Uncommitted Changes" })).toHaveCount(0);
3333
});
@@ -166,7 +166,7 @@ test.describe("Revert Commit", () => {
166166
await repositoryPage.revert.clickRevertButton();
167167
await expect(page.getByRole("button", { name: "Cancel" })).toBeVisible();
168168
const checkboxes = page.locator('input[type="checkbox"]');
169-
expect(await checkboxes.count()).toBeGreaterThan(0);
169+
await expect.poll(() => checkboxes.count()).toBeGreaterThan(0);
170170
});
171171

172172
await test.step("exit revert mode", async () => {

‎webui/test/e2e/common/setupInfra.spec.ts‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,7 @@ import { test, expect } from "@playwright/test";
22
import { SetupPage } from "../poms/setupPage";
33
import { LoginPage } from "../poms/loginPage";
44
import { RepositoriesPage } from "../poms/repositoriesPage";
5-
import { COMMON_STORAGE_STATE_PATH } from "../consts";
6-
import { writeCredentials } from "../credentialsFile";
5+
import { STORAGE_STATE_PATH, writeCredentials } from "../credentialsFile";
76

87
test("setup lakeFS and save auth state", async ({ page }) => {
98
await test.step("submit setup form", async () => {
@@ -29,7 +28,7 @@ test("setup lakeFS and save auth state", async ({ page }) => {
2928
const repositoriesPage = new RepositoriesPage(loginTab);
3029
await expect(repositoriesPage.noRepositoriesTitleLocator).toBeVisible();
3130

32-
await loginTab.context().storageState({ path: COMMON_STORAGE_STATE_PATH });
31+
await loginTab.context().storageState({ path: STORAGE_STATE_PATH });
3332
await writeCredentials(credentials);
3433
});
3534
});

‎webui/test/e2e/common/syncCommitMerge.spec.ts‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -47,8 +47,7 @@ test.describe('Commit and Merge Operations', () => {
4747

4848
await test.step("commit changes and verify sync endpoint", async () => {
4949
await repositoryPage.changes.showOnlyChanges();
50-
const count = await repositoryPage.changes.getUncommittedCount();
51-
expect(count).toBe(2);
50+
await expect.poll(() => repositoryPage.changes.getUncommittedCount()).toBe(2);
5251

5352
const commitRequestPromise = page.waitForRequest((request) => {
5453
return request.url().includes('/commits') && request.method() === 'POST';

‎webui/test/e2e/consts.ts‎

Lines changed: 0 additions & 7 deletions
This file was deleted.

‎webui/test/e2e/credentialsFile.ts‎

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,17 @@
11
import fs from "fs/promises";
2-
import {RAW_CREDENTIALS_FILE_PATH} from "./consts";
3-
import {LakeFSCredentials} from "./types";
2+
import path from "path";
3+
import { fileURLToPath } from "url";
4+
import { LakeFSCredentials } from "./types";
5+
6+
const __dirname = path.dirname(fileURLToPath(import.meta.url));
7+
const CREDENTIALS_PATH = path.join(__dirname, "../playwright/credentials.json");
8+
export const STORAGE_STATE_PATH = path.join(__dirname, "../playwright/common-storage-state.json");
49

510
export const getCredentials = async (): Promise<LakeFSCredentials|null> => {
611
try {
7-
return JSON.parse(await fs.readFile(RAW_CREDENTIALS_FILE_PATH, "utf-8"));
8-
} catch (e) {
9-
if (e.code === "ENOENT") {
12+
return JSON.parse(await fs.readFile(CREDENTIALS_PATH, "utf-8"));
13+
} catch (e: unknown) {
14+
if (e instanceof Error && (e as NodeJS.ErrnoException).code === "ENOENT") {
1015
return null;
1116
}
1217
throw e;
@@ -15,5 +20,5 @@ export const getCredentials = async (): Promise<LakeFSCredentials|null> => {
1520

1621
export const writeCredentials = async (credentials: LakeFSCredentials): Promise<void> => {
1722
const jsonCredentials = JSON.stringify(credentials);
18-
await fs.writeFile(RAW_CREDENTIALS_FILE_PATH, jsonCredentials);
23+
await fs.writeFile(CREDENTIALS_PATH, jsonCredentials);
1924
}

0 commit comments

Comments
 (0)