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

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

Merged
merged 26 commits into from
Apr 12, 2024
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
8e030b5
refactor record.js to extract upload logic into ts
cacieprins Mar 27, 2024
d67c503
tweak refactors so system tests produce same snapshots
cacieprins Mar 29, 2024
b19abf0
some todos, fixes exports/imports from api/index.ts
cacieprins Apr 1, 2024
c412b6c
fix api export so it can be imported by ts files
cacieprins Apr 1, 2024
d0804a0
Merge branch 'develop' into cacie/refactor/artifact-upload-to-ts
cacieprins Apr 1, 2024
3e553ad
cleans up types
cacieprins Apr 1, 2024
724c15b
extracting artifact metadata from options logs to debug but does not …
cacieprins Apr 1, 2024
39010ee
Merge branch 'develop' into cacie/refactor/artifact-upload-to-ts
cacieprins Apr 1, 2024
d40b153
fix type imports in print-run
cacieprins Apr 1, 2024
2520e5a
fix debug formatting for artifacts
cacieprins Apr 1, 2024
83f3669
fix reporting successful protocol uploads
cacieprins Apr 1, 2024
b8082f3
Merge branch 'develop' into cacie/refactor/artifact-upload-to-ts
cacieprins Apr 2, 2024
02c28f9
change inheritence to strategy
cacieprins Apr 2, 2024
09f1ede
Merge branch 'develop' into cacie/refactor/artifact-upload-to-ts
cacieprins Apr 2, 2024
3bb74a9
rm empty file
cacieprins Apr 2, 2024
8e6aaef
Merge branch 'develop' into cacie/refactor/artifact-upload-to-ts
cacieprins Apr 2, 2024
74c9f19
Merge branch 'develop' into cacie/refactor/artifact-upload-to-ts
cacieprins Apr 3, 2024
c1d4037
Update packages/server/lib/cloud/artifacts/upload_artifacts.ts
ryanthemanuel Apr 5, 2024
a6339ba
makes protocolManager optional to uploadArtifacts, fixes conditional …
cacieprins Apr 5, 2024
da1138a
Merge branch 'develop' into cacie/refactor/artifact-upload-to-ts
cacieprins Apr 5, 2024
4a9767b
missed a potentially undef
cacieprins Apr 5, 2024
4845f8a
convert to frozen object / keyof instead of string composition for ar…
cacieprins Apr 5, 2024
1b59bef
Merge branch 'develop' into cacie/refactor/artifact-upload-to-ts
ryanthemanuel Apr 9, 2024
cc79afd
Merge branch 'develop' into cacie/refactor/artifact-upload-to-ts
cacieprins Apr 10, 2024
18a08f1
Merge branch 'develop' into cacie/refactor/artifact-upload-to-ts
cacieprins Apr 10, 2024
b02fb6d
Merge branch 'develop' into cacie/refactor/artifact-upload-to-ts
jennifer-shehane Apr 10, 2024
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
27 changes: 17 additions & 10 deletions packages/server/lib/cloud/api/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ export interface CypressRequestOptions extends OptionsWithUrl {
cacheable?: boolean
}

// TODO: migrate to fetch from @cypress/request
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have an issue for this?

