Copilot commented on code in PR #3317:
URL: https://github.com/apache/apisix-dashboard/pull/3317#discussion_r2931469890
##########
e2e/utils/test.ts:
##########
@@ -24,50 +24,78 @@ import { env } from './env';
export type Test = typeof test;
export const test = baseTest.extend<object, { workerStorageState: string }>({
- storageState: ({ workerStorageState }, use) => use(workerStorageState),
+ storageState: ({ workerStorageState }, use) => {
+ return use(workerStorageState);
+ },
workerStorageState: [
async ({ browser }, use) => {
// Use parallelIndex as a unique identifier for each worker.
const id = test.info().parallelIndex;
- const fileName = path.resolve(
- test.info().project.outputDir,
- `.auth/${id}.json`
- );
+ const authDir = path.resolve(test.info().project.outputDir, '.auth');
+ const fileName = path.resolve(authDir, `${id}.json`);
const { adminKey } = await getAPISIXConf();
+ // Ensure .auth directory exists
+ await mkdir(authDir, { recursive: true });
+
// file exists and contains admin key, use it
- if (
- (await fileExists(fileName)) &&
- (await readFile(fileName)).toString().includes(adminKey)
- ) {
- return use(fileName);
+ if (await fileExists(fileName)) {
+ try {
+ const content = await readFile(fileName);
+ if (content.toString().includes(adminKey)) {
+ return use(fileName);
+ }
+ } catch {
+ // File exists but is unreadable, recreate it
+ }
}
- const page = await browser.newPage({ storageState: undefined });
+ let page;
+ try {
+ page = await browser.newPage({ storageState: undefined });
+
+ // have to use env here, because the baseURL is not available in worker
+ await page.goto(env.E2E_TARGET_URL, { waitUntil: 'load' });
+
+ // we need to authenticate
+ const settingsModal = page.getByRole('dialog', { name: 'Settings' });
+ await expect(settingsModal).toBeVisible({ timeout: 30000 });
+ // PasswordInput renders with a label, use getByLabel instead
+ const adminKeyInput = page.getByLabel('Admin Key');
+ await adminKeyInput.clear();
+ await adminKeyInput.fill(adminKey);
- // have to use env here, because the baseURL is not available in worker
- await page.goto(env.E2E_TARGET_URL);
+ const closeButton = page
+ .getByRole('dialog', { name: 'Settings' })
+ .getByRole('button');
+ await expect(closeButton).toBeVisible({ timeout: 10000 });
+ await closeButton.click();
- // we need to authenticate
- const settingsModal = page.getByRole('dialog', { name: 'Settings' });
- await expect(settingsModal).toBeVisible();
- // PasswordInput renders with a label, use getByLabel instead
- const adminKeyInput = page.getByLabel('Admin Key');
- await adminKeyInput.clear();
- await adminKeyInput.fill(adminKey);
- await page
- .getByRole('dialog', { name: 'Settings' })
- .getByRole('button')
- .click();
+ // Wait for auth to complete
+ await expect(settingsModal).toBeHidden({ timeout: 15000 });
+
+ // Wait for any post-auth navigation/loading to complete
+ await page.waitForLoadState('load');
+
+ await page.context().storageState({ path: fileName });
+
+ // Verify auth state file was created
+ if (!(await fileExists(fileName))) {
+ throw new Error(`Auth state file was not created at ${fileName}`);
+ }
+ } catch (error) {
+ console.error(`Failed to authenticate worker ${id}:`, error);
+ throw error;
Review Comment:
`console.error` here will raise a `no-console` lint warning (repo rule is
`no-console: warn`). If you want to keep diagnostics, consider attaching the
error to the Playwright test output (e.g., `test.info()` attachments) or add a
targeted eslint-disable for this line to avoid residual lint warnings.
##########
e2e/utils/req.ts:
##########
@@ -27,30 +27,66 @@ export const getPlaywrightRequestAdapter = (
ctx: APIRequestContext
): AxiosAdapter => {
return async (config) => {
- const { url, data, baseURL } = config;
+ const { url, data, baseURL, method } = config;
if (typeof url === 'undefined') {
throw new Error('Need to provide a url');
}
type Payload = Parameters<APIRequestContext['fetch']>[1];
const payload: Payload = {
headers: config.headers,
- method: config.method,
- failOnStatusCode: true,
+ method,
+ failOnStatusCode: false,
data,
};
const urlWithBase = `${baseURL}${url}`;
const res = await ctx.fetch(urlWithBase, payload);
+ const status = res.status();
try {
- return {
+ // Idempotent DELETE: Only treat 404 as 200 OK if explicitly requested
via header
+ const allowDelete404 = config.headers?.['X-Allow-404-Delete'] === 'true';
+ if (allowDelete404 && method?.toLowerCase() === 'delete' && status ===
404) {
+ console.warn(`[e2eReq] Ignored 404 on DELETE for ${urlWithBase},
treating as 200 OK.`);
+ return {
+ data: {},
Review Comment:
The `X-Allow-404-Delete` check is case-sensitive and assumes
`config.headers` is a plain object with that exact key. Axios often normalizes
headers (and may use `AxiosHeaders`), so this condition can fail and the
intended idempotent-DELETE behavior won't apply. Consider reading the header
case-insensitively / via AxiosHeaders accessors so the 404->200 mapping works
reliably.
##########
e2e/utils/req.ts:
##########
@@ -27,30 +27,66 @@ export const getPlaywrightRequestAdapter = (
ctx: APIRequestContext
): AxiosAdapter => {
return async (config) => {
- const { url, data, baseURL } = config;
+ const { url, data, baseURL, method } = config;
if (typeof url === 'undefined') {
throw new Error('Need to provide a url');
}
type Payload = Parameters<APIRequestContext['fetch']>[1];
const payload: Payload = {
headers: config.headers,
- method: config.method,
- failOnStatusCode: true,
+ method,
+ failOnStatusCode: false,
data,
};
const urlWithBase = `${baseURL}${url}`;
const res = await ctx.fetch(urlWithBase, payload);
+ const status = res.status();
try {
- return {
+ // Idempotent DELETE: Only treat 404 as 200 OK if explicitly requested
via header
+ const allowDelete404 = config.headers?.['X-Allow-404-Delete'] === 'true';
+ if (allowDelete404 && method?.toLowerCase() === 'delete' && status ===
404) {
+ console.warn(`[e2eReq] Ignored 404 on DELETE for ${urlWithBase},
treating as 200 OK.`);
+ return {
Review Comment:
This new `console.warn` will trigger the repo's `no-console` ESLint rule
(configured as a warning). If the intent is to keep logs out of CI output,
consider removing it, using Playwright's `test.info()` attachments, or adding a
targeted eslint-disable for this line.
--
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]
