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

feat: add alpha support for PWT snapshot testing #876

Merged
merged 9 commits into from
Nov 6, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
28 changes: 26 additions & 2 deletions packages/cli/src/commands/deploy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,13 @@ import type { Runtime } from '../rest/runtimes'
import {
Check, AlertChannelSubscription, AlertChannel, CheckGroup, Dashboard,
MaintenanceWindow, PrivateLocation, PrivateLocationCheckAssignment, PrivateLocationGroupAssignment,
Project, ProjectData,
Project, ProjectData, BrowserCheck,
} from '../constructs'
import chalk from 'chalk'
import { splitConfigFilePath, getGitInformation } from '../services/util'
import commonMessages from '../messages/common-messages'
import { ProjectDeployResponse } from '../rest/projects'
import { uploadSnapshots } from '../services/snapshot-service'

// eslint-disable-next-line no-restricted-syntax
enum ResourceDeployStatus {
Expand Down Expand Up @@ -54,12 +55,26 @@ export default class Deploy extends AuthCommand {
char: 'c',
description: commonMessages.configFile,
}),
'update-snapshots': Flags.boolean({
char: 'u',
description: 'Update any snapshots using the actual result of this test run.',
default: false,
// Mark --update-snapshots as hidden until we're ready for GA
hidden: true,
}),
}

