Skip to content
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

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions packages/manager/.changeset/pr-9777-tests-1696958372039.md
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))
173 changes: 145 additions & 28 deletions packages/manager/cypress/support/setup/defer-command.ts
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[];
};

/**
Copy link
Contributor

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!

* 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
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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 APIError[] if it is a schema validation error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 AxiosError<{ errors: APIError[] }> whereas schema validation errors are just APIError[] (in the schema validation Before screenshot in the PR description, Cypress even calls out that it's not an Error that was thrown, but an array).

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!

Copy link
Member

Choose a reason for hiding this comment

The 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 APIError[] pattern. 😣

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> => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just out of curiosity, do you know why we use LinodeApiV4Error instead of just using APIError[]?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LinodeApiV4Error is defined towards the top of this file, and it's just an alias for any object with an errors property containing an array of APIErrors.

APIError[] alone isn't applicable here because the error type we're checking for is an Axios error object which contains an HTTP response containing the errors. AxiosError<APIError[]> also isn't applicable since the response body isn't just an array of APIError objects, it's an object itself which contains an errors property.

This could have been expressed as AxiosError<{ errors: APIError[] }>, but I opted to define a type for this within the module because I expected to have to refer to that type a lot (in the end I really didn't).

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`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think it would be helpful here to add Linode APIv4 request failed **without status code** for the false condition just so the user knows there wasn't one so they dont spend time looking for it?


const errorDetails = e.response!.data.errors.map((error: APIError) => {
return error.field
? `- ${error.reason} (field '${error.field}')`
: `- ${error.reason}`;
Copy link
Contributor

Choose a reason for hiding this comment

The 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}`
: '';
Copy link
Contributor

Choose a reason for hiding this comment

The 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;
Copy link
Contributor

Choose a reason for hiding this comment

The 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
Copy link
Contributor

Choose a reason for hiding this comment

The 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');

Copy link
Contributor

Choose a reason for hiding this comment

The 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.
Expand Down Expand Up @@ -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;
Expand Down