fix: non-JSON response body display for upload errors (#29801)

* WIP - system test snapshots * bolster error output for response body potential on upload network errors * edit error output for readability * changelog * Update packages/server/lib/cloud/artifacts/upload_artifacts.ts Co-authored-by: Matt Schile <mschile@cypress.io> * fix partial rename * fix system test for single upload error vs aggregate --------- Co-authored-by: Matt Schile <mschile@cypress.io> Co-authored-by: Jennifer Shehane <jennifer@cypress.io>
cypress-io · Jul 12, 2024 · 3c28109 · 3c28109 · cypress-bot · Jul 12, 2024
1 parent 807b03d
commit 3c28109
Show file tree

Hide file tree

Showing 11 changed files with 196 additions and 107 deletions.
diff --git a/cli/CHANGELOG.md b/cli/CHANGELOG.md
@@ -5,6 +5,7 @@ _Released 7/16/2024 (PENDING)_
 
 **Bugfixes:**
 
+- CLI output properly displays non-JSON response bodies when a Test Replay upload attempt returns a non-JSON response body for a non-200 status code. Addressed in [#29801](https://github.com/cypress-io/cypress/pull/29801).
 - Fixed an issue where the ReadStream used to upload a Test Replay recording could erroneously be re-used when retrying in cases of retryable upload failures. Fixes [#29227](https://github.com/cypress-io/cypress/issues/29227).
 - Fixed an issue where command snapshots were not being captured within the `cy.origin()` command within Test Replay. Addressed in [#29828](https://github.com/cypress-io/cypress/pull/29828).
 

diff --git a/...es/errors/__snapshot-html__/CLOUD_PROTOCOL_UPLOAD_AGGREGATE_ERROR - withNetworkError.html b/...es/errors/__snapshot-html__/CLOUD_PROTOCOL_UPLOAD_AGGREGATE_ERROR - withNetworkError.html
diff --git a/packages/errors/__snapshot-html__/CLOUD_PROTOCOL_UPLOAD_AGGREGATE_ERROR.html b/packages/errors/__snapshot-html__/CLOUD_PROTOCOL_UPLOAD_AGGREGATE_ERROR.html
diff --git a/packages/errors/__snapshot-html__/CLOUD_PROTOCOL_UPLOAD_HTTP_FAILURE.html b/packages/errors/__snapshot-html__/CLOUD_PROTOCOL_UPLOAD_HTTP_FAILURE.html
diff --git a/packages/errors/src/errors.ts b/packages/errors/src/errors.ts
@@ -574,15 +574,17 @@ export const AllCypressErrors = {
         This error will not affect or change the exit code.
     `
   },
-  CLOUD_PROTOCOL_UPLOAD_HTTP_FAILURE: (error: Error & { url: string, status: number, statusText: string }) => {
+  CLOUD_PROTOCOL_UPLOAD_HTTP_FAILURE: (error: Error & { url: string, status: number, statusText: string, responseBody: string }) => {
     return errTemplate`\
         Warning: We encountered an HTTP error while uploading the Test Replay recording for this spec.
 
         These results will not display Test Replay recordings.
 
         This error will not affect or change the exit code.
 
-        ${fmt.url(error.url)} responded with HTTP ${fmt.stringify(error.status)}: ${fmt.highlightSecondary(error.statusText)}`
+        ${fmt.url(error.url)} responded with HTTP ${fmt.stringify(error.status)}: ${fmt.highlightSecondary(error.statusText)}
+        
+        ${fmt.highlightTertiary(error.responseBody)}`
   },
   CLOUD_PROTOCOL_UPLOAD_NEWORK_FAILURE: (error: Error & { url: string }) => {
     return errTemplate`\
@@ -597,29 +599,35 @@ export const AllCypressErrors = {
         ${fmt.highlightSecondary(error)}`
   },
   CLOUD_PROTOCOL_UPLOAD_AGGREGATE_ERROR: (error: {
-    errors: (Error & { kind?: 'NetworkError' | 'HttpError', url: string })[]
+    errors: (Error & { kind?: 'NetworkError', url: string } | Error & { kind: 'HttpError', url: string, status?: string, statusText?: string, responseBody?: string })[]
   }) => {
     if (error.errors.length === 1) {
-      if (error.errors[0]?.kind === 'NetworkError') {
-        return AllCypressErrors.CLOUD_PROTOCOL_UPLOAD_NEWORK_FAILURE(error.errors[0])
+      const firstError = error.errors[0]
+
+      if (firstError?.kind === 'NetworkError') {
+        return AllCypressErrors.CLOUD_PROTOCOL_UPLOAD_NEWORK_FAILURE(firstError as Error & { url: string })
       }
 
-      return AllCypressErrors.CLOUD_PROTOCOL_UPLOAD_HTTP_FAILURE(error.errors[0] as Error & { url: string, status: number, statusText: string})
+      return AllCypressErrors.CLOUD_PROTOCOL_UPLOAD_HTTP_FAILURE(error.errors[0] as Error & { url: string, status: number, statusText: string, responseBody: string})
     }
 
     let networkErr = error.errors.find((err) => {
       return err.kind === 'NetworkError'
     })
     const recommendation = networkErr ? errPartial`Some or all of the errors encountered are system-level network errors. Please verify your network configuration for connecting to ${fmt.highlightSecondary(networkErr.url)}` : null
 
+    const fmtUploadError = ({ message, responseBody }: { message: string, responseBody?: string }) => {
+      return `${message}${responseBody ? `:\n${responseBody}\n` : ''}`
+    }
+
     return errTemplate`\
         Warning: We encountered multiple errors while uploading the Test Replay recording for this spec.
 
         We attempted to upload the Test Replay recording ${fmt.stringify(error.errors.length)} times.
 
         ${recommendation}
 
-        ${fmt.listItems(error.errors.map((error) => error.message))}`
+        ${fmt.listItems(error.errors.map(fmtUploadError), { prefix: '' })}`
   },
   CLOUD_CANNOT_CREATE_RUN_OR_INSTANCE: (apiErr: Error) => {
     return errTemplate`\

diff --git a/packages/errors/test/unit/visualSnapshotErrors_spec.ts b/packages/errors/test/unit/visualSnapshotErrors_spec.ts
@@ -674,11 +674,12 @@ describe('visual error templates', () => {
     },
     CLOUD_PROTOCOL_UPLOAD_HTTP_FAILURE: () => {
       // @ts-expect-error
-      const err: Error & { status: number, statusText: string, url: string } = makeErr()
+      const err: Error & { status: number, statusText: string, url: string, message: string, responseBody: string } = makeErr()
 
       err.status = 500
       err.statusText = 'Internal Server Error'
       err.url = 'https://some/url'
+      err.responseBody = '{ status: 500, reason: \'unknown\'}'
 
       return {
         default: [err],

diff --git a/packages/server/lib/cloud/artifacts/upload_artifacts.ts b/packages/server/lib/cloud/artifacts/upload_artifacts.ts
@@ -16,6 +16,14 @@ import { NetworkError } from '../network/network_error'
 
 const debug = Debug('cypress:server:cloud:artifacts')
 
+const removeWhitespaceAndTrim = (str: string) => {
+  return str.split(/\n/)
+  .map((line) => {
+    return line.trim()
+  })
+  .join('')
+}
+
 const toUploadReportPayload = (acc: {
   screenshots: ArtifactMetadata[]
   video?: ArtifactMetadata
@@ -27,8 +35,14 @@ const toUploadReportPayload = (acc: {
     let { error, errorStack, allErrors } = reportWithoutOriginalError
 
     if (allErrors) {
-      error = `Failed to upload Test Replay after ${allErrors.length} attempts. Errors: ${allErrors.map((error) => error.message).join(', ')}`
-      errorStack = allErrors.map((error) => error.stack).join(', ')
+      const messages = allErrors.map((error) => {
+        return (HttpError.isHttpError(error) && error.responseBody) ?
+      `${error.message}: ${removeWhitespaceAndTrim(error.responseBody)}` :
+          error.message
+      })
+
+      error = `Failed to upload Test Replay after ${allErrors.length} attempts. Errors: ${messages.join(', ')}`
+      errorStack = allErrors.map((error) => error.stack).join('')
     } else if (error) {
       error = `Failed to upload Test Replay: ${error}`
     }

diff --git a/packages/server/lib/cloud/network/http_error.ts b/packages/server/lib/cloud/network/http_error.ts
@@ -9,6 +9,7 @@ export class HttpError extends Error {
     public readonly url: string,
     public readonly status: number,
     public readonly statusText: string,
+    public readonly responseBody: string,
     public readonly originalResponse: Response,
   ) {
     super(message)
@@ -20,16 +21,16 @@ export class HttpError extends Error {
 
   public static async fromResponse (response: Response): Promise<HttpError> {
     const status = response.status
-    const statusText = await (response.json().catch(() => {
-      return response.statusText
-    }))
+    const statusText = response.statusText
+    const responseBody = await response.text()
     const scrubbedUrl = scrubUrl(response.url)
 
     return new HttpError(
-      `${status} ${statusText} (${scrubUrl(response.url)})`,
+      `${scrubUrl(response.url)} responded with ${status} ${statusText}`,
       scrubbedUrl,
       status,
       statusText,
+      responseBody,
       response,
     )
   }

diff --git a/packages/server/test/unit/cloud/network/is_retryable_error_spec.ts b/packages/server/test/unit/cloud/network/is_retryable_error_spec.ts
@@ -17,15 +17,15 @@ describe('isRetryableError', () => {
 
   it('returns true with retryable http errors', () => {
     [408, 429, 502, 503, 504].forEach((status) => {
-      const err = new HttpError('some error', url, status, 'status text', sinon.createStubInstance(Response))
+      const err = new HttpError('some error', url, status, 'status text', 'response_body', sinon.createStubInstance(Response))
 
       expect(isRetryableError(err)).to.be.true
     })
   })
 
   it('returns false with non-retryable http errors', () => {
     [400, 401, 402, 403, 404, 405, 406, 407, 409, 410, 411, 412, 413, 414, 416, 417, 418, 421, 422, 423, 424, 425, 426, 428, 431, 451, 500, 501, 505, 507, 508, 510, 511].forEach((status) => {
-      const err = new HttpError('some error', url, status, 'status text', sinon.createStubInstance(Response))
+      const err = new HttpError('some error', url, status, 'status text', 'response_body', sinon.createStubInstance(Response))
 
       expect(isRetryableError(err)).to.be.false
     })