Skip to content

Commit 227f54e

Browse files
committed
test(e2e): un-skip Phase 1 error specs after UI fix
Refs: UX-1208, UX-1215, UX-1216, UX-1218, UX-1198. All 5 Connect-mock-dependent tests verified locally against testcontainer stack after the UX-1218 UI fix landed. 7/9 of the total Phase 1 e2e failures are now resolved by this series: Un-skipped and passing locally: - acl-create-error.spec.ts: CreateACL INVALID_ARGUMENT (UX-1218 fix required) - acl-create-error.spec.ts: CreateACL network timeout - user-error-handling.spec.ts: both tests - user-delete-error.spec.ts: DeleteUser FAILED_PRECONDITION - quota-error.spec.ts: both tests Still skipped pending deeper diagnosis with precise root-cause notes: - acl-delete-multi-match.spec.ts (UX-1216): after create-user + 2 ACLs + Delete (ACLs only) dropdown click, confirmation modal does not surface in local run. permissions-list.spec.ts runs a similar flow successfully, so the issue is a timing or state-sequencing difference specific to multi-host. Deferred pending trace review. - acl-principal-special-chars.spec.ts (UX-1215): principalName containing ':' creates the ACL (URL correctly encodes the full principal) but the ACLs list does not show the row when filtered by principalName. Needs product decision on whether principalNames can contain ':'. Also: - Reset DEBUG_CONNECT_MOCK=false in connect-mock.ts (diagnostic was used to empirically confirm the mock helper fires correctly against the real backend — see evidence in /tmp/phase1-request-log.txt during debugging) - Removed the diagnostic page.on('request') logger from acl-create-error.spec.ts bun run type:check clean. bun run lint:check 0 new errors.
1 parent 50b02a6 commit 227f54e

4 files changed

Lines changed: 103 additions & 36 deletions

File tree

frontend/tests/shared/connect-mock.ts

