Copilot commented on code in PR #3305:
URL: https://github.com/apache/apisix-dashboard/pull/3305#discussion_r2897365758
##########
src/apis/upstreams.ts:
##########
@@ -76,7 +76,13 @@ export const deleteAllUpstreams = async (req: AxiosInstance)
=> {
res.list.map((d) =>
req.delete(`${API_UPSTREAMS}/${d.value.id}`).catch((err) => {
// Ignore 404 errors as the resource might have been deleted
- if (axios.isAxiosError(err) && err.response?.status === 404) return;
+ // Ignore 400 errors as the upstream might still be referenced by
routes/services
+ if (
+ axios.isAxiosError(err) &&
+ (err.response?.status === 404 || err.response?.status === 400)
+ ) {
+ return;
+ }
Review Comment:
`deleteAllUpstreams` now ignores *all* HTTP 400 responses on delete. Since
400 can indicate problems other than “still referenced”, this can silently mask
real failures and leave upstreams behind unexpectedly. Consider narrowing the
ignore condition to the specific APISIX error you expect for “referenced”
upstreams (e.g., inspect response body/code), or use a more semantically
appropriate status if APISIX provides one.
##########
e2e/tsconfig.tsbuildinfo:
##########
@@ -0,0 +1 @@
+{"root":["./pom/consumer_groups.ts","./pom/consumers.ts","./pom/credentials.ts","./pom/global_rules.ts","./pom/plugin_configs.ts","./pom/plugin_metadata.ts","./pom/protos.ts","./pom/routes.ts","./pom/secrets.ts","./pom/services.ts","./pom/ssls.ts","./pom/stream_routes.ts","./pom/type.ts","./pom/upstreams.ts","./tests/auth.spec.ts","./tests/consumer_groups.crud-all-fields.spec.ts","./tests/consumer_groups.crud-required-fields.spec.ts","./tests/consumer_groups.list.spec.ts","./tests/consumers.credentials.list.spec.ts","./tests/consumers.crud-all-fields.spec.ts","./tests/consumers.crud-required-fields.spec.ts","./tests/consumers.list.spec.ts","./tests/global_rules.crud-all-fields.spec.ts","./tests/global_rules.crud-required-fields.spec.ts","./tests/global_rules.list.spec.ts","./tests/hot-path.upstream-service-route.spec.ts","./tests/plugin_configs.crud-all-fields.spec.ts","./tests/plugin_configs.crud-required-fields.spec.ts","./tests/plugin_configs.list.spec.ts","./tests/plugin_metadat
a.crud-all-fields.spec.ts","./tests/plugin_metadata.crud-required-fields.spec.ts","./tests/plugin_metadata.list.spec.ts","./tests/protos.crud-all-fields.spec.ts","./tests/protos.crud-required-fields.spec.ts","./tests/protos.list.spec.ts","./tests/routes.clear-upstream-field.spec.ts","./tests/routes.crud-all-fields.spec.ts","./tests/routes.crud-required-fields.spec.ts","./tests/routes.list.spec.ts","./tests/secrets.crud-all-fields.spec.ts","./tests/secrets.crud-required-fields.spec.ts","./tests/secrets.list.spec.ts","./tests/services.crud-all-fields.spec.ts","./tests/services.crud-required-fields.spec.ts","./tests/services.empty.spec.ts","./tests/services.list.spec.ts","./tests/services.routes.crud.spec.ts","./tests/services.routes.list.spec.ts","./tests/services.stream_routes.crud.spec.ts","./tests/services.stream_routes.list.spec.ts","./tests/ssls.check-labels.spec.ts","./tests/ssls.crud-all-fields.spec.ts","./tests/ssls.crud-required-fields.spec.ts","./tests/ssls.list.spec.ts","./
tests/stream_routes.crud-all-fields.spec.ts","./tests/stream_routes.crud-required-fields.spec.ts","./tests/stream_routes.list.spec.ts","./tests/stream_routes.show-disabled-error.spec.ts","./tests/upstreams.crud-all-fields.spec.ts","./tests/upstreams.crud-required-fields.spec.ts","./tests/upstreams.list.spec.ts","./utils/common.ts","./utils/env.ts","./utils/pagination-test-helper.ts","./utils/req.ts","./utils/test.ts","./utils/ui/index.ts","./utils/ui/labels.ts","./utils/ui/routes.ts","./utils/ui/services.ts","./utils/ui/ssls.ts","./utils/ui/stream_routes.ts","./utils/ui/upstreams.ts","../src/types/global.d.ts"],"version":"5.8.3"}
Review Comment:
`e2e/tsconfig.tsbuildinfo` looks like a generated TypeScript build artifact
and shouldn’t be committed. Please remove it from the repo and add an ignore
rule (e.g., `**/*.tsbuildinfo`) so it won’t get reintroduced by local builds.
```suggestion
```
##########
e2e/utils/test.ts:
##########
@@ -24,50 +24,85 @@ 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);
+ // Create an empty auth state file so Playwright doesn't fail
+ // Tests will retry auth on first page load
+ try {
+ await writeFile(fileName, JSON.stringify({ cookies: [], origins: []
}));
+ } catch (writeErr) {
+ console.error(`Failed to create fallback auth state: ${writeErr}`);
+ }
+ throw error;
+ } finally {
Review Comment:
The fallback-auth comment/behavior is inconsistent: after writing an empty
storage state file, the fixture still `throw`s, so tests won’t proceed to
“retry auth on first page load”. Either remove the fallback-file logic/comment,
or don’t rethrow and instead continue with the empty storageState so tests can
attempt interactive auth later.
--
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]
