refactor: extract artifact upload process from lib/modes/record.js #29240

cacieprins · 2024-04-01T14:04:14Z

Closes

Additional details

Previously, almost all of artifact uploading logic was handled in a single js file.

This refactor to gain the benefits of Typescript, within the server package:

Extracts upload logic from ./modes/record.js to ./cloud/artifacts/upload_artifacts.ts
Extracts stdout related logic to ./util/print-run.ts
Cleans up type definitions surrounding the necessary data for:
- Uploading artifacts to Cypress Cloud (IArtifact, defined in ./cloud/artifacts/artifact.ts
- Reporting artifact upload results to both stdout and Cypress Cloud (ArtifactUploadResult, defined in ./cloud/artifacts/artifact.ts)
Prepares this section of the codebase for unit testing
Changes the export of cloud/api/index.ts from CommonJS style to ES6 style, to accommodate exported types. This necessitated changing the require declaration in several JS files to reference .default.

Previously, this section of code was only tested via system tests. This PR does not reduce test coverage, and prepares this section of code for unit testing in the future.

Steps to test

Run cypress in record mode, in a project with test replay enabled.

How has the user experience changed?

It should not change.

PR Tasks

[NA] Have tests been added/updated?
[NA] Has a PR for user-facing changes been opened in cypress-documentation?
[NA] Have API changes been updated in the type definitions?

- streamlines the main uploadArtifact fn - extracts artifact specific logic to artifact classes - fully defines types for upload processing and reporting

packages/server/lib/cloud/artifacts/screenshot_artifact.ts

cacieprins · 2024-04-01T15:31:03Z

packages/server/lib/cloud/protocol.ts

    specAccess?: ReturnType<AppCaptureProtocolInterface['getDbMetadata']>
-  } | void> {
+  } | undefined> {
    if (!this._protocol || !filePath || !this._db) {


this codepath will probably go away with improved error messaging - should never get to this point from upload_artifacts if there is no capture artifact to upload

cacieprins · 2024-04-01T15:31:53Z

packages/server/lib/cloud/upload/send_file.ts

+const rp = require('@cypress/request-promise')
+const { fs } = require('../../util/fs')
+
+export const sendFile = (filePath: string, uploadUrl: string) => {


The only major change for this file other than renaming & moving was to change the export to a named export, rather than an assigned export object

cacieprins · 2024-04-01T15:36:13Z

packages/server/lib/cloud/api/index.ts

+// Potential todos: Refactor to named exports, refactor away from `this.` in exports,
+// move individual exports to their own files & convert this to barrelfile
+
+export default {


Since being able to export types was fairly important here, the main export had to change. Since several methods on this export reference other exported methods via this, this is only an intermediary step. Extracting these methods to their own files is out of scope for this refactor.

This export changing had the knock-on effect of requiring downstream JS consumers to change their require to ('/api').default.

should we make issues for the todo comments here for the future?

…throw if errors are encountered

cypress · 2024-04-02T17:36:44Z

6 flaky tests on run #54881 ↗︎

0	29135	1326	0	6

Details:

Merge branch 'develop' into cacie/refactor/artifact-upload-to-ts
Project: cypress	Commit: `b02fb6dc55`
Status: Passed	Duration: 18:31 💡
Started: Apr 10, 2024 9:21 PM	Ended: Apr 10, 2024 9:40 PM

top-nav.cy.ts • 1 flaky test • app-e2e

View Output

Test		Artifacts
... > dismisses the banner for a specified time		`Test Replay` `Screenshots`

scaffold-component-testing.cy.ts • 1 flaky test • launchpad-e2e

View Output

Test		Artifacts
scaffolding component testing > create-react-app > scaffolds component testing for Create React App v5 project		`Test Replay` `Screenshots`

commands/net_stubbing.cy.ts • 3 flaky tests • 5x-driver-webkit

View Output

Test Artifacts

network stubbing > intercepting request > can delay and throttle a StaticResponse

    </td>
  </tr>
  <tr>
    <td colspan="2">
      <a href="https://cloud.cypress.io/projects/ypt4pf/runs/54881/overview/82aef647-a9e3-4e23-9c0f-1150c75ec60e?reviewViewBy=FLAKY&utm_source=github&utm_medium=flaky&utm_campaign=view%20test">
        ... > with `resourceType` > can match a proxied image request by resourceType
      </a>
    </td>
    <td>
      
    </td>
  </tr>
  <tr>
    <td colspan="2">
      <a href="https://cloud.cypress.io/projects/ypt4pf/runs/54881/overview/d1e84fa6-b3c6-45a0-b8b1-3bd016514aaf?reviewViewBy=FLAKY&utm_source=github&utm_medium=flaky&utm_campaign=view%20test">
        ... > stops waiting when an xhr request is canceled
      </a>
    </td>
    <td>
      
    </td>
  </tr></table>

commands/waiting.cy.js • 1 flaky test • 5x-driver-webkit

View Output

Test		Artifacts
... > errors > throws waiting for the 3rd response		`</td> </tr></table>` Review all test suite changes for PR #29240 ↗︎

packages/server/lib/cloud/api/index.ts

packages/server/lib/cloud/artifacts/upload_artifacts.ts

packages/server/lib/cloud/artifacts/protocol_artifact.ts

packages/server/lib/cloud/artifacts/upload_artifacts.ts

…accessor in protocol fatal error report

ryanthemanuel

A few minor tweaks, but overall this looks good. A nice shift for part of the code base in the right direction.

AtofStryker · 2024-04-03T21:44:55Z

packages/server/lib/cloud/api/index.ts

@@ -58,6 +58,7 @@ export interface CypressRequestOptions extends OptionsWithUrl {
  cacheable?: boolean
 }

+// TODO: migrate to fetch from @cypress/request


Do we have an issue for this?

AtofStryker · 2024-04-03T21:45:36Z

packages/server/lib/cloud/api/index.ts

+// Potential todos: Refactor to named exports, refactor away from `this.` in exports,
+// move individual exports to their own files & convert this to barrelfile
+
+export default {


should we make issues for the todo comments here for the future?

AtofStryker · 2024-04-05T17:29:31Z

packages/server/lib/cloud/artifacts/artifact.ts

+}
+
+export interface IArtifact {
+  reportKey: 'video' | 'screenshots' | 'protocol'


we could refactor this to an enum and reference the enum strings in Artifact constructor but I don't think its necessary. For readability though I would love if the reportkey enum strings were in the same order here as in the Artifact class

If only ts enums were actually typesafe - I may convert to a more typesafe object with keyof type

probs the object with typesafefy vs an actual ts enum is better

packages/server/lib/cloud/artifacts/screenshot_artifact.ts

…tifact kinds

cypress-bot · 2024-04-18T23:17:43Z

Released in 13.8.0.

This comment thread has been locked. If you are still experiencing this issue after upgrading to
Cypress v13.8.0, please open a new issue.

cacieprins added 2 commits April 1, 2024 10:12

refactor record.js to extract upload logic into ts

8e030b5

- streamlines the main uploadArtifact fn - extracts artifact specific logic to artifact classes - fully defines types for upload processing and reporting

tweak refactors so system tests produce same snapshots

d67c503

cacieprins force-pushed the cacie/refactor/artifact-upload-to-ts branch from 5f4c57d to d67c503 Compare April 1, 2024 14:13

cacieprins and others added 3 commits April 1, 2024 10:40

some todos, fixes exports/imports from api/index.ts

b19abf0

fix api export so it can be imported by ts files

c412b6c

Merge branch 'develop' into cacie/refactor/artifact-upload-to-ts

d0804a0

cacieprins changed the title ~~Cacie/refactor/artifact upload to ts~~ refactor: extract artifact upload process from lib/modes/record.js Apr 1, 2024

cacieprins commented Apr 1, 2024

View reviewed changes

packages/server/lib/cloud/artifacts/screenshot_artifact.ts Outdated Show resolved Hide resolved

cacieprins commented Apr 1, 2024

View reviewed changes

cacieprins and others added 7 commits April 1, 2024 11:58

cleans up types

3e553ad

extracting artifact metadata from options logs to debug but does not …

724c15b

…throw if errors are encountered

Merge branch 'develop' into cacie/refactor/artifact-upload-to-ts

39010ee

fix type imports in print-run

d40b153

fix debug formatting for artifacts

2520e5a

fix reporting successful protocol uploads

83f3669

Merge branch 'develop' into cacie/refactor/artifact-upload-to-ts

b8082f3

cacieprins marked this pull request as ready for review April 2, 2024 14:18

cacieprins and others added 2 commits April 2, 2024 12:47

change inheritence to strategy

02c28f9

Merge branch 'develop' into cacie/refactor/artifact-upload-to-ts

09f1ede

cacieprins and others added 3 commits April 2, 2024 14:27

rm empty file

3bb74a9

Merge branch 'develop' into cacie/refactor/artifact-upload-to-ts

8e6aaef

Merge branch 'develop' into cacie/refactor/artifact-upload-to-ts

74c9f19

cacieprins requested review from ryanthemanuel and AtofStryker April 3, 2024 15:32