-
Notifications
You must be signed in to change notification settings - Fork 367
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test: Improve Cypress error messages when Linode API errors occur #9777
Changes from all commits
4c01ac6
ad53f47
b6a7743
8d8f415
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
"@linode/manager": Tests | ||
--- | ||
|
||
Improve error message display when Linode API error occurs in Cypress test ([#9777](https://github.com/linode/manager/pull/9777)) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,28 +1,145 @@ | ||
// *********************************************** | ||
// This example commands.js shows you how to | ||
// create various custom commands and overwrite | ||
// existing commands. | ||
// | ||
// For more comprehensive examples of custom | ||
// commands please read more here: | ||
// https://on.cypress.io/custom-commands | ||
// *********************************************** | ||
// | ||
// | ||
// -- This is a parent command -- | ||
// Cypress.Commands.add("login", (email, password) => { ... }) | ||
// | ||
// | ||
// -- This is a child command -- | ||
// Cypress.Commands.add("drag", { prevSubject: 'element'}, (subject, options) => { ... }) | ||
// | ||
// | ||
// -- This is a dual command -- | ||
// Cypress.Commands.add("dismiss", { prevSubject: 'optional'}, (subject, options) => { ... }) | ||
// | ||
// | ||
// -- This will overwrite an existing command -- | ||
// Cypress.Commands.overwrite("visit", (originalFn, url, options) => { ... }) | ||
import type { AxiosError } from 'axios'; | ||
import type { APIError } from '@linode/api-v4'; | ||
|
||
type LinodeApiV4Error = { | ||
errors: APIError[]; | ||
}; | ||
|
||
/** | ||
* Returns `true` if the given error is a Linode API schema validation error. | ||
* | ||
* Type guards `e` as an array of `APIError` objects. | ||
* | ||
* @param e - Error. | ||
* | ||
* @returns `true` if `e` is a Linode API schema validation error. | ||
*/ | ||
const isValidationError = (e: any): e is APIError[] => { | ||
// When a Linode APIv4 schema validation error occurs, an array of `APIError` | ||
// objects is thrown rather than a typical `Error` type. | ||
Comment on lines
+18
to
+19
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure if this is intended, but I think this is the source of the behavior. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah our error handling/normalization between Cloud Manager and api-v4 is a bit wacky Are you saying that api-v4 throws Axios errors normally, but There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @bnussman-akamai Yeah, that's my understanding -- an actual error from the API (like a 400 response, etc.) is an I don't really think it's a big deal, and even if this is a mistake I'm guessing we're in too deep now, just a little thing that caught me by surprise while working on this! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I've noticed the same and agree we're in too deep π On a semi related note, @jcallahan-akamai and I were just recently talking about how we can't see status codes because of our |
||
return ( | ||
Array.isArray(e) && | ||
e.every((item: any) => { | ||
return 'reason' in item; | ||
}) | ||
); | ||
}; | ||
|
||
/** | ||
* Returns `true` if the given error is an Axios error. | ||
* | ||
* Type guards `e` as an `AxiosError` instance. | ||
* | ||
* @param e - Error. | ||
* | ||
* @returns `true` if `e` is an `AxiosError`. | ||
*/ | ||
const isAxiosError = (e: any): e is AxiosError => { | ||
return !!e.isAxiosError; | ||
}; | ||
|
||
/** | ||
* Returns `true` if the given error is a Linode API v4 request error. | ||
* | ||
* Type guards `e` as an `AxiosError<LinodeApiV4Error>` instance. | ||
* | ||
* @param e - Error. | ||
* | ||
* @returns `true` if `e` is a Linode API v4 request error. | ||
*/ | ||
const isLinodeApiError = (e: any): e is AxiosError<LinodeApiV4Error> => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just out of curiosity, do you know why we use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This could have been expressed as |
||
if (isAxiosError(e)) { | ||
const errorData = e.response?.data?.errors; | ||
return ( | ||
Array.isArray(errorData) && | ||
errorData.every((item: any) => { | ||
return 'reason' in item; | ||
}) | ||
); | ||
} | ||
return false; | ||
}; | ||
|
||
/** | ||
* Detects known error types and returns a new Error with more detailed message. | ||
* | ||
* Unknown error types are returned without modification. | ||
* | ||
* @param e - Error. | ||
* | ||
* @returns A new error with added information in message, or `e`. | ||
*/ | ||
const enhanceError = (e: any) => { | ||
// Check for most specific error types first. | ||
if (isLinodeApiError(e)) { | ||
// If `e` is a Linode APIv4 error response, show the status code, error messages, | ||
// and request URL when applicable. | ||
const summary = !!e.response?.status | ||
? `Linode APIv4 request failed with status code ${e.response.status}` | ||
: `Linode APIv4 request failed`; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you think it would be helpful here to add |
||
|
||
const errorDetails = e.response!.data.errors.map((error: APIError) => { | ||
return error.field | ||
? `- ${error.reason} (field '${error.field}')` | ||
: `- ${error.reason}`; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here. Its up to you though, I dont know if the status code and the error field are important, if not, feel free to disregard my comments. |
||
}); | ||
|
||
const requestInfo = | ||
!!e.request?.responseURL && !!e.config.method | ||
? `\nRequest: ${e.config.method.toUpperCase()} ${e.request.responseURL}` | ||
: ''; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. and here |
||
|
||
return new Error(`${summary}\n${errorDetails.join('\n')}${requestInfo}`); | ||
} | ||
|
||
if (isAxiosError(e)) { | ||
// If `e` is an Axios error (but not a Linode API error specifically), show the | ||
// status code, error messages, and request URL when applicable. | ||
const summary = !!e.response?.status | ||
? `Request failed with status code ${e.response.status}` | ||
: `Request failed`; | ||
|
||
const requestInfo = | ||
!!e.request?.responseURL && !!e.config.method | ||
? `\nRequest: ${e.config.method.toUpperCase()} ${e.request.responseURL}` | ||
: ''; | ||
|
||
return new Error(`${summary}${requestInfo}`); | ||
} | ||
|
||
// Handle cases where a validation error is thrown. | ||
// These are arrays containing `APIError` objects; no additional request context | ||
// is included so we only have the validation error messages themselves to work with. | ||
if (isValidationError(e)) { | ||
// Validation errors do not contain any additional context (request URL, payload, etc.). | ||
// Show the validation error messages instead. | ||
const multipleErrors = e.length > 1; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Another way to do this: const errorPlurality = e.length > 1 ? 'errors' : 'error';
const summary = `Request failed with Linode schema validation ${errorPlurality}`; |
||
const summary = multipleErrors | ||
? 'Request failed with Linode schema validation errors' | ||
: 'Request failed with Linode schema validation error'; | ||
|
||
// Format, accounting for 0, 1, or more errors. | ||
const validationErrorMessage = multipleErrors | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since both conditions are pretty similar, another way to do it is: const prefix = multipleErrors ? '- ' : '';
// Format, accounting for 0, 1, or more errors.
const validationErrorMessage = e
.map((error) =>
error.field
? `${prefix}${error.reason} (field '${error.field}')`
: `${prefix}${error.reason}`
)
.join('\n'); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. or tbh adding a hyphen before any error, even if its just one single error might still help readability. If this is the case, you wouldn't need the conditional. Up to you though! |
||
? e | ||
.map((error) => | ||
error.field | ||
? `- ${error.reason} (field '${error.field}')` | ||
: `- ${error.reason}` | ||
) | ||
.join('\n') | ||
: e | ||
.map((error) => | ||
error.field | ||
? `${error.reason} (field '${error.field}')` | ||
: `${error.reason}` | ||
) | ||
.join('\n'); | ||
|
||
return new Error(`${summary}\n${validationErrorMessage}`); | ||
} | ||
// Return `e` unmodified if it's not handled by any of the above cases. | ||
return e; | ||
}; | ||
|
||
/** | ||
* Describes an object which can contain a label. | ||
|
@@ -87,9 +204,9 @@ Cypress.Commands.add( | |
let result: T; | ||
try { | ||
result = await promise; | ||
} catch (e) { | ||
commandLog.end(); | ||
throw e; | ||
} catch (e: any) { | ||
commandLog.error(e); | ||
throw enhanceError(e); | ||
} | ||
commandLog.end(); | ||
return result; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very well documented, makes it easy to follow, thank you!