async run (): Promise<void> {
ux.action.start('Parsing your project', undefined, { stdout: true })
const { flags } = await this.parse(Deploy)
const { force, preview, 'schedule-on-deploy': scheduleOnDeploy, output, config: configFilename } = flags
const {
force,
preview,
'schedule-on-deploy': scheduleOnDeploy,
output,
config: configFilename,
'update-snapshots': updateSnapshots,
} = flags
const { configDirectory, configFilenames } = splitConfigFilePath(configFilename)
const {
config: checklyConfig,
Expand All @@ -85,6 +100,15 @@ export default class Deploy extends AuthCommand {
const repoInfo = getGitInformation(project.repoUrl)
ux.action.stop()

if (!preview && updateSnapshots) {
for (const check of Object.values(project.data.check)) {
if (!(check instanceof BrowserCheck)) {
continue
}
check.snapshots = await uploadSnapshots(check.rawSnapshots)
}
}

const projectPayload = project.synthesize(false)
if (!projectPayload.resources.length) {
if (preview) {
Expand Down
11 changes: 11 additions & 0 deletions packages/cli/src/commands/test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,13 @@
char: 'n',
description: 'A name to use when storing results in Checkly with --record.',
}),
'update-snapshots': Flags.boolean({
clample marked this conversation as resolved.
Show resolved Hide resolved
char: 'u',
description: 'Update any snapshots using the actual result of this test run.',
default: false,
// Mark --update-snapshots as hidden until we're ready for GA
hidden: true,
}),
}

static args = {
Expand Down Expand Up @@ -125,6 +132,7 @@
config: configFilename,
record: shouldRecord,
'test-session-name': testSessionName,
'update-snapshots': updateSnapshots,
} = flags
const filePatterns = argv as string[]

Expand Down Expand Up @@ -218,6 +226,8 @@
shouldRecord,
repoInfo,
ciInfo.environment,
updateSnapshots,
configDirectory,
)

runner.on(Events.RUN_STARTED,
Expand All @@ -237,6 +247,7 @@
if (result.hasFailures) {
process.exitCode = 1
}

reporters.forEach(r => r.onCheckEnd(checkRunId, {
logicalId: check.logicalId,
sourceFile: check.getSourceFile(),
Expand Down Expand Up @@ -317,7 +328,7 @@
// Sort and print the checks in a way that's consistent with AbstractListReporter
const sortedCheckFiles = [...new Set(checks.map((check) => check.getSourceFile()))].sort()
const sortedChecks = checks.sort((a, b) => a.name.localeCompare(b.name))
const checkFilesMap: Map<string, Array<Check>> = new Map(sortedCheckFiles.map((file) => [file!, []]))

Check warning on line 331 in packages/cli/src/commands/test.ts

View workflow job for this annotation

GitHub Actions / lint

Forbidden non-null assertion
sortedChecks.forEach(check => {
checkFilesMap.get(check.getSourceFile()!)!.push(check)
})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ describe('BrowserCheck', () => {
const bundle = BrowserCheck.bundle(getFilePath('entrypoint.js'), '2022.10')
delete Session.basePath

expect(bundle).toEqual({
expect(bundle).toMatchObject({
script: fs.readFileSync(getFilePath('entrypoint.js')).toString(),
scriptPath: 'fixtures/browser-check/entrypoint.js',
dependencies: [
Expand Down
9 changes: 9 additions & 0 deletions packages/cli/src/constructs/browser-check.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { Parser } from '../services/check-parser/parser'
import { CheckConfigDefaults } from '../services/checkly-config-loader'
import { pathToPosix } from '../services/util'
import { Content, Entrypoint } from './construct'
import { detectSnapshots, Snapshot } from '../services/snapshot-service'

export interface CheckDependency {
path: string
Expand Down Expand Up @@ -37,6 +38,11 @@ export class BrowserCheck extends Check {
dependencies?: Array<CheckDependency>
sslCheckDomain?: string

// For snapshots, we first store `rawSnapshots` with the path to the file.
// The `snapshots` field is set later (with a `key`) after these are uploaded to storage.
rawSnapshots?: Array<{ absolutePath: string, path: string }>
snapshots?: Array<Snapshot>

/**
* Constructs the Browser Check instance
*
Expand Down Expand Up @@ -73,6 +79,7 @@ export class BrowserCheck extends Check {
this.script = bundle.script
this.scriptPath = bundle.scriptPath
this.dependencies = bundle.dependencies
this.rawSnapshots = bundle.snapshots
} else {
throw new Error('Unrecognized type for the "code" property. The "code" property should be a string of JS/TS code.')
}
Expand Down Expand Up @@ -120,6 +127,7 @@ export class BrowserCheck extends Check {
script: parsed.entrypoint.content,
scriptPath: pathToPosix(path.relative(Session.basePath!, parsed.entrypoint.filePath)),
dependencies: deps,
snapshots: detectSnapshots(Session.basePath!, parsed.entrypoint.filePath),
}
}

Expand All @@ -135,6 +143,7 @@ export class BrowserCheck extends Check {
scriptPath: this.scriptPath,
dependencies: this.dependencies,
sslCheckDomain: this.sslCheckDomain || null, // empty string is converted to null
snapshots: this.snapshots,
}
}
}
2 changes: 2 additions & 0 deletions packages/cli/src/rest/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import Locations from './locations'
import TestSessions from './test-sessions'
import EnvironmentVariables from './environment-variables'
import HeartbeatChecks from './heartbeat-checks'
import ChecklyStorage from './checkly-storage'

export function getDefaults () {
const apiKey = config.getApiKey()
Expand Down Expand Up @@ -98,3 +99,4 @@ export const privateLocations = new PrivateLocations(api)
export const testSessions = new TestSessions(api)
export const environmentVariables = new EnvironmentVariables(api)
export const heartbeatCheck = new HeartbeatChecks(api)
export const checklyStorage = new ChecklyStorage(api)
23 changes: 23 additions & 0 deletions packages/cli/src/rest/checkly-storage.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import type { AxiosInstance } from 'axios'
import type { Readable } from 'node:stream'

class ChecklyStorage {
api: AxiosInstance
constructor (api: AxiosInstance) {
this.api = api
}

upload (stream: Readable) {
return this.api.post<{ key: string }>(
'/next/checkly-storage/upload',
stream,
{ headers: { 'Content-Type': 'application/octet-stream' } },
)
}

download (key: string) {
return this.api.post('/next/checkly-storage/download', { key }, { responseType: 'stream' })
}
}

export default ChecklyStorage
28 changes: 15 additions & 13 deletions packages/cli/src/services/abstract-check-runner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -141,20 +141,8 @@ export default abstract class AbstractCheckRunner extends EventEmitter {
} else if (subtopic === 'run-end') {
this.disableTimeout(checkRunId)
const { result } = message
const {
region,
logPath,
checkRunDataPath,
} = result.assets
if (logPath && (this.verbose || result.hasFailures)) {
result.logs = await assets.getLogs(region, logPath)
}
if (checkRunDataPath && (this.verbose || result.hasFailures)) {
result.checkRunData = await assets.getCheckRunData(region, checkRunDataPath)
}

await this.processCheckResult(result)
const links = testResultId && result.hasFailures && await this.getShortLinks(testResultId)

this.emit(Events.CHECK_SUCCESSFUL, checkRunId, check, result, links)
this.emit(Events.CHECK_FINISHED, check)
} else if (subtopic === 'error') {
Expand All @@ -164,6 +152,20 @@ export default abstract class AbstractCheckRunner extends EventEmitter {
}
}

async processCheckResult (result: any) {
const {
region,
logPath,
checkRunDataPath,
} = result.assets
if (logPath && (this.verbose || result.hasFailures)) {
result.logs = await assets.getLogs(region, logPath)
}
if (checkRunDataPath && (this.verbose || result.hasFailures)) {
result.checkRunData = await assets.getCheckRunData(region, checkRunDataPath)
}
}

private allChecksFinished (): Promise<void> {
let finishedCheckCount = 0
const numChecks = this.checks.size
Expand Down
64 changes: 64 additions & 0 deletions packages/cli/src/services/snapshot-service.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
import * as fsAsync from 'node:fs/promises'
import * as fs from 'node:fs'
import * as path from 'node:path'
import * as stream from 'node:stream/promises'

import { checklyStorage } from '../rest/api'
import { findFilesRecursively, pathToPosix } from './util'

export interface Snapshot {
key: string,
path: string,
}

export async function pullSnapshots (basePath: string, snapshots?: Snapshot[] | null) {
if (!snapshots?.length) {
return
}

try {
for (const snapshot of snapshots) {
const fullPath = path.resolve(basePath, snapshot.path)
if (!fullPath.startsWith(basePath)) {
// The snapshot file should always be within the project, but we validate this just in case.
throw new Error(`Detected invalid snapshot file ${fullPath}`)
}
await fsAsync.mkdir(path.dirname(fullPath), { recursive: true })
const fileStream = fs.createWriteStream(fullPath)
const { data: contentStream } = await checklyStorage.download(snapshot.key)
contentStream.pipe(fileStream)
await stream.finished(contentStream)
}
} catch (err: any) {
throw new Error(`Error downloading snapshots: ${err.message}`)
}
}

export function detectSnapshots (projectBasePath: string, scriptFilePath: string) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

TODO:

Playwright is able to maintain different snapshot files for different platforms (f.ex *-chromium-linux.png for Chromium Linux, which is what we use in our runtimes).

This current implementation will detect all of the snapshots, though, rather than just pushing the Chromium Linux ones.

// By default, PWT will store snapshots in the `script.spec.js-snapshots` directory.
// Other paths can be configured, though, and we should add support for those as well.
// https://playwright.dev/docs/api/class-testconfig#test-config-snapshot-path-template
const snapshotFiles = findFilesRecursively(`${scriptFilePath}-snapshots`)
return snapshotFiles.map(absolutePath => ({
absolutePath,
path: pathToPosix(path.relative(projectBasePath, absolutePath)),
}))
}

export async function uploadSnapshots (rawSnapshots?: Array<{ absolutePath: string, path: string }>) {
if (!rawSnapshots?.length) {
return []
}

try {
const snapshots: Array<Snapshot> = []
for (const rawSnapshot of rawSnapshots) {
const snapshotStream = fs.createReadStream(rawSnapshot.absolutePath)
const { data: { key } } = await checklyStorage.upload(snapshotStream)
snapshots.push({ key, path: rawSnapshot.path })
}
return snapshots
} catch (err: any) {
throw new Error(`Error uploading snapshots: ${err.message}`)
}
}
16 changes: 15 additions & 1 deletion packages/cli/src/services/test-runner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import AbstractCheckRunner, { RunLocation, CheckRunId } from './abstract-check-r
import { GitInformation } from './util'
import { Check } from '../constructs/check'
import { Project } from '../constructs'
import { pullSnapshots } from '../services/snapshot-service'

import * as uuid from 'uuid'

Expand All @@ -13,6 +14,8 @@ export default class TestRunner extends AbstractCheckRunner {
shouldRecord: boolean
repoInfo: GitInformation | null
environment: string | null
updateSnapshots: boolean
baseDirectory: string
constructor (
accountId: string,
project: Project,
Expand All @@ -23,6 +26,8 @@ export default class TestRunner extends AbstractCheckRunner {
shouldRecord: boolean,
repoInfo: GitInformation | null,
environment: string | null,
updateSnapshots: boolean,
baseDirectory: string,
) {
super(accountId, timeout, verbose)
this.project = project
Expand All @@ -31,6 +36,8 @@ export default class TestRunner extends AbstractCheckRunner {
this.shouldRecord = shouldRecord
this.repoInfo = repoInfo
this.environment = environment
this.updateSnapshots = updateSnapshots
this.baseDirectory = baseDirectory
}

async scheduleChecks (
Expand All @@ -46,7 +53,7 @@ export default class TestRunner extends AbstractCheckRunner {
...check.synthesize(),
group: check.groupId ? this.project.data['check-group'][check.groupId.ref].synthesize() : undefined,
groupId: undefined,
sourceInfo: { checkRunSuiteId, checkRunId },
sourceInfo: { checkRunSuiteId, checkRunId, updateSnapshots: this.updateSnapshots },
logicalId: check.logicalId,
filePath: check.getSourceFile(),
}))
Expand All @@ -71,4 +78,11 @@ export default class TestRunner extends AbstractCheckRunner {
throw new Error(err.response?.data?.message ?? err.message)
}
}

async processCheckResult (result: any) {
await super.processCheckResult(result)
if (this.updateSnapshots) {
await pullSnapshots(this.baseDirectory, result.assets?.snapshots)
}
}
}
29 changes: 29 additions & 0 deletions packages/cli/src/services/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,35 @@ export async function walkDirectory (
}
}

export function findFilesRecursively (directory: string, ignoredPaths: Array<string> = []) {
Copy link
Member

Choose a reason for hiding this comment

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

Don't we have a directory walker somewhere? I think we need a more flexible solution to support multiple scenarios. This is already the case at the runners IMO

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We have walkDirectory here in util.ts. It's unused, though, and calls a callback rather than giving a list of matching files. I'll probably delete it.

findFilesRecursively should be basically the same as the one we have in runners, it's just missing the filePredicate arg. Should I go ahead and proactively add the filePredicate arg? Is there anything else that I should add to this one?

if (!fsSync.statSync(directory, { throwIfNoEntry: false })?.isDirectory()) {
return []
}

const files = []
const directoriesToVisit = [directory]
const ignoredPathsSet = new Set(ignoredPaths)
while (directoriesToVisit.length > 0) {
const currentDirectory = directoriesToVisit.shift()!
const contents = fsSync.readdirSync(currentDirectory, { withFileTypes: true })
for (const content of contents) {
if (content.isSymbolicLink()) {
continue
}
const fullPath = path.resolve(currentDirectory, content.name)
if (ignoredPathsSet.has(fullPath)) {
continue
}
if (content.isDirectory()) {
directoriesToVisit.push(fullPath)
} else {
files.push(fullPath)
}
}
}
return files
}

export async function loadJsFile (filepath: string): Promise<any> {
try {
// There is a Node opened issue related with a segmentation fault using ES6 modules
Expand Down
Loading