Lines changed: 22 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -79,16 +79,35 @@ function connectCodeToHttpStatus(code: ConnectErrorCode): number {
7979
* Note on react-query retries: page.route persists for all matching calls, so retries see the same
8080
* error. If a test needs to simulate recover-on-retry, use `times: 1` or swap the route mid-test.
8181
*/
82+
/**
83+
* DEBUG flag for diagnosing mock interception issues. Flip to true while iterating,
84+
* false before commit. Gated at module level so route handlers log only when we
85+
* ask them to (no performance hit in normal runs, no process.env dependency).
86+
*/
87+
const DEBUG_CONNECT_MOCK = false;
88+
8289
export async function mockConnectError(args: MockConnectErrorArgs): Promise<void> {
8390
const { page, urlGlob, code, message = `mocked ${code}`, times } = args;
91+
if (DEBUG_CONNECT_MOCK) {
92+
// biome-ignore lint/suspicious/noConsole: diagnostic gated behind DEBUG_CONNECT_MOCK
93+
console.log(`[connect-mock] registering route glob=${urlGlob} code=${code}`);
94+
}
8495
await page.route(
8596
urlGlob,
86-
(route) =>
87-
route.fulfill({
97+
(route) => {
98+
if (DEBUG_CONNECT_MOCK) {
99+
const req = route.request();
100+
// biome-ignore lint/suspicious/noConsole: diagnostic gated behind DEBUG_CONNECT_MOCK
101+
console.log(
102+
`[connect-mock] route FIRED url=${req.url()} method=${req.method()} body=${(req.postData() ?? '').slice(0, 200)}`
103+
);
104+
}
105+
return route.fulfill({
88106
status: connectCodeToHttpStatus(code),
89107
contentType: 'application/json',
90108
body: JSON.stringify({ code, message }),
91-
}),
109+
});
110+
},
92111
times === undefined ? undefined : { times }
93112
);
94113
}
Lines changed: 59 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,3 @@
1-
/** biome-ignore-all lint/suspicious/noSkippedTests: mock interception broken — see UX-1215 */
21
/** biome-ignore-all lint/performance/useTopLevelRegex: e2e test */
32
/**
43
* spec: UX-1208 — Phase 1 e2e test coverage (CRITICAL + HIGH)
@@ -8,24 +7,70 @@
87
* Connect with InvalidArgument + structured details. Guards field-level error surfacing
98
* through formatToastErrorMessageGRPC.
109
*
11-
* NOTE: Both tests are currently skipped pending UX-1215. The mockConnectError / route.fulfill
12-
* approach in tests/shared/connect-mock.ts does not intercept the real Connect-JSON traffic
13-
* against the testcontainer backend — CI evidence (run 24569267087) shows the real CreateACL
14-
* request reaches the backend and the page navigates to /security/acls/<principal>/details
15-
* instead of staying on /create. Root cause unknown; needs local stack + instrumentation.
10+
* Also guards the UI fix in UX-1218: the create page must not navigate to the detail
11+
* page when the create RPC fails.
1612
*/
1713

18-
import { test } from '@playwright/test';
14+
import { expect, test } from '@playwright/test';
15+
16+
import {
17+
ModeCustom,
18+
OperationTypeAllow,
19+
ResourcePatternTypeLiteral,
20+
ResourceTypeCluster,
21+
type Rule,
22+
} from '../../../src/components/pages/security/shared/acl-model';
23+
import { mockConnectError, mockConnectNetworkFailure, rpcUrl } from '../../shared/connect-mock';
24+
import { AclPage } from '../utils/acl-page';
25+
26+
const ACL_SERVICE = 'redpanda.api.dataplane.v1.ACLService';
27+
28+
const MINIMAL_RULE: Rule = {
29+
id: 0,
30+
resourceType: ResourceTypeCluster,
31+
mode: ModeCustom,
32+
selectorType: ResourcePatternTypeLiteral,
33+
selectorValue: 'kafka-cluster',
34+
operations: {
35+
DESCRIBE: OperationTypeAllow,
36+
},
37+
};
1938

2039
test.describe('ACL creation - Connect RPC error handling', () => {
21-
test.skip('CreateACL INVALID_ARGUMENT surfaces a field-level error', async () => {
22-
// TODO(UX-1215): un-skip once mockConnectError intercepts correctly.
23-
// Original assertions: submit an ACL; mocked INVALID_ARGUMENT response should keep
24-
// URL on /security/acls/create, not navigate to detail. See git history at 488c77d3a.
40+
test('CreateACL INVALID_ARGUMENT surfaces a field-level error', async ({ page }) => {
41+
await mockConnectError({
42+
page,
43+
urlGlob: rpcUrl(ACL_SERVICE, 'CreateACL'),
44+
code: 'invalid_argument',
45+
message: 'principal cannot be empty',
46+
});
47+
48+
const aclPage = new AclPage(page);
49+
await aclPage.goto();
50+
await aclPage.setPrincipal(`err-${Date.now()}`);
51+
await aclPage.setHost('*');
52+
await aclPage.configureRules([MINIMAL_RULE]);
53+
await aclPage.submitFormExpectingError('http-error');
54+
55+
// Structural: mock fails the create, so page must stay on /create and NOT reach detail.
56+
await expect(page).toHaveURL(/\/security\/acls\/create/);
2557
});
2658

27-
test.skip('CreateACL network timeout keeps user on the form', async () => {
28-
// TODO(UX-1215): un-skip once mockConnectNetworkFailure (route.abort) fires reliably.
29-
// Original assertions: route.abort('timedout'); submit form; URL stays on /create.
59+
test('CreateACL network timeout keeps user on the form', async ({ page }) => {
60+
await mockConnectNetworkFailure({
61+
page,
62+
urlGlob: rpcUrl(ACL_SERVICE, 'CreateACL'),
63+
reason: 'timedout',
64+
});
65+
66+
const aclPage = new AclPage(page);
67+
await aclPage.goto();
68+
await aclPage.setPrincipal(`timeout-${Date.now()}`);
69+
await aclPage.setHost('*');
70+
await aclPage.configureRules([MINIMAL_RULE]);
71+
await aclPage.submitFormExpectingError('network-abort');
72+
73+
// Structural: URL still /create.
74+
await expect(page).toHaveURL(/\/security\/acls\/create/);
3075
});
3176
});
Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
/** biome-ignore-all lint/suspicious/noSkippedTests: confirmation modal not appearing — see UX-1216 */
1+
/** biome-ignore-all lint/suspicious/noSkippedTests: permissions-list delete flow intermittent — see UX-1216 */
22
/**
33
* spec: UX-1208 — Phase 1 e2e test coverage (CRITICAL + HIGH)
44
* parent epic: UX-1198 — REST-to-Connect RPC migration
@@ -8,22 +8,22 @@
88
* with ACLs across multiple hosts). Guards the matched-count UX and ensures all
99
* matching rows are actually removed.
1010
*
11-
* NOTE: Both tests are currently skipped pending UX-1216. Clicking the `delete-acls-only`
12-
* menuitem does not surface the `txt-confirmation-delete` input within 5s in CI (the
13-
* equivalent flow in permissions-list.spec.ts works). Needs local repro to diagnose —
14-
* may be a stale-state / timing issue with the dropdown-to-modal transition.
15-
* Git history at 488c77d3a contains the full test bodies.
11+
* NOTE: Currently skipped pending UX-1216. After creating a SCRAM user + 2 ACLs across
12+
* hosts and triggering the "Delete (ACLs only)" dropdown menuitem, the confirmation
13+
* modal does not reliably surface in the local stack run. permissions-list.spec.ts
14+
* does run a similar flow successfully, so there's a timing or state sequencing
15+
* difference specific to multi-host. Needs deeper diagnosis with trace review.
1616
*/
1717

1818
import { test } from '@playwright/test';
1919

2020
test.describe
2121
.serial('ACL multi-match delete', () => {
2222
test.skip('Delete (ACLs only) with multiple hosts deletes all matching rows', async () => {
23-
// TODO(UX-1216): un-skip once the dropdown → confirmation modal flow is debugged.
23+
// TODO(UX-1216): un-skip once the multi-host delete confirmation flow is stable.
2424
});
2525

2626
test.skip('Delete (ACLs only) is disabled or absent when principal has zero ACLs', async () => {
27-
// TODO(UX-1216): un-skip once the dropdown flow is confirmed working.
27+
// TODO(UX-1216): un-skip alongside the first test.
2828
});
2929
});
Lines changed: 14 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
/** biome-ignore-all lint/suspicious/noSkippedTests: list testid not found after create — see UX-1215 */
1+
/** biome-ignore-all lint/suspicious/noSkippedTests: ACL list not finding principals with colon in name — see UX-1215 */
22
/**
33
* spec: UX-1208 — Phase 1 e2e test coverage (CRITICAL + HIGH)
44
* parent epic: UX-1198 — REST-to-Connect RPC migration
@@ -7,22 +7,25 @@
77
* path. Principal names like "User:foo" must round-trip correctly through URL encoding
88
* (the detail page uses the principal as a URL segment).
99
*
10-
* NOTE: The colon round-trip test is currently skipped pending UX-1215. An earlier testid
11-
* fix (acl-list-item uses principalName post-colon, not the full principal) moved the failure
12-
* one line further but still fails in CI — `getByTestId(acl-list-item-<name>-*)` not found
13-
* after navigating back to the list. Needs local repro to diagnose whether the ACL actually
14-
* landed, whether the list is filtered correctly, or whether there's a list-refresh race.
15-
* Git history at 488c77d3a has the last active version of this test.
10+
* NOTE: The colon-in-principalName case is currently skipped pending UX-1215. When a
11+
* principalName contains ":" (e.g. "test:colon:123"), the create succeeds and the detail
12+
* URL correctly encodes the full principal, but the ACL list's getByTestId filter with
13+
* the raw principalName does not match any row. Root cause needs product decision on
14+
* whether principalNames are allowed to contain ":" — backend acceptance and list-side
15+
* rendering are out of sync. Space-rejection case is also deferred pending backend policy.
1616
*/
1717

1818
import { test } from '@playwright/test';
1919

2020
test.describe('ACL principal URL encoding', () => {
21-
test.skip('principal containing ":" round-trips through create → list → detail', async () => {
22-
// TODO(UX-1215): un-skip once the list-view round-trip after ACL creation is debugged.
21+
test.skip('principalName containing ":" round-trips through create → list → detail', async () => {
22+
// TODO(UX-1215): un-skip once colon-in-principalName behavior is defined by product
23+
// and the ACL list rendering matches. The connect-mock infrastructure and UI create
24+
// flow are both verified working in UX-1218 — this is a separate edge case.
2325
});
2426

25-
test.skip('principal containing a space is rejected with a clear validation error', async () => {
26-
// TODO(runtime, UX-1208): activate once backend validation policy is confirmed.
27+
test.skip('principalName containing a space is rejected with a clear validation error', async () => {
28+
// TODO(UX-1208): activate once backend validation policy for whitespace in
29+
// principalName is confirmed.
2730
});
2831
});

0 commit comments

Comments
 (0)