-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Changes from 21 commits
8e030b5
d67c503
b19abf0
c412b6c
d0804a0
3e553ad
724c15b
39010ee
d40b153
2520e5a
83f3669
b8082f3
02c28f9
09f1ede
3bb74a9
8e6aaef
74c9f19
c1d4037
a6339ba
da1138a
4a9767b
4845f8a
1b59bef
cc79afd
18a08f1
b02fb6d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -58,6 +58,7 @@ export interface CypressRequestOptions extends OptionsWithUrl { | |
cacheable?: boolean | ||
} | ||
|
||
// TODO: migrate to fetch from @cypress/request | ||
const rp = request.defaults((params: CypressRequestOptions, callback) => { | ||
let resp | ||
|
||
|
@@ -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 | ||
|
@@ -298,7 +300,7 @@ type UpdateInstanceArtifactsPayload = { | |
type UpdateInstanceArtifactsOptions = { | ||
runId: string | ||
instanceId: string | ||
timeout: number | undefined | ||
timeout?: number | ||
} | ||
|
||
let preflightResult = { | ||
|
@@ -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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 export changing had the knock-on effect of requiring downstream JS consumers to change their require to There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -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) | ||
|
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' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
} | ||
} | ||
} |
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) | ||
} |
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', | ||
} | ||
} |
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 | ||
}) | ||
}) | ||
} |
There was a problem hiding this comment.
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?