test: Improve Cypress error messages when Linode API errors occur by jdamore-linode · Pull Request #9777 · linode/manager

jdamore-linode · 2023-10-10T17:15:12Z

Description 📝

This improves error handling when a Linode API error or validation error occurs during a Cypress test by adding additional information to the command log. This is intended to help troubleshoot test failures.

(Meta: Not sure if this is better suited as test or refactor type, happy to change this to refactor if we think that's more applicable)

Changes 🔄

Detect common error types (Linode API error, generic Axios error, Linode schema validation error) and add additional information to the error message when Promise rejects in cy.defer

Preview 📷

Before	After
Linode API error:

Linode schema validation error:

How to test 🧪

If any of the automated tests fail during a before hook (which has been happening a lot lately), we can rely on the archived screenshots and videos to confirm that this change works.
Otherwise, the easiest thing to do is to force a test to make an invalid request and observe the error using yarn cy:debug:
1. Modify linodeRequest in cypress/support/util/linode-utils.ts (line 10) to contain a short/weak root_pass property
2. Run yarn cy:debug and select rescue-linode.spec.ts in the UI
3. Observe that the API responds with a 400 and confirm that detailed messaging is shown in the Cypress UI

As an Author I have considered 🤔

Check all that apply

jdamore-linode · 2023-10-10T17:17:57Z

packages/manager/cypress/support/setup/defer-command.ts

+  // When a Linode APIv4 schema validation error occurs, an array of `APIError`
+  // objects is thrown rather than a typical `Error` type.


I'm not sure if this is intended, but I think this is the source of the behavior.

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?

@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!

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

…occurs in Cypress test

bnussman-akamai

bnussman-akamai · 2023-10-11T19:16:43Z

packages/manager/cypress/support/setup/defer-command.ts

+  // When a Linode APIv4 schema validation error occurs, an array of `APIError`
+  // objects is thrown rather than a typical `Error` type.


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?

coliu-akamai

Thanks Joe -- error messages were definitely a lot more helpful!

✅ confirmed error message for a weak root_password
✅ confirmed error message for a null root_password

tyler-akamai · 2023-10-12T15:23:20Z

Confirmed the error message looks good!

Great job ✅

tyler-akamai · 2023-10-12T14:29:55Z

packages/manager/cypress/support/setup/defer-command.ts

+  errors: APIError[];
+};
+
+/**


Very well documented, makes it easy to follow, thank you!

tyler-akamai · 2023-10-12T14:44:47Z

packages/manager/cypress/support/setup/defer-command.ts

+ *
+ * @returns `true` if `e` is a Linode API v4 request error.
+ */
+const isLinodeApiError = (e: any): e is AxiosError<LinodeApiV4Error> => {


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

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

tyler-akamai · 2023-10-12T14:48:38Z

packages/manager/cypress/support/setup/defer-command.ts

+    // and request URL when applicable.
+    const summary = !!e.response?.status
+      ? `Linode APIv4 request failed with status code ${e.response.status}`
+      : `Linode APIv4 request failed`;


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?

tyler-akamai · 2023-10-12T14:50:13Z

packages/manager/cypress/support/setup/defer-command.ts

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


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.

tyler-akamai · 2023-10-12T14:51:26Z

packages/manager/cypress/support/setup/defer-command.ts

+    const requestInfo =
+      !!e.request?.responseURL && !!e.config.method
+        ? `\nRequest: ${e.config.method.toUpperCase()} ${e.request.responseURL}`
+        : '';


tyler-akamai · 2023-10-12T14:54:44Z

packages/manager/cypress/support/setup/defer-command.ts

+  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;


Another way to do this:

const errorPlurality = e.length > 1 ? 'errors' : 'error'; const summary = `Request failed with Linode schema validation ${errorPlurality}`;

tyler-akamai · 2023-10-12T14:59:10Z

packages/manager/cypress/support/setup/defer-command.ts

+      : 'Request failed with Linode schema validation error';
+
+    // Format, accounting for 0, 1, or more errors.
+    const validationErrorMessage = multipleErrors


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

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!

Improve error messages when Linode API errors occur

4c01ac6

jdamore-linode added Ready for Review Missing Changeset labels Oct 10, 2023

jdamore-linode self-assigned this Oct 10, 2023

jdamore-linode requested a review from a team as a code owner October 10, 2023 17:15

jdamore-linode requested review from coliu-akamai and tyler-akamai and removed request for a team October 10, 2023 17:15

jdamore-linode commented Oct 10, 2023

View reviewed changes

Added changeset: Improve error message display when Linode API error …

ad53f47

…occurs in Cypress test

jdamore-linode removed the Missing Changeset label Oct 10, 2023

Joe D'Amore added 2 commits October 10, 2023 13:27

Include request method in message

b6a7743

Return an Error instead of a string for validation errors

8d8f415

bnussman-akamai approved these changes Oct 11, 2023

View reviewed changes

coliu-akamai approved these changes Oct 12, 2023

View reviewed changes

tyler-akamai approved these changes Oct 12, 2023

View reviewed changes

jaalah-akamai added the Approved Multiple approvals and ready to merge! label Oct 12, 2023

mjac0bs removed the Ready for Review label Oct 12, 2023

jdamore-linode merged commit cc73646 into linode:develop Oct 16, 2023

This was referenced Oct 26, 2023

Release v1.106.0 - release → staging #9842

Merged

Release v1.106.0 - staging → master #9850

Merged

		// When a Linode APIv4 schema validation error occurs, an array of `APIError`
		// objects is thrown rather than a typical `Error` type.

Conversation

jdamore-linode commented Oct 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description 📝

Changes 🔄

Preview 📷

How to test 🧪

As an Author I have considered 🤔

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnussman-akamai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coliu-akamai left a comment

Choose a reason for hiding this comment

Uh oh!

tyler-akamai commented Oct 12, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jdamore-linode commented Oct 10, 2023 •

edited

Loading