const rp = request.defaults((params: CypressRequestOptions, callback) => {
let resp

Expand Down Expand Up @@ -274,22 +275,23 @@ type CreateRunResponse = {
} | undefined
}

type ArtifactMetadata = {
export type ArtifactMetadata = {
url: string
fileSize?: number
fileSize?: number | bigint
uploadDuration?: number
success: boolean
error?: string
errorStack?: string
}

type ProtocolMetadata = ArtifactMetadata & {
export type ProtocolMetadata = ArtifactMetadata & {
specAccess?: {
size: bigint
offset: bigint
size: number
cacieprins marked this conversation as resolved.
Show resolved Hide resolved
offset: number
}
}

type UpdateInstanceArtifactsPayload = {
export type UpdateInstanceArtifactsPayload = {
screenshots: ArtifactMetadata[]
video?: ArtifactMetadata
protocol?: ProtocolMetadata
Expand All @@ -298,7 +300,7 @@ type UpdateInstanceArtifactsPayload = {
type UpdateInstanceArtifactsOptions = {
runId: string
instanceId: string
timeout: number | undefined
timeout?: number
}

let preflightResult = {
Expand All @@ -307,7 +309,10 @@ let preflightResult = {

let recordRoutes = apiRoutes

module.exports = {
// 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 {
Copy link
Contributor Author

@cacieprins cacieprins Apr 1, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

rp,

// For internal testing
Expand Down Expand Up @@ -400,8 +405,10 @@ module.exports = {
let script

try {
if (captureProtocolUrl || process.env.CYPRESS_LOCAL_PROTOCOL_PATH) {
script = await this.getCaptureProtocolScript(captureProtocolUrl || process.env.CYPRESS_LOCAL_PROTOCOL_PATH)
const protocolUrl = captureProtocolUrl || process.env.CYPRESS_LOCAL_PROTOCOL_PATH

if (protocolUrl) {
script = await this.getCaptureProtocolScript(protocolUrl)
}
} catch (e) {
debugProtocol('Error downloading capture code', e)
Expand Down
105 changes: 105 additions & 0 deletions packages/server/lib/cloud/artifacts/artifact.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
import Debug from 'debug'
import { performance } from 'perf_hooks'

const debug = Debug('cypress:server:cloud:artifact')

const isAggregateError = (err: any): err is AggregateError => {
return !!err.errors
}

export interface IArtifact {
reportKey: 'video' | 'screenshots' | 'protocol'
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

uploadUrl: string
filePath: string
fileSize: number | bigint
upload: () => Promise<ArtifactUploadResult>
}

export interface ArtifactUploadResult {
success: boolean
error?: Error | string
url: string
pathToFile: string
fileSize?: number | bigint
key: IArtifact['reportKey']
errorStack?: string
allErrors?: Error[]
specAccess?: {
offset: number
size: number
}
uploadDuration?: number
}

export type ArtifactUploadStrategy<T> = (filePath: string, uploadUrl: string, fileSize: number | bigint) => T

export class Artifact<T extends ArtifactUploadStrategy<UploadResponse>, UploadResponse extends Promise<any> = Promise<{}>> {
constructor (
public reportKey: 'protocol' | 'screenshots' | 'video',
public readonly filePath: string,
public readonly uploadUrl: string,
public readonly fileSize: number | bigint,
private uploadStrategy: T,
) {
}

public async upload (): Promise<ArtifactUploadResult> {
const startTime = performance.now()

this.debug('upload starting')

try {
const response = await this.uploadStrategy(this.filePath, this.uploadUrl, this.fileSize)

this.debug('upload succeeded: %O', response)

return this.composeSuccessResult(response ?? {}, performance.now() - startTime)
} catch (e) {
this.debug('upload failed: %O', e)

return this.composeFailureResult(e, performance.now() - startTime)
}
}

private debug (formatter: string = '', ...args: (string | object | number)[]) {
if (!debug.enabled) return

debug(`%s: %s -> %s (%dB) ${formatter}`, this.reportKey, this.filePath, this.uploadUrl, this.fileSize, ...args)
}

private commonResultFields (): Pick<ArtifactUploadResult, 'url' | 'pathToFile' | 'fileSize' | 'key'> {
return {
key: this.reportKey,
url: this.uploadUrl,
pathToFile: this.filePath,
fileSize: this.fileSize,
}
}

protected composeSuccessResult<T extends Object = {}> (response: T, uploadDuration: number): ArtifactUploadResult {
return {
...response,
...this.commonResultFields(),
success: true,
uploadDuration,
}
}

protected composeFailureResult<T extends Error> (err: T, uploadDuration: number): ArtifactUploadResult {
const errorReport = isAggregateError(err) ? {
error: err.errors[err.errors.length - 1].message,
errorStack: err.errors[err.errors.length - 1].stack,
allErrors: err.errors,
} : {
error: err.message,
errorStack: err.stack,
}

return {
...errorReport,
...this.commonResultFields(),
success: false,
uploadDuration,
}
}
}
6 changes: 6 additions & 0 deletions packages/server/lib/cloud/artifacts/file_upload_strategy.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
import { sendFile } from '../upload/send_file'
import type { ArtifactUploadStrategy } from './artifact'

export const fileUploadStrategy: ArtifactUploadStrategy<Promise<any>> = (filePath, uploadUrl) => {
return sendFile(filePath, uploadUrl)
}
61 changes: 61 additions & 0 deletions packages/server/lib/cloud/artifacts/protocol_artifact.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
import fs from 'fs/promises'
import type { ProtocolManager } from '../protocol'
import { IArtifact, ArtifactUploadStrategy, ArtifactUploadResult, Artifact } from './artifact'

interface ProtocolUploadStrategyResult {
success: boolean
fileSize: number | bigint
specAccess: {
offset: number
size: number
}
}

const createProtocolUploadStrategy = (protocolManager: ProtocolManager) => {
const strategy: ArtifactUploadStrategy<Promise<ProtocolUploadStrategyResult | {}>> =
async (filePath, uploadUrl, fileSize) => {
const fatalError = protocolManager.getFatalError()

if (fatalError) {
throw fatalError.error
}

const res = await protocolManager.uploadCaptureArtifact({ uploadUrl, fileSize, filePath })

return res ?? {}
}

return strategy
}

export const createProtocolArtifact = async (filePath: string, uploadUrl: string, protocolManager: ProtocolManager): Promise<IArtifact> => {
const { size } = await fs.stat(filePath)

return new Artifact('protocol', filePath, uploadUrl, size, createProtocolUploadStrategy(protocolManager))
}

export const composeProtocolErrorReportFromOptions = async ({
protocolManager,
protocolCaptureMeta,
captureUploadUrl,
}: {
protocolManager?: ProtocolManager
protocolCaptureMeta: { url?: string, disabledMessage?: string }
captureUploadUrl?: string
}): Promise<ArtifactUploadResult> => {
const url = captureUploadUrl || protocolCaptureMeta.url
const pathToFile = protocolManager?.getArchivePath()
const fileSize = pathToFile ? (await fs.stat(pathToFile))?.size : 0

const fatalError = protocolManager?.getFatalError()

return {
key: 'protocol',
url: url ?? 'UNKNOWN',
pathToFile: pathToFile ?? 'UNKNOWN',
fileSize,
success: false,
error: fatalError?.error.message || 'UNKNOWN',
errorStack: fatalError?.error.stack || 'UNKNOWN',
}
}
44 changes: 44 additions & 0 deletions packages/server/lib/cloud/artifacts/screenshot_artifact.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
import fs from 'fs/promises'
import Debug from 'debug'
import { Artifact, IArtifact } from './artifact'
import { fileUploadStrategy } from './file_upload_strategy'

const debug = Debug('cypress:server:cloud:artifacts:screenshot')

const createScreenshotArtifact = async (filePath: string, uploadUrl: string): Promise<IArtifact | undefined> => {
try {
const { size } = await fs.stat(filePath)

return new Artifact('screenshots', filePath, uploadUrl, size, fileUploadStrategy)
} catch (e) {
debug('Error creating screenshot artifact: %O', e)

return
}
}

export const createScreenshotArtifactBatch = (
screenshotUploadUrls: {screenshotId: string, uploadUrl: string}[],
screenshotFiles: {screenshotId: string, path: string}[],
): Promise<IArtifact[]> => {
const correlatedPaths = screenshotUploadUrls.map(({ screenshotId, uploadUrl }) => {
const correlatedFilePath = screenshotFiles.find((pathPair) => {
return pathPair.screenshotId === screenshotId
})?.path

return correlatedFilePath ? {
filePath: correlatedFilePath,
uploadUrl,
} : undefined
}).filter((pair): pair is { filePath: string, uploadUrl: string } => {
AtofStryker marked this conversation as resolved.
Show resolved Hide resolved
return !!pair
})

return Promise.all(correlatedPaths.map(({ filePath, uploadUrl }) => {
return createScreenshotArtifact(filePath, uploadUrl)
})).then((artifacts) => {
return artifacts.filter((artifact): artifact is IArtifact => {
return !!artifact
})
})
}
Loading
Loading