From e045daadad7a37d3967def5642c434974b2eb289 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 26 Feb 2026 13:04:03 -0600 Subject: [PATCH 1/3] nicer browser console output on expected API errors --- app/api/client.ts | 70 ++++++++++++++----- app/components/AttachFloatingIpModal.tsx | 8 ++- app/forms/image-upload.tsx | 18 +++-- app/pages/project/instances/NetworkingTab.tsx | 8 ++- app/pages/project/snapshots/SnapshotsPage.tsx | 18 ++++- app/pages/system/silos/SiloScimTab.tsx | 18 ++++- app/table/cells/IpPoolCell.tsx | 8 ++- app/table/cells/IpVersionCell.tsx | 8 ++- test/e2e/image-upload.e2e.ts | 11 +++ 9 files changed, 129 insertions(+), 38 deletions(-) diff --git a/app/api/client.ts b/app/api/client.ts index ae8299c0cd..c310cece6d 100644 --- a/app/api/client.ts +++ b/app/api/client.ts @@ -88,7 +88,7 @@ type HandledResult = { type: 'success'; data: T } | { type: 'error'; data: Ap // getUseApiQuery, etc. This is fine because it is only being called inside // functions where `method` is already required to be an API method. const handleResult = - (method: string) => + ({ method, errorsExpected }: { method: string; errorsExpected?: string }) => (result: ApiResult): HandledResult => { if (result.type === 'success') return { type: 'success', data: result.data } @@ -111,15 +111,30 @@ const handleResult = const consolePage = window.location.pathname + window.location.search // TODO: need to change oxide.ts to put the HTTP method on the result in // order to log it here - console.error( - `More info about API ${error.statusCode || 'error'} on ${consolePage} - -API URL: ${result.response.url} + const logFn = errorsExpected ? console.info : console.error + const details = `API URL: ${result.response.url} Request ID: ${error.requestId} Error code: ${error.errorCode} -Error message: ${error.message.replace(/\n/g, '\n' + ' '.repeat('Error message: '.length))} +Error message: ${error.message.replace(/\n/g, '\n' + ' '.repeat('Error message: '.length))}` + if (errorsExpected) { + logFn( + `API ${error.statusCode || 'error'} on ${consolePage} + +%cThis error is expected: %c${errorsExpected} + +${details} +`, + 'font-weight: bold', + 'font-weight: normal' + ) + } else { + logFn( + `More info about API ${error.statusCode || 'error'} on ${consolePage} + +${details} ` - ) + ) + } } return { type: 'error', data: error } @@ -234,9 +249,17 @@ export function ensurePrefetched( export const q = ( f: (p: Params) => Promise>, params: Params, - options: UseQueryOtherOptions = {} -) => - queryOptions({ + options: UseQueryOtherOptions & { + /** + * When set, errors are logged as `console.info` instead of + * `console.error`. Appears after "This error is expected: " in the + * console, e.g., `"404 means the image name is not taken."` + */ + errorsExpected?: string + } = {} +) => { + const { errorsExpected, ...rqOptions } = options + return queryOptions({ // method name first means we can invalidate all calls to this endpoint by // invalidating [f.name] (see invalidateEndpoint) queryKey: [f.name, params], @@ -248,7 +271,7 @@ export const q = ( // in the middle of prefetching queryFn: () => f(params) - .then(handleResult(f.name)) + .then(handleResult({ method: f.name, errorsExpected })) .then((result) => { if (result.type === 'success') return result.data throw result.data @@ -258,8 +281,9 @@ export const q = ( // up as `error` state instead, pass `throwOnError: false` as an // option from the calling component and it will override this throwOnError: (err) => err.statusCode === 404, - ...options, + ...rqOptions, }) +} const ERRORS_ALLOWED = 'errors-allowed' @@ -282,21 +306,31 @@ const ERRORS_ALLOWED = 'errors-allowed' export const qErrorsAllowed = ( f: (p: Params) => Promise>, params: Params, - options: UseQueryOtherOptions> = {} -) => - queryOptions({ + options: UseQueryOtherOptions> & { + /** + * Explanation of why errors from this endpoint are expected, logged as + * `console.info` instead of `console.error`. Should be a sentence + * fragment explaining why the error is expected, e.g., + * `"404 means the image name is not taken"`. + */ + errorsExpected: string + } +) => { + const { errorsExpected, ...rqOptions } = options + return queryOptions({ // extra bit of key is important to distinguish from normal query. if we // hit a given endpoint twice on the same page, once the normal way and // once with errors allowed the responses have different shapes, so we do // not want to share the cache and mix them up queryKey: [f.name, params, ERRORS_ALLOWED], - queryFn: () => f(params).then(handleResult(f.name)), + queryFn: () => f(params).then(handleResult({ method: f.name, errorsExpected })), // No point having throwOnError because errors do not throw. Worth // considering still throwing for particular errors: sometimes we expect // a 403, other times we expect 404s. We could take a list of acceptable // status codes. - ...options, + ...rqOptions, }) +} // Unlike the query one, we don't need this to go through an options object // because we are not calling the mutation in two spots and sharing the options @@ -326,7 +360,7 @@ export const useApiMutation = ( // us back Params, but TS can't prove Omit // === Params structurally. f(params as Params, { signal: __signal }) - .then(handleResult(f.name)) + .then(handleResult({ method: f.name })) .then((result) => { if (result.type === 'success') return result.data throw result.data diff --git a/app/components/AttachFloatingIpModal.tsx b/app/components/AttachFloatingIpModal.tsx index de8d063473..5107cb3dc9 100644 --- a/app/components/AttachFloatingIpModal.tsx +++ b/app/components/AttachFloatingIpModal.tsx @@ -27,7 +27,13 @@ import { ModalForm } from './form/ModalForm' function IpPoolName({ ipPoolId }: { ipPoolId: string }) { const { data: result } = useQuery( - qErrorsAllowed(api.ipPoolView, { path: { pool: ipPoolId } }) + qErrorsAllowed( + api.ipPoolView, + { path: { pool: ipPoolId } }, + { + errorsExpected: 'the referenced IP pool may have been deleted.', + } + ) ) // As with IpPoolCell, this should never happen, but to be safe … if (!result || result.type === 'error') return null diff --git a/app/forms/image-upload.tsx b/app/forms/image-upload.tsx index a38732ef03..c99b2bea42 100644 --- a/app/forms/image-upload.tsx +++ b/app/forms/image-upload.tsx @@ -517,19 +517,17 @@ export default function ImageCreate() { // check that image name isn't taken before starting the whole thing const image = await queryClient .fetchQuery( - q(api.imageView, { - path: { image: values.imageName }, - query: { project }, - }) + q( + api.imageView, + { path: { image: values.imageName }, query: { project } }, + { + errorsExpected: '404 means the image name is not taken.', + } + ) ) .catch((e) => { // eat a 404 since that's what we want. anything else should still blow up - if (e.statusCode === 404) { - console.info( - '/v1/images 404 is expected. It means the image name is not taken.' - ) - return null - } + if (e.statusCode === 404) return null throw e }) if (image) { diff --git a/app/pages/project/instances/NetworkingTab.tsx b/app/pages/project/instances/NetworkingTab.tsx index 994836927b..a8aff40545 100644 --- a/app/pages/project/instances/NetworkingTab.tsx +++ b/app/pages/project/instances/NetworkingTab.tsx @@ -132,9 +132,11 @@ export async function clientLoader({ params }: LoaderFunctionArgs) { for (const pool of pools.items) { // both IpPoolCell and the fetch in the model use errors-allowed // versions to avoid blowing up in the unlikely event of an error - const { queryKey } = qErrorsAllowed(api.ipPoolView, { - path: { pool: pool.id }, - }) + const { queryKey } = qErrorsAllowed( + api.ipPoolView, + { path: { pool: pool.id } }, + { errorsExpected: 'the referenced IP pool may have been deleted.' } + ) queryClient.setQueryData(queryKey, { type: 'success', data: pool }) } }), diff --git a/app/pages/project/snapshots/SnapshotsPage.tsx b/app/pages/project/snapshots/SnapshotsPage.tsx index bcbe981112..4f5c2a9383 100644 --- a/app/pages/project/snapshots/SnapshotsPage.tsx +++ b/app/pages/project/snapshots/SnapshotsPage.tsx @@ -48,7 +48,15 @@ const DiskNameFromId = ({ value: string onClick: (disk: Disk) => void }) => { - const { data } = useQuery(qErrorsAllowed(api.diskView, { path: { disk: value } })) + const { data } = useQuery( + qErrorsAllowed( + api.diskView, + { path: { disk: value } }, + { + errorsExpected: 'the source disk may have been deleted.', + } + ) + ) if (!data) return if (data.type === 'error') return Deleted @@ -86,7 +94,13 @@ export async function clientLoader({ params }: LoaderFunctionArgs) { .then((disks) => { for (const disk of disks.items) { queryClient.setQueryData( - qErrorsAllowed(api.diskView, { path: { disk: disk.id } }).queryKey, + qErrorsAllowed( + api.diskView, + { path: { disk: disk.id } }, + { + errorsExpected: 'the source disk may have been deleted.', + } + ).queryKey, { type: 'success', data: disk } ) } diff --git a/app/pages/system/silos/SiloScimTab.tsx b/app/pages/system/silos/SiloScimTab.tsx index e6b5b62333..113efe7cc6 100644 --- a/app/pages/system/silos/SiloScimTab.tsx +++ b/app/pages/system/silos/SiloScimTab.tsx @@ -74,7 +74,15 @@ const EmptyState = () => ( export async function clientLoader({ params }: LoaderFunctionArgs) { const { silo } = getSiloSelector(params) // Use errors-allowed approach so 403s don't throw and break the loader - await queryClient.prefetchQuery(qErrorsAllowed(api.scimTokenList, { query: { silo } })) + await queryClient.prefetchQuery( + qErrorsAllowed( + api.scimTokenList, + { query: { silo } }, + { + errorsExpected: 'the current user may not have permission to list SCIM tokens.', + } + ) + ) return null } @@ -86,7 +94,13 @@ type ModalState = export default function SiloScimTab() { const siloSelector = useSiloSelector() const { data: tokensResult } = usePrefetchedQuery( - qErrorsAllowed(api.scimTokenList, { query: siloSelector }) + qErrorsAllowed( + api.scimTokenList, + { query: siloSelector }, + { + errorsExpected: 'the current user may not have permission to list SCIM tokens.', + } + ) ) const [modalState, setModalState] = useState(false) diff --git a/app/table/cells/IpPoolCell.tsx b/app/table/cells/IpPoolCell.tsx index 58383e0f8d..9412b0a834 100644 --- a/app/table/cells/IpPoolCell.tsx +++ b/app/table/cells/IpPoolCell.tsx @@ -14,7 +14,13 @@ import { EmptyCell, SkeletonCell } from './EmptyCell' export const IpPoolCell = ({ ipPoolId }: { ipPoolId: string }) => { const { data: result } = useQuery( - qErrorsAllowed(api.ipPoolView, { path: { pool: ipPoolId } }) + qErrorsAllowed( + api.ipPoolView, + { path: { pool: ipPoolId } }, + { + errorsExpected: 'the referenced IP pool may have been deleted.', + } + ) ) if (!result) return // this should essentially never happen, but it's probably better than blowing diff --git a/app/table/cells/IpVersionCell.tsx b/app/table/cells/IpVersionCell.tsx index 0b66e12bea..76f8d010a9 100644 --- a/app/table/cells/IpVersionCell.tsx +++ b/app/table/cells/IpVersionCell.tsx @@ -14,7 +14,13 @@ import { EmptyCell, SkeletonCell } from './EmptyCell' export const IpVersionCell = ({ ipPoolId }: { ipPoolId: string }) => { const { data: result } = useQuery( - qErrorsAllowed(api.ipPoolView, { path: { pool: ipPoolId } }) + qErrorsAllowed( + api.ipPoolView, + { path: { pool: ipPoolId } }, + { + errorsExpected: 'the referenced IP pool may have been deleted.', + } + ) ) if (!result) return if (result.type === 'error') return diff --git a/test/e2e/image-upload.e2e.ts b/test/e2e/image-upload.e2e.ts index cc98bd0e4e..0491d8c5ad 100644 --- a/test/e2e/image-upload.e2e.ts +++ b/test/e2e/image-upload.e2e.ts @@ -15,6 +15,14 @@ import { sleep, } from './utils' +/** Assert that a console message matching `msg` was logged at the given level. */ +async function expectConsoleMessage(page: Page, msg: string, type: string) { + const messages = await page.consoleMessages() + const match = messages.find((m) => m.text().includes(msg)) + expect(match, `expected console message containing "${msg}"`).toBeTruthy() + expect(match!.type()).toBe(type) +} + // playwright isn't quick enough to catch each step going from ready to running // to complete in time, so we just assert that they all start out ready and end // up complete @@ -69,6 +77,9 @@ test.describe('Image upload', () => { // now the modal pops open and the thing starts going await expectUploadProcess(page) + // the image name check 404 should be logged as info, not error + await expectConsoleMessage(page, 'This error is expected', 'info') + await expect(page).toHaveURL('/projects/mock-project/images') await expectRowVisible(page.locator('role=table'), { name: 'new-image', From 7e4ad3e836865220aa700f2d8d5ed66f7b565ff6 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 26 Feb 2026 14:26:59 -0600 Subject: [PATCH 2/3] get even more elaborate with expected status --- app/api/client.ts | 51 ++++++++++++++----- app/components/AttachFloatingIpModal.tsx | 5 +- app/forms/image-upload.tsx | 5 +- app/pages/project/instances/NetworkingTab.tsx | 9 +++- app/pages/project/snapshots/SnapshotsPage.tsx | 36 +++++++------ app/pages/system/silos/SiloScimTab.tsx | 30 +++++------ app/table/cells/IpPoolCell.tsx | 5 +- app/table/cells/IpVersionCell.tsx | 5 +- test/e2e/image-upload.e2e.ts | 3 +- 9 files changed, 93 insertions(+), 56 deletions(-) diff --git a/app/api/client.ts b/app/api/client.ts index c310cece6d..0324f2c015 100644 --- a/app/api/client.ts +++ b/app/api/client.ts @@ -82,13 +82,27 @@ export type ResultsPage = { items: TItem[]; nextPage?: string | null } type HandledResult = { type: 'success'; data: T } | { type: 'error'; data: ApiError } +type ExpectedError = { + /** + * Why this error response is expected at the call site. + * Logged after "This error is expected: ", so this should be a sentence + * fragment starting lowercase and ending with punctuation, e.g., + * "404 means the image name is not taken." + */ + explanation: string + /** Expected HTTP status for the handled error case */ + statusCode: number +} + +const expectedErrorLabel = ({ statusCode }: ExpectedError) => `status ${statusCode}` + // method: keyof Api would be strictly more correct, but making it a string // means we can call this directly in all the spots below instead of having to // make it generic over Api, which requires passing it as an argument to // getUseApiQuery, etc. This is fine because it is only being called inside // functions where `method` is already required to be an API method. const handleResult = - ({ method, errorsExpected }: { method: string; errorsExpected?: string }) => + ({ method, errorsExpected }: { method: string; errorsExpected?: ExpectedError }) => (result: ApiResult): HandledResult => { if (result.type === 'success') return { type: 'success', data: result.data } @@ -111,16 +125,19 @@ const handleResult = const consolePage = window.location.pathname + window.location.search // TODO: need to change oxide.ts to put the HTTP method on the result in // order to log it here - const logFn = errorsExpected ? console.info : console.error + const matchesExpectedError = + !!errorsExpected && error.statusCode === errorsExpected.statusCode + const logFn = matchesExpectedError ? console.info : console.error const details = `API URL: ${result.response.url} Request ID: ${error.requestId} Error code: ${error.errorCode} Error message: ${error.message.replace(/\n/g, '\n' + ' '.repeat('Error message: '.length))}` - if (errorsExpected) { + if (matchesExpectedError) { logFn( `API ${error.statusCode || 'error'} on ${consolePage} -%cThis error is expected: %c${errorsExpected} +%cThis error is expected: %c${errorsExpected.explanation} +Expected: ${expectedErrorLabel(errorsExpected)} ${details} `, @@ -128,10 +145,17 @@ ${details} 'font-weight: normal' ) } else { + const mismatchDetails = errorsExpected + ? ` +Expected: ${expectedErrorLabel(errorsExpected)} +Reason: ${errorsExpected.explanation} +` + : '' logFn( `More info about API ${error.statusCode || 'error'} on ${consolePage} -${details} +${mismatchDetails}${details} + ` ) } @@ -251,11 +275,11 @@ export const q = ( params: Params, options: UseQueryOtherOptions & { /** - * When set, errors are logged as `console.info` instead of - * `console.error`. Appears after "This error is expected: " in the - * console, e.g., `"404 means the image name is not taken."` + * Expected error details. Matching errors are logged as `console.info` + * with the explanation; mismatches remain `console.error`. + * The explanation is rendered after "This error is expected: ". */ - errorsExpected?: string + errorsExpected?: ExpectedError } = {} ) => { const { errorsExpected, ...rqOptions } = options @@ -308,12 +332,11 @@ export const qErrorsAllowed = ( params: Params, options: UseQueryOtherOptions> & { /** - * Explanation of why errors from this endpoint are expected, logged as - * `console.info` instead of `console.error`. Should be a sentence - * fragment explaining why the error is expected, e.g., - * `"404 means the image name is not taken"`. + * Expected error details. Matching errors are logged as `console.info` + * with the explanation; mismatches remain `console.error`. + * The explanation is rendered after "This error is expected: ". */ - errorsExpected: string + errorsExpected: ExpectedError } ) => { const { errorsExpected, ...rqOptions } = options diff --git a/app/components/AttachFloatingIpModal.tsx b/app/components/AttachFloatingIpModal.tsx index 5107cb3dc9..2e48b5f7da 100644 --- a/app/components/AttachFloatingIpModal.tsx +++ b/app/components/AttachFloatingIpModal.tsx @@ -31,7 +31,10 @@ function IpPoolName({ ipPoolId }: { ipPoolId: string }) { api.ipPoolView, { path: { pool: ipPoolId } }, { - errorsExpected: 'the referenced IP pool may have been deleted.', + errorsExpected: { + explanation: 'the referenced IP pool may have been deleted.', + statusCode: 404, + }, } ) ) diff --git a/app/forms/image-upload.tsx b/app/forms/image-upload.tsx index c99b2bea42..69ad3b79d3 100644 --- a/app/forms/image-upload.tsx +++ b/app/forms/image-upload.tsx @@ -521,7 +521,10 @@ export default function ImageCreate() { api.imageView, { path: { image: values.imageName }, query: { project } }, { - errorsExpected: '404 means the image name is not taken.', + errorsExpected: { + explanation: 'the image name may not exist yet.', + statusCode: 404, + }, } ) ) diff --git a/app/pages/project/instances/NetworkingTab.tsx b/app/pages/project/instances/NetworkingTab.tsx index a8aff40545..f7dfc6aa73 100644 --- a/app/pages/project/instances/NetworkingTab.tsx +++ b/app/pages/project/instances/NetworkingTab.tsx @@ -130,12 +130,17 @@ export async function clientLoader({ params }: LoaderFunctionArgs) { .fetchQuery(q(api.ipPoolList, { query: { limit: ALL_ISH } })) .then((pools) => { for (const pool of pools.items) { - // both IpPoolCell and the fetch in the model use errors-allowed + // both IpPoolCell and the fetch in the modal use errors-allowed // versions to avoid blowing up in the unlikely event of an error const { queryKey } = qErrorsAllowed( api.ipPoolView, { path: { pool: pool.id } }, - { errorsExpected: 'the referenced IP pool may have been deleted.' } + { + errorsExpected: { + explanation: 'the referenced IP pool may have been deleted.', + statusCode: 404, + }, + } ) queryClient.setQueryData(queryKey, { type: 'success', data: pool }) } diff --git a/app/pages/project/snapshots/SnapshotsPage.tsx b/app/pages/project/snapshots/SnapshotsPage.tsx index 4f5c2a9383..fbf4c2a57d 100644 --- a/app/pages/project/snapshots/SnapshotsPage.tsx +++ b/app/pages/project/snapshots/SnapshotsPage.tsx @@ -41,6 +41,18 @@ import { TableActions } from '~/ui/lib/Table' import { docLinks } from '~/util/links' import { pb } from '~/util/path-builder' +const diskViewErrorsAllowedQ = (disk: string) => + qErrorsAllowed( + api.diskView, + { path: { disk } }, + { + errorsExpected: { + explanation: 'the source disk may have been deleted.', + statusCode: 404, + }, + } + ) + const DiskNameFromId = ({ value, onClick, @@ -48,15 +60,7 @@ const DiskNameFromId = ({ value: string onClick: (disk: Disk) => void }) => { - const { data } = useQuery( - qErrorsAllowed( - api.diskView, - { path: { disk: value } }, - { - errorsExpected: 'the source disk may have been deleted.', - } - ) - ) + const { data } = useQuery(diskViewErrorsAllowedQ(value)) if (!data) return if (data.type === 'error') return Deleted @@ -93,16 +97,10 @@ export async function clientLoader({ params }: LoaderFunctionArgs) { .fetchQuery(q(api.diskList, { query: { project, limit: 200 } })) .then((disks) => { for (const disk of disks.items) { - queryClient.setQueryData( - qErrorsAllowed( - api.diskView, - { path: { disk: disk.id } }, - { - errorsExpected: 'the source disk may have been deleted.', - } - ).queryKey, - { type: 'success', data: disk } - ) + queryClient.setQueryData(diskViewErrorsAllowedQ(disk.id).queryKey, { + type: 'success', + data: disk, + }) } }), ]) diff --git a/app/pages/system/silos/SiloScimTab.tsx b/app/pages/system/silos/SiloScimTab.tsx index 113efe7cc6..5aeeee375b 100644 --- a/app/pages/system/silos/SiloScimTab.tsx +++ b/app/pages/system/silos/SiloScimTab.tsx @@ -44,6 +44,18 @@ import { docLinks } from '~/util/links' export const handle = makeCrumb('SCIM') +const scimTokenListErrorsAllowedQ = (silo: string) => + qErrorsAllowed( + api.scimTokenList, + { query: { silo } }, + { + errorsExpected: { + explanation: 'the current user may not have permission to list SCIM tokens.', + statusCode: 403, + }, + } + ) + const colHelper = createColumnHelper() const staticColumns = [ colHelper.accessor('id', { @@ -74,15 +86,7 @@ const EmptyState = () => ( export async function clientLoader({ params }: LoaderFunctionArgs) { const { silo } = getSiloSelector(params) // Use errors-allowed approach so 403s don't throw and break the loader - await queryClient.prefetchQuery( - qErrorsAllowed( - api.scimTokenList, - { query: { silo } }, - { - errorsExpected: 'the current user may not have permission to list SCIM tokens.', - } - ) - ) + await queryClient.prefetchQuery(scimTokenListErrorsAllowedQ(silo)) return null } @@ -94,13 +98,7 @@ type ModalState = export default function SiloScimTab() { const siloSelector = useSiloSelector() const { data: tokensResult } = usePrefetchedQuery( - qErrorsAllowed( - api.scimTokenList, - { query: siloSelector }, - { - errorsExpected: 'the current user may not have permission to list SCIM tokens.', - } - ) + scimTokenListErrorsAllowedQ(siloSelector.silo) ) const [modalState, setModalState] = useState(false) diff --git a/app/table/cells/IpPoolCell.tsx b/app/table/cells/IpPoolCell.tsx index 9412b0a834..e4cb6ba4da 100644 --- a/app/table/cells/IpPoolCell.tsx +++ b/app/table/cells/IpPoolCell.tsx @@ -18,7 +18,10 @@ export const IpPoolCell = ({ ipPoolId }: { ipPoolId: string }) => { api.ipPoolView, { path: { pool: ipPoolId } }, { - errorsExpected: 'the referenced IP pool may have been deleted.', + errorsExpected: { + explanation: 'the referenced IP pool may have been deleted.', + statusCode: 404, + }, } ) ) diff --git a/app/table/cells/IpVersionCell.tsx b/app/table/cells/IpVersionCell.tsx index 76f8d010a9..7961341354 100644 --- a/app/table/cells/IpVersionCell.tsx +++ b/app/table/cells/IpVersionCell.tsx @@ -18,7 +18,10 @@ export const IpVersionCell = ({ ipPoolId }: { ipPoolId: string }) => { api.ipPoolView, { path: { pool: ipPoolId } }, { - errorsExpected: 'the referenced IP pool may have been deleted.', + errorsExpected: { + explanation: 'the referenced IP pool may have been deleted.', + statusCode: 404, + }, } ) ) diff --git a/test/e2e/image-upload.e2e.ts b/test/e2e/image-upload.e2e.ts index 0491d8c5ad..542d1cef87 100644 --- a/test/e2e/image-upload.e2e.ts +++ b/test/e2e/image-upload.e2e.ts @@ -77,8 +77,9 @@ test.describe('Image upload', () => { // now the modal pops open and the thing starts going await expectUploadProcess(page) - // the image name check 404 should be logged as info, not error + // the image name check 404 should be logged as expected-info, with context await expectConsoleMessage(page, 'This error is expected', 'info') + await expectConsoleMessage(page, 'the image name may not exist yet.', 'info') await expect(page).toHaveURL('/projects/mock-project/images') await expectRowVisible(page.locator('role=table'), { From f50023d509eb79a7a98b8a89fe085e729ebae2a3 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 26 Feb 2026 16:13:00 -0600 Subject: [PATCH 3/3] skip console message checks in FF, consolidate logic --- test/e2e/error-pages.e2e.ts | 19 ++++++------------- test/e2e/image-upload.e2e.ts | 9 +-------- test/e2e/instance-metrics.e2e.ts | 6 +++--- test/e2e/utils.ts | 24 +++++++++++++++++++++--- 4 files changed, 31 insertions(+), 27 deletions(-) diff --git a/test/e2e/error-pages.e2e.ts b/test/e2e/error-pages.e2e.ts index c92d743048..79d21a6691 100644 --- a/test/e2e/error-pages.e2e.ts +++ b/test/e2e/error-pages.e2e.ts @@ -5,9 +5,7 @@ * * Copyright Oxide Computer Company */ -import { expect, test } from '@playwright/test' - -import { getPageAsUser, hasConsoleMessage } from './utils' +import { expect, expectConsoleMessage, getPageAsUser, test } from './utils' test('Shows 404 page when a resource is not found', async ({ page }) => { await page.goto('/nonexistent') @@ -19,22 +17,17 @@ test('Shows 404 page when a resource is not found', async ({ page }) => { await expect(page.getByRole('button', { name: 'Sign out' })).toBeVisible() }) -test('Shows something went wrong page on other errors', async ({ page, browserName }) => { +test('Shows something went wrong page on other errors', async ({ page }) => { await page.goto('/projects/error-503') // specially handled in mock server await expect(page.getByText('Something went wrong')).toBeVisible() // Invariant failed doesn't show up in the page... await expect(page.getByText('Invariant failed')).toBeHidden() - // But we do see it in the browser console. Skip Firefox because it handles - // these errors differently and it's hard to get the error text out. - // eslint-disable-next-line playwright/no-conditional-in-test - if (browserName !== 'firefox') { - const error = - 'Expected query to be prefetched.\nKey: ["projectView",{"path":{"project":"error-503"}}]' - // eslint-disable-next-line playwright/no-conditional-expect - expect(await hasConsoleMessage(page, error)).toBeTruthy() - } + // But we do see it in the browser console + const error = + 'Expected query to be prefetched.\nKey: ["projectView",{"path":{"project":"error-503"}}]' + await expectConsoleMessage(page, error) // test clicking sign out await page.getByRole('button', { name: 'Sign out' }).click() diff --git a/test/e2e/image-upload.e2e.ts b/test/e2e/image-upload.e2e.ts index 542d1cef87..cfb3f97e03 100644 --- a/test/e2e/image-upload.e2e.ts +++ b/test/e2e/image-upload.e2e.ts @@ -9,20 +9,13 @@ import { expect, test, type Page } from '@playwright/test' import { chooseFile, + expectConsoleMessage, expectNotVisible, expectRowVisible, expectVisible, sleep, } from './utils' -/** Assert that a console message matching `msg` was logged at the given level. */ -async function expectConsoleMessage(page: Page, msg: string, type: string) { - const messages = await page.consoleMessages() - const match = messages.find((m) => m.text().includes(msg)) - expect(match, `expected console message containing "${msg}"`).toBeTruthy() - expect(match!.type()).toBe(type) -} - // playwright isn't quick enough to catch each step going from ready to running // to complete in time, so we just assert that they all start out ready and end // up complete diff --git a/test/e2e/instance-metrics.e2e.ts b/test/e2e/instance-metrics.e2e.ts index effaecc081..5047bb6eb9 100644 --- a/test/e2e/instance-metrics.e2e.ts +++ b/test/e2e/instance-metrics.e2e.ts @@ -10,7 +10,7 @@ import { expect, test } from '@playwright/test' import { OXQL_GROUP_BY_ERROR } from '~/api' -import { getPageAsUser, hasConsoleMessage } from './utils' +import { expectConsoleMessage, expectNoConsoleMessage, getPageAsUser } from './utils' test('Click through instance metrics', async ({ page }) => { await page.goto('/projects/mock-project/instances/db1/metrics/cpu') @@ -67,14 +67,14 @@ test('empty and loading states', async ({ page }) => { await expect(noData).toBeVisible() // idle state returns group_by must be aligned error, treated as empty - expect(await hasConsoleMessage(page, OXQL_GROUP_BY_ERROR)).toBe(false) // error not in console + await expectNoConsoleMessage(page, OXQL_GROUP_BY_ERROR) await statePicker.click() await page.getByRole('option', { name: 'State: Idle' }).click() await expect(loading).toBeVisible() await expect(loading).toBeHidden() await expect(page.getByText('Something went wrong')).toBeHidden() await expect(noData).toBeVisible() - expect(await hasConsoleMessage(page, OXQL_GROUP_BY_ERROR)).toBe(true) // error present in console + await expectConsoleMessage(page, OXQL_GROUP_BY_ERROR) // make sure empty state goes away again for the first one await statePicker.click() diff --git a/test/e2e/utils.ts b/test/e2e/utils.ts index 77dc01a329..e573ef51b6 100644 --- a/test/e2e/utils.ts +++ b/test/e2e/utils.ts @@ -5,7 +5,7 @@ * * Copyright Oxide Computer Company */ -import { expect, type Browser, type Locator, type Page } from '@playwright/test' +import { expect, test, type Browser, type Locator, type Page } from '@playwright/test' import { MiB } from '~/util/units' @@ -286,7 +286,25 @@ export async function addTlsCert(page: Page) { await page.getByRole('button', { name: 'Add Certificate' }).click() } -export async function hasConsoleMessage(page: Page, msg: string) { +/** + * Assert that a console message matching `msg` was logged (optionally at a + * given level). Skips on Firefox because Playwright's `page.consoleMessages()` + * drops real console output there and only returns React profiling entries. + */ +export async function expectConsoleMessage(page: Page, msg: string, type?: string) { + // eslint-disable-next-line playwright/no-conditional-in-test + if (test.info().project.name === 'firefox') return + const messages = await page.consoleMessages() + const match = messages.find((m) => m.text().includes(msg)) + expect(match, `expected console message containing "${msg}"`).toBeTruthy() + if (type) expect(match!.type()).toBe(type) +} + +/** Assert that no console message matching `msg` was logged. Skips on Firefox. */ +export async function expectNoConsoleMessage(page: Page, msg: string) { + // eslint-disable-next-line playwright/no-conditional-in-test + if (test.info().project.name === 'firefox') return const messages = await page.consoleMessages() - return messages.some((m) => m.text().includes(msg)) + const match = messages.find((m) => m.text().includes(msg)) + expect(match, `expected no console message containing "${msg}"`).toBeFalsy() }