diff --git a/packages/cli/e2e/__tests__/test.spec.ts b/packages/cli/e2e/__tests__/test.spec.ts index 0305ff2a7..62d9da2d3 100644 --- a/packages/cli/e2e/__tests__/test.spec.ts +++ b/packages/cli/e2e/__tests__/test.spec.ts @@ -316,4 +316,50 @@ describe('test', { timeout: 45000 }, () => { } }, 120_000) }) + + describe('shared-constructs-project', () => { + let fixt: FixtureSandbox + + beforeAll(async () => { + fixt = await FixtureSandbox.create({ + // The parser's sandbox spec covers the same fixture; it lives with the + // parser fixtures so there is one copy. + source: path.join( + __dirname, '..', '..', 'src', 'services', '__tests__', 'project-parser-fixtures', 'shared-constructs-project', + ), + template: 'playwright', + }) + }, 180_000) + + afterAll(async () => { + await fixt?.destroy() + }) + + it('Should list a check under the module that declares it, not under the check file importing it', async () => { + const result = await runTest(fixt, ['--list', 'shared/checks']) + expect(result.stdout).toContain('src/shared/checks.ts') + expect(result.stdout).toContain('Shared API') + expect(result.stdout).not.toContain('src/b.check.ts') + }) + + it('Should select a check made by a helper when naming the check file that called it', async () => { + const result = await runTest(fixt, ['--list', 'factory.check']) + expect(result.stdout).toContain('src/lib/factory.ts') + expect(result.stdout).toContain('factory-browser') + }) + + it('Should select the checks a check file loads from imported modules, listed under their own files', async () => { + const result = await runTest(fixt, ['--list', 'b.check']) + expect(result.stdout).toContain('src/b.check.ts') + expect(result.stdout).toContain('src/shared/checks.ts') + expect(result.stdout).toContain('Shared API') + }) + + it('Should not select checks from modules the named check file does not load', async () => { + const result = await runTest(fixt, ['--list', 'a.check']) + expect(result.stdout).toContain('src/a.check.ts') + expect(result.stdout).not.toContain('Shared API') + expect(result.stdout).not.toContain('Shared browser') + }) + }) }) diff --git a/packages/cli/src/commands/debug/parse-project.ts b/packages/cli/src/commands/debug/parse-project.ts index d8a15b1d3..a56575089 100644 --- a/packages/cli/src/commands/debug/parse-project.ts +++ b/packages/cli/src/commands/debug/parse-project.ts @@ -7,7 +7,7 @@ import { Diagnostics, Session, } from '../../constructs/index.js' -import { splitConfigFilePath } from '../../services/util.js' +import { getGitRepoRoot, splitConfigFilePath } from '../../services/util.js' import commonMessages from '../../messages/common-messages.js' import { loadSnapshot, Runtime } from '../../runtimes/index.js' import { Bundler } from '../../services/check-parser/bundler.js' @@ -37,6 +37,9 @@ export type ParseProjectOutput = { type: string member: boolean payload: unknown + // Present when the project lives in a git repository: the declaring + // file relative to the repository root, as a deploy sends it. + sourceFile?: string }[] } | null } @@ -223,7 +226,9 @@ export default class ParseProjectCommand extends Command { bundleMs = performance.now() - bundleStartedAt const synthesizeStartedAt = performance.now() - const synthesized = bundle.synthesize() + // Same envelope as `checkly deploy` sends, including the per-resource + // `sourceFile` relative to the git repository root. + const synthesized = bundle.synthesize({ repoRoot: getGitRepoRoot() }) synthesizeMs = performance.now() - synthesizeStartedAt return synthesized })() diff --git a/packages/cli/src/commands/test.ts b/packages/cli/src/commands/test.ts index 67f6dad02..85b5319a6 100644 --- a/packages/cli/src/commands/test.ts +++ b/packages/cli/src/commands/test.ts @@ -10,7 +10,7 @@ import { } from '../services/abstract-check-runner.js' import TestRunner from '../services/test-runner.js' import { loadChecklyConfig, resolveDependencyCacheVersion } from '../services/checkly-config-loader.js' -import { filterByFileNamePattern, filterByCheckNamePattern, filterByTags } from '../services/test-filters.js' +import { filterByCheckFiles, filterByCheckNamePattern, filterByTags } from '../services/test-filters.js' import { AuthCommand } from './authCommand.js' import { BrowserCheck, Check, HeartbeatMonitor, MultiStepCheck, Project, RetryStrategyBuilder, RuntimeCheck, Session } from '../constructs/index.js' import type { Region } from '../index.js' @@ -214,22 +214,18 @@ export default class Test extends AuthCommand { return false } - let entrypointMatch = false - if (check instanceof BrowserCheck || check instanceof MultiStepCheck) { - // For historical reasons the path used for filtering has always - // been relative to the project base path. - const relativeEntrypoint = isEntrypoint(check.code) - ? Session.relativePosixPath(check.code.entrypoint) - : undefined - - if (relativeEntrypoint) { - if (filterByFileNamePattern(filePatterns, relativeEntrypoint)) { - entrypointMatch = true - } - } - } - - if (!entrypointMatch && !filterByFileNamePattern(filePatterns, check.getSourceFile())) { + // For historical reasons the entrypoint used for filtering has + // always been relative to the project base path. + const scripted = check instanceof BrowserCheck || check instanceof MultiStepCheck + const entrypoint = scripted && isEntrypoint(check.code) + ? Session.relativePosixPath(check.code.entrypoint) + : undefined + const fileMatch = filterByCheckFiles(filePatterns, { + sourceFile: check.getSourceFile(), + loadedFrom: Session.relativeCheckFilePath(check.loadingFileAbsolutePath), + entrypoint, + }) + if (!fileMatch) { return false } diff --git a/packages/cli/src/constructs/__tests__/check.spec.ts b/packages/cli/src/constructs/__tests__/check.spec.ts index 8fbc1f523..762277b8a 100644 --- a/packages/cli/src/constructs/__tests__/check.spec.ts +++ b/packages/cli/src/constructs/__tests__/check.spec.ts @@ -1,3 +1,5 @@ +import path from 'node:path' + import { afterEach, beforeEach, describe, expect, it } from 'vitest' import { Frequency } from '../frequency.js' @@ -17,6 +19,28 @@ describe('Check', () => { Session.reset() }) + describe('__checkFilePath', () => { + const request = { method: 'GET' as const, url: 'https://api.example.com/health' } + + it('is the declaring file relative to the parse directory', () => { + // Created from a test, so the declaring file is the session's current + // check file. + Session.checkFilesDirectory = path.resolve('/proj') + Session.checkFileAbsolutePath = path.resolve('/proj/src/a.check.ts') + const check = new ApiCheck('a', { name: 'A', request }) + expect(check.checkFileAbsolutePath).toBe(path.resolve('/proj/src/a.check.ts')) + expect(check.__checkFilePath).toBe('src/a.check.ts') + expect(check.getSourceFile()).toBe('src/a.check.ts') + }) + + it('is unset while no project is being parsed, as for constructs in the config file', () => { + Session.checkFileAbsolutePath = path.resolve('/proj/checkly.config.ts') + const check = new ApiCheck('a', { name: 'A', request }) + expect(check.checkFileAbsolutePath).toBe(path.resolve('/proj/checkly.config.ts')) + expect(check.__checkFilePath).toBeUndefined() + }) + }) + it('synthesizes Frequency instances as numeric frequency fields', () => { const check = new ApiCheck('api-health', { name: 'API Health', diff --git a/packages/cli/src/constructs/__tests__/session.spec.ts b/packages/cli/src/constructs/__tests__/session.spec.ts new file mode 100644 index 000000000..8f2747ffb --- /dev/null +++ b/packages/cli/src/constructs/__tests__/session.spec.ts @@ -0,0 +1,28 @@ +import path from 'node:path' + +import { afterEach, describe, expect, it } from 'vitest' + +import { Session } from '../session.js' + +describe('Session.relativeCheckFilePath', () => { + afterEach(() => { + Session.reset() + }) + + it('is relative to the parse directory, with posix separators', () => { + Session.checkFilesDirectory = path.resolve('/proj') + expect(Session.relativeCheckFilePath(path.resolve('/proj/src/a.check.ts'))).toBe('src/a.check.ts') + }) + + it('walks up for a declaring file outside the parse directory', () => { + Session.checkFilesDirectory = path.resolve('/repo/apps/web') + expect(Session.relativeCheckFilePath(path.resolve('/repo/packages/checks/index.ts'))) + .toBe('../../packages/checks/index.ts') + }) + + it('is unset without a parse directory or a file', () => { + expect(Session.relativeCheckFilePath(path.resolve('/proj/src/a.check.ts'))).toBeUndefined() + Session.checkFilesDirectory = path.resolve('/proj') + expect(Session.relativeCheckFilePath(undefined)).toBeUndefined() + }) +}) diff --git a/packages/cli/src/constructs/check-group-v1.ts b/packages/cli/src/constructs/check-group-v1.ts index a71b28b3b..93cca252d 100644 --- a/packages/cli/src/constructs/check-group-v1.ts +++ b/packages/cli/src/constructs/check-group-v1.ts @@ -2,6 +2,8 @@ import path from 'node:path' import { glob } from 'glob' +import { pathToPosix } from '../services/util.js' + import { AlertChannel, AlertChannelRef } from './alert-channel.js' import { EnvironmentVariable } from './environment-variable.js' import { PrivateLocation, PrivateLocationRef } from './private-location.js' @@ -41,14 +43,20 @@ const defaultApiCheckDefaults: ApiCheckDefaultConfig = { type BrowserCheckConfig = CheckConfigDefaults & { /** - * Glob pattern to include multiple files, i.e. all `.spec.ts` files + * Glob pattern to include multiple files, i.e. all `.spec.ts` files, + * relative to the check file being loaded when the group is created. + * When nothing matches there and the group is declared in another file + * (a shared module, a helper), that file's directory is searched. */ testMatch: string | string[] } type MultiStepCheckConfig = CheckConfigDefaults & { /** - * Glob pattern to include multiple files, i.e. all `.spec.ts` files + * Glob pattern to include multiple files, i.e. all `.spec.ts` files, + * relative to the check file being loaded when the group is created. + * When nothing matches there and the group is declared in another file + * (a shared module, a helper), that file's directory is searched. */ testMatch: string | string[] } @@ -344,13 +352,12 @@ export class CheckGroupV1 extends Construct { this.runParallel = props.runParallel // `browserChecks` is not a CheckGroup resource property. Not present in synthesize() this.browserChecks = props.browserChecks - const fileAbsolutePath = Session.checkFileAbsolutePath! if (props.browserChecks?.testMatch) { - this.__addChecks(fileAbsolutePath, props.browserChecks.testMatch, CheckTypes.BROWSER) + this.__addChecks(props.browserChecks.testMatch, CheckTypes.BROWSER) } this.multiStepChecks = props.multiStepChecks if (props.multiStepChecks?.testMatch) { - this.__addChecks(fileAbsolutePath, props.multiStepChecks.testMatch, CheckTypes.MULTI_STEP) + this.__addChecks(props.multiStepChecks.testMatch, CheckTypes.MULTI_STEP) } Session.registerConstruct(this) this.__addSubscriptions() @@ -423,13 +430,28 @@ export class CheckGroupV1 extends Construct { return new CheckGroupRef(`check-group-${id}`, id) } + /** + * Creates a check for every file `testMatch` finds next to the check + * file that was being loaded when the group was created (as it always + * did) or, when nothing matches there, next to the file that declares + * the group. + */ private __addChecks ( - fileAbsolutePath: string, testMatch: string | string[], checkType: typeof CheckTypes.BROWSER | typeof CheckTypes.MULTI_STEP, ) { - const parent = path.dirname(fileAbsolutePath) - const matched = glob.sync(testMatch, { nodir: true, cwd: parent }) + const roots = [...new Set([this.loadingFileAbsolutePath, this.checkFileAbsolutePath])] + .filter((file): file is string => file !== undefined) + .map(file => path.dirname(file)) + let parent = roots[0] + let matched: string[] = [] + for (const root of roots) { + matched = glob.sync(testMatch, { nodir: true, cwd: root }) + if (matched.length > 0) { + parent = root + break + } + } for (const match of matched) { const filepath = path.join(parent, match) const props = { @@ -440,7 +462,12 @@ export class CheckGroupV1 extends Construct { }, // the browserChecks props inherited from the group are applied in BrowserCheck.constructor() } - const checkLogicalId = Session.relativePosixPath(filepath) + // Before a project is parsed (a group declared in the Checkly config + // file) there is no base path; the check constructor rejects the + // construct anyway, with a message naming the config file. + const checkLogicalId = Session.basePath + ? Session.relativePosixPath(filepath) + : pathToPosix(match) if (checkType === CheckTypes.BROWSER) { new BrowserCheck(checkLogicalId, props) } else { diff --git a/packages/cli/src/constructs/check.ts b/packages/cli/src/constructs/check.ts index b3831e4f1..e2cfd28eb 100644 --- a/packages/cli/src/constructs/check.ts +++ b/packages/cli/src/constructs/check.ts @@ -370,7 +370,12 @@ export abstract class Check extends Construct { useGlobalAlertSettings?: boolean runParallel?: boolean triggerIncident?: IncidentTrigger - __checkFilePath?: string // internal variable to filter by check file name from the CLI + /** + * The declaring file relative to the directory the project is parsed + * from; `checkly test ` filters on it and reporters group by it. + * Internal. + */ + __checkFilePath?: string #intent?: CheckIntent | null #aiAutoRepairEnabled?: boolean | null @@ -408,7 +413,7 @@ export abstract class Check extends Construct { this.useGlobalAlertSettings = !this.alertSettings this.runParallel = config.runParallel ?? false this.triggerIncident = config.triggerIncident - this.__checkFilePath = Session.checkFilePath + this.__checkFilePath = Session.relativeCheckFilePath(this.checkFileAbsolutePath) } protected async validateDoubleCheck (diagnostics: Diagnostics): Promise { diff --git a/packages/cli/src/constructs/construct.ts b/packages/cli/src/constructs/construct.ts index 58016f329..b4f0e5357 100644 --- a/packages/cli/src/constructs/construct.ts +++ b/packages/cli/src/constructs/construct.ts @@ -1,3 +1,4 @@ +import fs from 'node:fs' import path from 'node:path' import { LOGICAL_ID_PATTERN } from '../constants.js' @@ -6,6 +7,7 @@ import { Diagnostics } from './diagnostics.js' import { Session } from './session.js' import { Ref } from './ref.js' import { Bundler } from '../services/check-parser/bundler.js' +import { captureDeclaringFile } from './internal/declaring-file.js' /** * Base interface for construct-like objects in the Checkly CLI system. @@ -55,8 +57,24 @@ export abstract class Construct implements Validate, Bundle { physicalId?: string | number /** Whether this construct is a member of the project */ member: boolean - /** Absolute path to the check file that created this construct */ + /** + * Absolute path of the file that declares this construct: the user file + * whose code ran the constructor, read off the call stack, so a construct + * in a module shared by several check files is attributed to that module + * and not to whichever check file imported it first. When no user code is + * on the stack (the CLI created the construct itself, or a test did) it + * is the parser's current check file, if any. + */ checkFileAbsolutePath?: string + /** + * The check file the parser was loading when this construct was created, + * if any. It differs from `checkFileAbsolutePath` when the construct was + * declared in a module the check file imports, or when helper code in + * another file created it on the check file's behalf. Relative file + * paths are resolved next to this file first, as they always were, and + * next to the declaring file when they do not exist there. + */ + readonly loadingFileAbsolutePath?: string /** * Diagnostics recorded during construction. A constructor cannot perform the * async work that validate() can, so any issue it notices (e.g. an argument @@ -85,7 +103,15 @@ export abstract class Construct implements Validate, Bundle { this.type = type this.physicalId = physicalId this.member = member ?? true - this.checkFileAbsolutePath = Session.checkFileAbsolutePath + this.loadingFileAbsolutePath = Session.checkFileAbsolutePath + // One constructor frame per class from Construct down to the class + // being instantiated sits on top of the stack; the declaring file is + // the frame below them. + let constructorChainLength = 1 + for (let cls = new.target; cls && cls !== Construct; cls = Object.getPrototypeOf(cls)) { + constructorChainLength++ + } + this.checkFileAbsolutePath = captureDeclaringFile(constructorChainLength) ?? this.loadingFileAbsolutePath Session.validateCreateConstruct(this) } @@ -114,8 +140,13 @@ export abstract class Construct implements Validate, Bundle { } /** - * Resolves a content file path relative to the check file that created this construct. - * If the path is already absolute, returns it as-is. + * Resolves a content file path relative to the check file that was being + * loaded when this construct was created, or, when the path does not + * exist there, relative to the file that declares the construct. So a + * factory called from a check file keeps resolving paths from the check + * file's directory, and a construct declared in a shared module can + * keep its script next to that module. An absolute path is returned as + * is. * * @param contentPath The relative or absolute path to resolve * @returns The absolute path to the content file @@ -130,7 +161,12 @@ export abstract class Construct implements Validate, Bundle { throw new Error('Internal error: attempting to use relative content file path without checkFileAbsolutePath set') } - return path.join(path.dirname(this.checkFileAbsolutePath), contentPath) + const candidates = [...new Set([this.loadingFileAbsolutePath, this.checkFileAbsolutePath])] + .filter((file): file is string => file !== undefined) + .map(file => path.join(path.dirname(file), contentPath)) + // A path that exists nowhere resolves against the first base, so the + // error reported later names the location that was always tried. + return candidates.find(candidate => fs.existsSync(candidate)) ?? candidates[0] } /** @@ -181,7 +217,7 @@ export abstract class Construct implements Validate, Bundle { * Used when script code is stored in a separate file rather than inline. */ export interface Entrypoint { - /** Path to the script file, relative to the check file or absolute */ + /** Path to the script file, relative to the file that declares the construct, or absolute */ entrypoint: string } diff --git a/packages/cli/src/constructs/internal/__tests__/declaring-file.spec.ts b/packages/cli/src/constructs/internal/__tests__/declaring-file.spec.ts new file mode 100644 index 000000000..03dee0379 --- /dev/null +++ b/packages/cli/src/constructs/internal/__tests__/declaring-file.spec.ts @@ -0,0 +1,198 @@ +import fs from 'node:fs' +import os from 'node:os' +import path from 'node:path' +import { fileURLToPath } from 'node:url' + +import { afterAll, beforeAll, describe, expect, it } from 'vitest' + +import { captureDeclaringFile, declaringFileFromFrames, type Frame } from '../declaring-file.js' +import { Session } from '../../session.js' + +const ownRoot = '/proj/node_modules/checkly/dist' + +const ctor = (fileName: string | null | undefined): Frame => ({ fileName, isConstructor: true }) +const code = (fileName: string | null | undefined): Frame => ({ fileName, isConstructor: false }) + +// Construct -> Check -> ApiCheck. +const CLI_FRAMES = [ + ctor(`${ownRoot}/constructs/construct.js`), + ctor(`${ownRoot}/constructs/check.js`), + ctor(`${ownRoot}/constructs/api-check.js`), +] + +// Posix fixtures are parsed with posix rules on every platform; the win32 +// case below pins its own. +const options = { ownRoot, constructorChainLength: CLI_FRAMES.length, platformPath: path.posix } + +describe('declaringFileFromFrames', () => { + it('returns the first frame outside the CLI', () => { + const frames = [...CLI_FRAMES, code('/proj/src/shared/alerts.ts'), code('/proj/src/a.check.ts')] + expect(declaringFileFromFrames(frames, options)).toBe('/proj/src/shared/alerts.ts') + }) + + it('skips a subclass constructor in the user\'s code, since the file that ran `new` declares the construct', () => { + const frames = [...CLI_FRAMES, ctor('/proj/src/lib/team-check.ts'), code('/proj/src/a.check.ts')] + expect(declaringFileFromFrames(frames, { ...options, constructorChainLength: 4 })).toBe('/proj/src/a.check.ts') + }) + + it('keeps a helper function in the user\'s code as the declaring file', () => { + const frames = [...CLI_FRAMES, code('/proj/src/lib/factory.ts'), code('/proj/src/a.check.ts')] + expect(declaringFileFromFrames(frames, options)).toBe('/proj/src/lib/factory.ts') + }) + + it('keeps a wrapper class in the user\'s code as the declaring file, unlike a subclass', () => { + // `class Suite { constructor () { new ApiCheck(...) } }` is a helper: its + // constructor frame lies beyond the construct's own chain. + const frames = [...CLI_FRAMES, ctor('/proj/src/lib/suite.ts'), code('/proj/src/a.check.ts')] + expect(declaringFileFromFrames(frames, options)).toBe('/proj/src/lib/suite.ts') + }) + + it('stops skipping early when the chain is shorter than announced', () => { + const frames = [...CLI_FRAMES, code('/proj/src/a.check.ts')] + expect(declaringFileFromFrames(frames, { ...options, constructorChainLength: 10 })).toBe('/proj/src/a.check.ts') + }) + + it('skips CLI helpers between constructor frames, as for a group expanding its testMatch', () => { + const frames = [ + ctor(`${ownRoot}/constructs/construct.js`), + ctor(`${ownRoot}/constructs/check.js`), + ctor(`${ownRoot}/constructs/browser-check.js`), + code(`${ownRoot}/constructs/check-group-v1.js`), + ctor(`${ownRoot}/constructs/check-group-v1.js`), + code('/proj/src/group.check.ts'), + ] + expect(declaringFileFromFrames(frames, options)).toBe('/proj/src/group.check.ts') + }) + + it('turns a file URL into a path', () => { + const frames = [...CLI_FRAMES, code('file:///proj/src/a.check.mjs'), code('node:internal/modules/esm/module_job')] + expect(declaringFileFromFrames(frames, options)).toBe('/proj/src/a.check.mjs') + }) + + it('skips frames without a file, node internals and eval code', () => { + const frames = [ + ...CLI_FRAMES, + code(null), code(undefined), code(''), code('node:internal/process/task_queues'), code(''), + code('/proj/src/a.check.ts'), + ] + expect(declaringFileFromFrames(frames, options)).toBe('/proj/src/a.check.ts') + }) + + it('gives up when the first outside frame is a tool under node_modules', () => { + const frames = [ + ...CLI_FRAMES, + code(`${ownRoot}/services/project-parser.js`), + code(`${ownRoot}/commands/deploy.js`), + code('/proj/node_modules/@oclif/core/lib/command.js'), + code('node:internal/main/run_main_module'), + ] + expect(declaringFileFromFrames(frames, options)).toBeUndefined() + }) + + it('gives up when there are no frames', () => { + expect(declaringFileFromFrames([], options)).toBeUndefined() + expect(declaringFileFromFrames(CLI_FRAMES, options)).toBeUndefined() + }) + + it('does not mistake a sibling of the CLI root for CLI code', () => { + // `/proj/node_modules/checkly/dist-tools` shares a prefix with the root + // but is not inside it; it is under node_modules, so it is a tool. + const frames = [...CLI_FRAMES, code('/proj/node_modules/checkly/dist-tools/x.js')] + expect(declaringFileFromFrames(frames, options)).toBeUndefined() + expect(declaringFileFromFrames([...CLI_FRAMES, code('/projects/a.check.ts')], { ...options, ownRoot: '/proj' })) + .toBe('/projects/a.check.ts') + }) + + it('handles Windows paths and file URLs', () => { + const root = 'C:\\proj\\node_modules\\checkly\\dist' + const win32 = { ownRoot: root, constructorChainLength: 1, platformPath: path.win32 } + const frames = [ + ctor('c:\\PROJ\\node_modules\\checkly\\dist\\constructs\\construct.js'), + code(`${root}\\constructs\\check.js`), + code('file:///C:/proj/checks/a.check.ts'), + code('C:\\proj\\checks\\b.check.ts'), + ] + expect(declaringFileFromFrames(frames, win32)).toBe('C:\\proj\\checks\\a.check.ts') + expect(declaringFileFromFrames([code(`${root}\\constructs\\check.js`), code('D:\\other\\a.check.ts')], win32)) + .toBe('D:\\other\\a.check.ts') + expect(declaringFileFromFrames([code(`${root}\\constructs\\check.js`), code('C:\\proj\\node_modules\\tool\\x.js')], win32)) + .toBeUndefined() + }) +}) + +describe('captureDeclaringFile', () => { + let dir: string + + beforeAll(() => { + // Left unresolved (on macOS the temp dir sits behind a symlink): the + // loader reports a module it resolved itself at its physical path, and + // the entry file at the path it was handed. + dir = fs.mkdtempSync(path.join(os.tmpdir(), 'checkly-declaring-file-')) + const helper = fileURLToPath(new URL('../declaring-file.ts', import.meta.url)) + fs.writeFileSync(path.join(dir, 'base.ts'), [ + `import { captureDeclaringFile } from ${JSON.stringify(helper)}`, + // Mirrors Construct: the chain length counts the classes from Base down + // to the one being instantiated, and the capture happens in the + // constructor body. + 'export class Base {', + ' declaringFile?: string', + ' constructor () {', + ' let chain = 1', + ' for (let cls = new.target; cls !== Base; cls = Object.getPrototypeOf(cls)) chain++', + ' this.declaringFile = captureDeclaringFile(chain)', + ' }', + '}', + 'export const declaringFile = captureDeclaringFile(0)', + '', + ].join('\n')) + fs.writeFileSync(path.join(dir, 'sub.ts'), [ + 'import { Base } from \'./base.js\'', + 'export class Sub extends Base {}', + 'export function factory () { return new Sub() }', + 'export class Wrapper { inner = new Sub() }', + '', + ].join('\n')) + fs.writeFileSync(path.join(dir, 'entry.ts'), [ + 'import { Sub, Wrapper, factory } from \'./sub.js\'', + 'export { declaringFile } from \'./base.js\'', + 'export const viaSubclass = new Sub().declaringFile', + 'export const viaFactory = factory().declaringFile', + 'export const viaWrapper = new Wrapper().inner.declaringFile', + '', + ].join('\n')) + }) + + afterAll(() => { + fs.rmSync(dir, { recursive: true, force: true }) + Session.reset() + }) + + it('finds nothing when called from a test, which runs inside the CLI tree under a tool', () => { + expect(captureDeclaringFile(0)).toBeUndefined() + }) + + it('names the file whose code ran, not the file that imported it', async () => { + // The JS realpath, like the loader and the parser use: it resolves the + // macOS temp-dir symlink but, unlike the native one, does not expand + // Windows 8.3 short names such as RUNNER~1. + const real = (name: string) => fs.realpathSync(path.join(dir, name)) + const loaded = await Session.loadFile>(path.join(dir, 'entry.ts')) + // Top-level code in the imported module, at its physical path. + expect(loaded.declaringFile).toBe(real('base.ts')) + // The subclass constructor frame is skipped; the file running `new` + // counts, and as the entry file it keeps the path it was loaded by (not + // resolved; jiti only normalises the separators on Windows). + expect(loaded.viaSubclass?.replaceAll('/', path.sep)).toBe(path.join(dir, 'entry.ts')) + // A helper function is not part of the chain, so its file counts. + expect(loaded.viaFactory).toBe(real('sub.ts')) + // Nor is a wrapper class, even though its frame is a constructor. + expect(loaded.viaWrapper).toBe(real('sub.ts')) + }) + + it('leaves the error hooks as they were', () => { + const before = { prepareStackTrace: Error.prepareStackTrace, stackTraceLimit: Error.stackTraceLimit } + captureDeclaringFile(1) + expect(Error.prepareStackTrace).toBe(before.prepareStackTrace) + expect(Error.stackTraceLimit).toBe(before.stackTraceLimit) + }) +}) diff --git a/packages/cli/src/constructs/internal/declaring-file.ts b/packages/cli/src/constructs/internal/declaring-file.ts new file mode 100644 index 000000000..5bfbc4626 --- /dev/null +++ b/packages/cli/src/constructs/internal/declaring-file.ts @@ -0,0 +1,165 @@ +import path from 'node:path' +import { fileURLToPath } from 'node:url' + +/** + * Finds the user file that declares a construct by looking at the call + * stack of the construct's constructor. + * + * The project parser loads check files one at a time, but a module a check + * file imports is evaluated once, during its first importer's load, and then + * served from the module cache. A construct declared in such a module (a + * shared alerts file, a group definition no check glob matches) therefore + * cannot be attributed by "the file being loaded right now": that would name + * whichever importer happened to load first. The call stack does not have + * this problem: the frame right below the constructor chain is the code that + * ran `new SomeCheck(...)`, wherever it lives. + * + * Frames are considered from the innermost outwards: + * - the constructor chain of the construct's own class hierarchy is skipped + * (one frame per class from `Construct` down to the class being + * instantiated, subclasses in the user's code included), so the file that + * ran `new` counts rather than the file declaring a subclass; + * - frames without a file, `node:` internals and eval'd code are skipped; + * - frames inside the CLI's own code (`dist/` in the published package, + * `src/` when run from source) are skipped, so the parser and helpers such + * as a group's `testMatch` expansion never count either; + * - the first remaining frame is the answer, unless it sits under a + * `node_modules` directory: then no user code created the construct (the + * CLI created it itself and the next frame out is the command runner, or a + * test runner did), and the caller falls back to the parser's current file. + * + * Both jiti (TypeScript check files) and Node's own ESM loader report the + * original file, as a plain path or a `file://` URL. A module the loader + * resolved itself is reported at its physical location (symlinks resolved); + * the file the loader was asked to load keeps the path it was given, which is + * why the project parser hands it physical paths. + * + * Accepted limitations: a construct created inside a helper function or a + * wrapper class in the user's code is attributed to the helper's file; a + * construct library installed under `node_modules` (rather than linked from + * workspace source) falls back to the parser's current file; and V8 emits a + * single frame for a run of two or more consecutive subclasses without an + * explicit constructor, so behind such a run one more frame is skipped than + * the chain has classes, which only matters when that frame is a wrapper + * class's constructor. + */ + +/** + * The directory holding the CLI's own code: `dist/` in the published package, + * `src/` when running from source. Frames under it belong to the CLI, not to + * the user's project. + */ +const OWN_ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..', '..') + +/** + * Frames to capture. The CLI's own constructor chain is at most five frames + * deep (`CheckGroupV1` creating a `BrowserCheck` for a `testMatch` entry); + * the rest is headroom for subclasses and helper functions in the user's + * code. + */ +const STACK_DEPTH = 32 + +const NODE_MODULES_SEGMENT = /[\\/]node_modules[\\/]/ + +/** What the frame filter needs to know about a call site. */ +export interface Frame { + fileName?: string | null + isConstructor: boolean +} + +export interface DeclaringFileOptions { + /** The directory holding the CLI's own code. */ + ownRoot: string + /** + * The number of classes from `Construct` down to the class being + * instantiated; each one contributes a constructor frame at the top of + * the stack. + */ + constructorChainLength: number + /** + * For tests only, so Windows frames can be exercised on other platforms. + */ + platformPath?: path.PlatformPath +} + +function isInside (root: string, file: string, platformPath: path.PlatformPath): boolean { + const relative = platformPath.relative(root, file) + const escapes = relative === '..' || relative.startsWith(`..${platformPath.sep}`) + return !escapes && !platformPath.isAbsolute(relative) +} + +/** + * Picks the declaring file out of a list of stack frames, innermost first. + * See the module description for the rules. + */ +export function declaringFileFromFrames ( + frames: Frame[], + { ownRoot, constructorChainLength, platformPath = path }: DeclaringFileOptions, +): string | undefined { + // The chain is skipped by count, not by looking for constructor frames: + // a wrapper class in the user's code that creates a construct in its own + // constructor is a helper, and its frame must count. The flag only guards + // against a chain shorter than expected. + let skipped = 0 + for (const { fileName, isConstructor } of frames) { + if (skipped < constructorChainLength && isConstructor) { + skipped++ + continue + } + if (!fileName || fileName.startsWith('node:')) { + continue + } + let file: string + if (fileName.startsWith('file:')) { + try { + file = fileURLToPath(fileName, { windows: platformPath === path.win32 }) + } catch { + continue + } + } else if (platformPath.isAbsolute(fileName)) { + file = fileName + } else { + // ``, `eval at ...` and similar. + continue + } + if (isInside(ownRoot, file, platformPath)) { + continue + } + return NODE_MODULES_SEGMENT.test(file) ? undefined : file + } + return undefined +} + +/** + * The user file that declares the construct whose constructor is running, or + * `undefined` when no user code is on the stack (the CLI or a test runner + * created the construct) or the runtime does not expose structured stack + * frames. + * + * `Error.prepareStackTrace` is replaced for the duration of a single + * synchronous `new Error()` so V8 hands over call sites instead of a + * formatted string, then restored; only file names and the constructor flag + * are read, which no source map changes. + * + * @param constructorChainLength see {@link DeclaringFileOptions} + */ +export function captureDeclaringFile (constructorChainLength: number): string | undefined { + const { prepareStackTrace, stackTraceLimit } = Error + let callSites: unknown + try { + Error.prepareStackTrace = (_error, sites) => sites + Error.stackTraceLimit = STACK_DEPTH + callSites = new Error().stack + } finally { + Error.prepareStackTrace = prepareStackTrace + Error.stackTraceLimit = stackTraceLimit + } + if (!Array.isArray(callSites)) { + return undefined + } + const frames = (callSites as NodeJS.CallSite[]).map(site => ({ + fileName: site.getFileName(), + isConstructor: site.isConstructor(), + })) + return declaringFileFromFrames(frames, { ownRoot: OWN_ROOT, constructorChainLength }) +} diff --git a/packages/cli/src/constructs/session.ts b/packages/cli/src/constructs/session.ts index fcf6e887f..2e49a2b97 100644 --- a/packages/cli/src/constructs/session.ts +++ b/packages/cli/src/constructs/session.ts @@ -60,8 +60,20 @@ export class Session { static checkFilter?: CheckFilter static browserCheckDefaults?: CheckConfigDefaults static multiStepCheckDefaults?: CheckConfigDefaults - static checkFilePath?: string + /** + * The parser's current check file, as an absolute physical path. A + * construct whose declaring file cannot be read off the call stack (one + * the CLI creates itself for a `browserChecks` or `multiStepChecks` glob + * entry, a Playwright check, or one created from a test) is attributed + * to it; see `Construct.checkFileAbsolutePath`. + */ static checkFileAbsolutePath?: string + /** + * The physical directory the project is parsed from. `Check.__checkFilePath`, + * the path `checkly test ` filters on and reporters group by, is + * relative to it. + */ + static checkFilesDirectory?: string static availableRuntimes: Record static defaultRuntimeId?: string static verifyRuntimeDependencies = true @@ -88,8 +100,8 @@ export class Session { this.checkFilter = undefined this.browserCheckDefaults = undefined this.multiStepCheckDefaults = undefined - this.checkFilePath = undefined this.checkFileAbsolutePath = undefined + this.checkFilesDirectory = undefined this.availableRuntimes = {} this.defaultRuntimeId = undefined this.verifyRuntimeDependencies = true @@ -255,6 +267,19 @@ export class Session { return this.embeddedPackagesMaterializer } + /** + * A construct's declaring file relative to the directory the project is + * parsed from, with posix separators; `undefined` until a parse is under + * way (constructs declared in the Checkly config file are created before + * that) or when the file is unknown. + */ + static relativeCheckFilePath (absolutePath?: string): string | undefined { + if (!Session.checkFilesDirectory || !absolutePath) { + return undefined + } + return pathToPosix(path.relative(Session.checkFilesDirectory, absolutePath)) + } + static relativePosixPath (filePath: string): string { return pathToPosix(path.relative(Session.basePath!, filePath)) } diff --git a/packages/cli/src/services/__tests__/checkly-config-loader.spec.ts b/packages/cli/src/services/__tests__/checkly-config-loader.spec.ts index dbc411165..350c57a49 100644 --- a/packages/cli/src/services/__tests__/checkly-config-loader.spec.ts +++ b/packages/cli/src/services/__tests__/checkly-config-loader.spec.ts @@ -462,10 +462,8 @@ describe('loadChecklyConfig()', () => { it('loads the config file with its absolute path as the current check file', async () => { const filename = 'good-config.ts' let checkFileAbsolutePath: string | undefined - let checkFilePath: string | undefined const loadFile = vi.spyOn(Session, 'loadFile').mockImplementation(() => { checkFileAbsolutePath = Session.checkFileAbsolutePath - checkFilePath = Session.checkFilePath return Promise.resolve({ logicalId: 'test', projectName: 'Test' }) }) @@ -474,9 +472,6 @@ describe('loadChecklyConfig()', () => { expect(loadFile).toHaveBeenCalledOnce() expect(checkFileAbsolutePath).toBe(path.join(configDir, filename)) expect(path.isAbsolute(checkFileAbsolutePath!)).toBe(true) - // Session.checkFilePath drives `checkly test --files` filtering and is - // reserved for check files. - expect(checkFilePath).toBeUndefined() }) it('clears the current check file after loading the config, even on failure', async () => { const filename = 'good-config.ts' @@ -526,6 +521,24 @@ describe('loadChecklyConfig()', () => { expect(constructs[0]).toBeInstanceOf(CheckGroupV1) expect(constructs[0].checkFileAbsolutePath).toBe(configPath) }) + it('rejects a config-declared CheckGroup whose testMatch finds files, naming the config file', async () => { + // Checks cannot be declared in the config file, and a testMatch that + // matches creates checks. This used to crash on the missing base path + // before it got as far as that rule. + const filename = 'good-config.ts' + vi.spyOn(Session, 'loadFile').mockImplementation(() => { + // eslint-disable-next-line @typescript-eslint/no-unused-vars + const group = new CheckGroupV1('config-group', { + name: 'Config group', + locations: ['us-east-1'], + browserChecks: { testMatch: 'good-config.js' }, + }) + return Promise.resolve({ logicalId: 'test', projectName: 'Test' }) + }) + + await expect(loadChecklyConfig(configDir, [filename])) + .rejects.toThrow('Creating a BrowserCheck construct in the Checkly config file isn\'t supported.') + }) }) it('config from absolute path', async () => { const filename = 'good-config.ts' diff --git a/packages/cli/src/services/__tests__/project-parser-fixtures/shared-constructs-project/checkly.b-only.config.ts b/packages/cli/src/services/__tests__/project-parser-fixtures/shared-constructs-project/checkly.b-only.config.ts new file mode 100644 index 000000000..c88d9689e --- /dev/null +++ b/packages/cli/src/services/__tests__/project-parser-fixtures/shared-constructs-project/checkly.b-only.config.ts @@ -0,0 +1,13 @@ +import { defineConfig } from 'checkly' + +// Loads only b.check.ts, so the shared modules are first evaluated during +// a different importer's load than with the default config. +export default defineConfig({ + projectName: 'shared constructs project', + logicalId: 'shared-constructs-project', + repoUrl: 'https://github.com/checkly/checkly-cli', + checks: { + checkMatch: 'src/b.check.ts', + locations: ['eu-west-1'], + }, +}) diff --git a/packages/cli/src/services/__tests__/project-parser-fixtures/shared-constructs-project/checkly.config.ts b/packages/cli/src/services/__tests__/project-parser-fixtures/shared-constructs-project/checkly.config.ts new file mode 100644 index 000000000..f2c17c977 --- /dev/null +++ b/packages/cli/src/services/__tests__/project-parser-fixtures/shared-constructs-project/checkly.config.ts @@ -0,0 +1,14 @@ +import { defineConfig } from 'checkly' + +// Used by src/services/__tests__/project-parser-source-file.spec.ts (parse +// output) and by e2e/__tests__/test.spec.ts (`checkly test --list`); a +// change here shows up in both. +export default defineConfig({ + projectName: 'shared constructs project', + logicalId: 'shared-constructs-project', + repoUrl: 'https://github.com/checkly/checkly-cli', + checks: { + checkMatch: 'src/**/*.check.ts', + locations: ['eu-west-1'], + }, +}) diff --git a/packages/cli/src/services/__tests__/project-parser-fixtures/shared-constructs-project/package.json b/packages/cli/src/services/__tests__/project-parser-fixtures/shared-constructs-project/package.json new file mode 100644 index 000000000..6a86e9852 --- /dev/null +++ b/packages/cli/src/services/__tests__/project-parser-fixtures/shared-constructs-project/package.json @@ -0,0 +1,6 @@ +{ + "name": "shared-constructs-project", + "type": "module", + "version": "1.0.0", + "private": true +} diff --git a/packages/cli/src/services/__tests__/project-parser-fixtures/shared-constructs-project/src/a.check.ts b/packages/cli/src/services/__tests__/project-parser-fixtures/shared-constructs-project/src/a.check.ts new file mode 100644 index 000000000..4091d30c5 --- /dev/null +++ b/packages/cli/src/services/__tests__/project-parser-fixtures/shared-constructs-project/src/a.check.ts @@ -0,0 +1,13 @@ +import { ApiCheck } from 'checkly/constructs' + +import { group, ops } from './shared/alerts.js' + +new ApiCheck('a', { + name: 'A', + group, + alertChannels: [ops], + request: { + method: 'GET', + url: 'https://api.example.com/a', + }, +}) diff --git a/packages/cli/src/services/__tests__/project-parser-fixtures/shared-constructs-project/src/b.check.ts b/packages/cli/src/services/__tests__/project-parser-fixtures/shared-constructs-project/src/b.check.ts new file mode 100644 index 000000000..1f324bedc --- /dev/null +++ b/packages/cli/src/services/__tests__/project-parser-fixtures/shared-constructs-project/src/b.check.ts @@ -0,0 +1,16 @@ +import { ApiCheck } from 'checkly/constructs' + +import { group, ops } from './shared/alerts.js' +import './shared/checks.js' +import './shared/browser.js' +import './shared/legacy-group.js' + +new ApiCheck('b', { + name: 'B', + group, + alertChannels: [ops], + request: { + method: 'GET', + url: 'https://api.example.com/b', + }, +}) diff --git a/packages/cli/src/services/__tests__/project-parser-fixtures/shared-constructs-project/src/factory-home.test.ts b/packages/cli/src/services/__tests__/project-parser-fixtures/shared-constructs-project/src/factory-home.test.ts new file mode 100644 index 000000000..7ea2c2201 --- /dev/null +++ b/packages/cli/src/services/__tests__/project-parser-fixtures/shared-constructs-project/src/factory-home.test.ts @@ -0,0 +1,2 @@ +// Deliberately free of imports so it bundles without @playwright/test. +console.log('factory home') diff --git a/packages/cli/src/services/__tests__/project-parser-fixtures/shared-constructs-project/src/factory.check.ts b/packages/cli/src/services/__tests__/project-parser-fixtures/shared-constructs-project/src/factory.check.ts new file mode 100644 index 000000000..2107ef477 --- /dev/null +++ b/packages/cli/src/services/__tests__/project-parser-fixtures/shared-constructs-project/src/factory.check.ts @@ -0,0 +1,3 @@ +import { browserCheck } from './lib/factory.js' + +browserCheck('factory-browser', './factory-home.test.ts') diff --git a/packages/cli/src/services/__tests__/project-parser-fixtures/shared-constructs-project/src/legacy-home.test.ts b/packages/cli/src/services/__tests__/project-parser-fixtures/shared-constructs-project/src/legacy-home.test.ts new file mode 100644 index 000000000..350a17dc8 --- /dev/null +++ b/packages/cli/src/services/__tests__/project-parser-fixtures/shared-constructs-project/src/legacy-home.test.ts @@ -0,0 +1,2 @@ +// Deliberately free of imports so it bundles without @playwright/test. +console.log('legacy home') diff --git a/packages/cli/src/services/__tests__/project-parser-fixtures/shared-constructs-project/src/legacy.check.ts b/packages/cli/src/services/__tests__/project-parser-fixtures/shared-constructs-project/src/legacy.check.ts new file mode 100644 index 000000000..9b7f6fcc4 --- /dev/null +++ b/packages/cli/src/services/__tests__/project-parser-fixtures/shared-constructs-project/src/legacy.check.ts @@ -0,0 +1,3 @@ +import { legacyGroup } from './lib/groups.js' + +legacyGroup('legacy', { browserChecks: { testMatch: 'legacy-*.test.ts' } }) diff --git a/packages/cli/src/services/__tests__/project-parser-fixtures/shared-constructs-project/src/lib/factory.ts b/packages/cli/src/services/__tests__/project-parser-fixtures/shared-constructs-project/src/lib/factory.ts new file mode 100644 index 000000000..332d0dbf0 --- /dev/null +++ b/packages/cli/src/services/__tests__/project-parser-fixtures/shared-constructs-project/src/lib/factory.ts @@ -0,0 +1,10 @@ +import { BrowserCheck } from 'checkly/constructs' + +// Creates a browser check on behalf of the calling check file; the entrypoint +// is relative to that check file, not to this module. +export function browserCheck (logicalId: string, entrypoint: string) { + return new BrowserCheck(logicalId, { + name: logicalId, + code: { entrypoint }, + }) +} diff --git a/packages/cli/src/services/__tests__/project-parser-fixtures/shared-constructs-project/src/lib/groups.ts b/packages/cli/src/services/__tests__/project-parser-fixtures/shared-constructs-project/src/lib/groups.ts new file mode 100644 index 000000000..f2ff3f1e5 --- /dev/null +++ b/packages/cli/src/services/__tests__/project-parser-fixtures/shared-constructs-project/src/lib/groups.ts @@ -0,0 +1,11 @@ +import { CheckGroupV1, CheckGroupV1Props } from 'checkly/constructs' + +// Creates a legacy group on behalf of the calling check file; testMatch +// globs are relative to that check file, not to this module. +export function legacyGroup (logicalId: string, props: Partial) { + return new CheckGroupV1(logicalId, { + name: logicalId, + locations: ['eu-west-1'], + ...props, + }) +} diff --git a/packages/cli/src/services/__tests__/project-parser-fixtures/shared-constructs-project/src/lib/team-check.ts b/packages/cli/src/services/__tests__/project-parser-fixtures/shared-constructs-project/src/lib/team-check.ts new file mode 100644 index 000000000..15f6a3636 --- /dev/null +++ b/packages/cli/src/services/__tests__/project-parser-fixtures/shared-constructs-project/src/lib/team-check.ts @@ -0,0 +1,9 @@ +import { ApiCheck, ApiCheckProps } from 'checkly/constructs' + +// A subclass; instances belong to the file that runs `new`, not to this one. +export class TeamApiCheck extends ApiCheck { + constructor (logicalId: string, props: Omit & { url: string }) { + const { url, ...rest } = props + super(logicalId, { ...rest, request: { method: 'GET', url } }) + } +} diff --git a/packages/cli/src/services/__tests__/project-parser-fixtures/shared-constructs-project/src/shared/alerts.ts b/packages/cli/src/services/__tests__/project-parser-fixtures/shared-constructs-project/src/shared/alerts.ts new file mode 100644 index 000000000..51c2a58eb --- /dev/null +++ b/packages/cli/src/services/__tests__/project-parser-fixtures/shared-constructs-project/src/shared/alerts.ts @@ -0,0 +1,11 @@ +import { CheckGroupV2, EmailAlertChannel } from 'checkly/constructs' + +// Not matched by the check glob; only reached through the check files that +// import it. +export const ops = new EmailAlertChannel('ops', { + address: 'ops@example.com', +}) + +export const group = new CheckGroupV2('shared-group', { + name: 'Shared group', +}) diff --git a/packages/cli/src/services/__tests__/project-parser-fixtures/shared-constructs-project/src/shared/browser.ts b/packages/cli/src/services/__tests__/project-parser-fixtures/shared-constructs-project/src/shared/browser.ts new file mode 100644 index 000000000..f6bbb5946 --- /dev/null +++ b/packages/cli/src/services/__tests__/project-parser-fixtures/shared-constructs-project/src/shared/browser.ts @@ -0,0 +1,9 @@ +import { BrowserCheck } from 'checkly/constructs' + +// The entrypoint is relative to this file, not to the check file importing it. +export const sharedBrowser = new BrowserCheck('shared-browser', { + name: 'Shared browser', + code: { + entrypoint: './homepage.test.ts', + }, +}) diff --git a/packages/cli/src/services/__tests__/project-parser-fixtures/shared-constructs-project/src/shared/checks.ts b/packages/cli/src/services/__tests__/project-parser-fixtures/shared-constructs-project/src/shared/checks.ts new file mode 100644 index 000000000..8846bead9 --- /dev/null +++ b/packages/cli/src/services/__tests__/project-parser-fixtures/shared-constructs-project/src/shared/checks.ts @@ -0,0 +1,12 @@ +import { ApiCheck } from 'checkly/constructs' + +import { group } from './alerts.js' + +export const sharedApi = new ApiCheck('shared-api', { + name: 'Shared API', + group, + request: { + method: 'GET', + url: 'https://api.example.com/shared', + }, +}) diff --git a/packages/cli/src/services/__tests__/project-parser-fixtures/shared-constructs-project/src/shared/homepage.test.ts b/packages/cli/src/services/__tests__/project-parser-fixtures/shared-constructs-project/src/shared/homepage.test.ts new file mode 100644 index 000000000..a6862d9a8 --- /dev/null +++ b/packages/cli/src/services/__tests__/project-parser-fixtures/shared-constructs-project/src/shared/homepage.test.ts @@ -0,0 +1,2 @@ +// Deliberately free of imports so it bundles without @playwright/test. +console.log('homepage') diff --git a/packages/cli/src/services/__tests__/project-parser-fixtures/shared-constructs-project/src/shared/legacy-group.ts b/packages/cli/src/services/__tests__/project-parser-fixtures/shared-constructs-project/src/shared/legacy-group.ts new file mode 100644 index 000000000..f74d6473a --- /dev/null +++ b/packages/cli/src/services/__tests__/project-parser-fixtures/shared-constructs-project/src/shared/legacy-group.ts @@ -0,0 +1,9 @@ +import { CheckGroupV1 } from 'checkly/constructs' + +// The testMatch matches nothing next to the check file importing this module, +// so the checks come from next to this file. +export const legacyShared = new CheckGroupV1('legacy-shared', { + name: 'Legacy shared', + locations: ['eu-west-1'], + browserChecks: { testMatch: 'shared-*.test.ts' }, +}) diff --git a/packages/cli/src/services/__tests__/project-parser-fixtures/shared-constructs-project/src/shared/shared-home.test.ts b/packages/cli/src/services/__tests__/project-parser-fixtures/shared-constructs-project/src/shared/shared-home.test.ts new file mode 100644 index 000000000..3c9ad0a08 --- /dev/null +++ b/packages/cli/src/services/__tests__/project-parser-fixtures/shared-constructs-project/src/shared/shared-home.test.ts @@ -0,0 +1,2 @@ +// Deliberately free of imports so it bundles without @playwright/test. +console.log('shared home') diff --git a/packages/cli/src/services/__tests__/project-parser-fixtures/shared-constructs-project/src/team.check.ts b/packages/cli/src/services/__tests__/project-parser-fixtures/shared-constructs-project/src/team.check.ts new file mode 100644 index 000000000..390d28959 --- /dev/null +++ b/packages/cli/src/services/__tests__/project-parser-fixtures/shared-constructs-project/src/team.check.ts @@ -0,0 +1,3 @@ +import { TeamApiCheck } from './lib/team-check.js' + +new TeamApiCheck('team', { name: 'Team', url: 'https://api.example.com/team' }) diff --git a/packages/cli/src/services/__tests__/project-parser-session.spec.ts b/packages/cli/src/services/__tests__/project-parser-session.spec.ts index 51572ab75..dd9339a5b 100644 --- a/packages/cli/src/services/__tests__/project-parser-session.spec.ts +++ b/packages/cli/src/services/__tests__/project-parser-session.spec.ts @@ -39,6 +39,23 @@ describe('parseProject() Session plumbing', () => { expect(Session.embeddedPackages).toBeUndefined() }) + it('parses from the physical directory, since loaders report files with symlinks resolved', async () => { + // On macOS os.tmpdir() is a symlink (/var -> /private/var), so the + // directory handed in here differs from its physical path. + await parseProject({ + directory: dir, + projectLogicalId: 'test-project', + projectName: 'Test Project', + availableRuntimes: {}, + defaultRuntimeId: '2025.04', + }) + + const physical = await fs.realpath(dir) + expect(Session.checkFilesDirectory).toBe(physical) + expect(Session.basePath).toBe(physical) + expect(Session.contextPath).toBe(physical) + }) + it('leaves Session.embeddedPackages undefined when not configured', async () => { await parseProject({ directory: dir, diff --git a/packages/cli/src/services/__tests__/project-parser-source-file.spec.ts b/packages/cli/src/services/__tests__/project-parser-source-file.spec.ts new file mode 100644 index 000000000..64a7cadcd --- /dev/null +++ b/packages/cli/src/services/__tests__/project-parser-source-file.spec.ts @@ -0,0 +1,117 @@ +import fs from 'node:fs/promises' +import path from 'node:path' + +import { describe, it, expect, beforeAll, afterAll } from 'vitest' + +import { FixtureSandbox } from '../../testing/fixture-sandbox.js' +import { ParseProjectOutput } from '../../commands/debug/parse-project.js' + +/** + * A construct is attributed to the file that declares it, not to the check + * file that happened to import that file first. The fixture keeps an alert + * channel, a group, an API check, a browser check and a legacy group in + * modules the check glob does not match, imported by two check files, plus + * a factory helper, a group helper and a subclass. + */ + +// Each parse spawns the packed CLI, parses and bundles the fixture. +const PARSE_TIMEOUT = 180_000 + +async function parseProject (fixt: FixtureSandbox, ...args: string[]): Promise { + const result = await fixt.run('pnpm', ['checkly', 'debug', 'parse-project', ...args]).catch(err => err) + // The command reports an error it caught as `{ errors }` on stdout and + // still exits 0, so a payload-less output is a failure too. + const output = result.exitCode === 0 ? JSON.parse(result.stdout) : undefined + if (result.exitCode !== 0 || !output?.diagnostics) { + // eslint-disable-next-line no-console + console.error('stderr', result.stderr) + // eslint-disable-next-line no-console + console.error('stdout', result.stdout) + } + expect(result.exitCode).toBe(0) + expect(output).toHaveProperty('diagnostics') + return output +} + +/** Every resource's `sourceFile` by logical id, leaving out the ones without one. */ +function sourceFiles (output: ParseProjectOutput): Record { + return Object.fromEntries( + (output.payload?.resources ?? []) + .filter(r => r.sourceFile !== undefined) + .map(r => [r.logicalId, r.sourceFile!]), + ) +} + +function payloadOf (output: ParseProjectOutput, logicalId: string): unknown { + return output.payload?.resources.find(r => r.logicalId === logicalId)?.payload +} + +describe('parseProject() source file attribution', () => { + let fixt: FixtureSandbox + let output: ParseProjectOutput + + beforeAll(async () => { + fixt = await FixtureSandbox.create({ + source: path.join(__dirname, 'project-parser-fixtures', 'shared-constructs-project'), + }) + // `sourceFile` is relative to the git repository root. + await fs.mkdir(fixt.abspath('.git')) + output = await parseProject(fixt) + }, PARSE_TIMEOUT) + + afterAll(async () => { + await fixt?.destroy() + }) + + it('names the module that declares each construct', () => { + expect(output.diagnostics.fatal).toBe(false) + expect(sourceFiles(output)).toEqual({ + 'ops': 'src/shared/alerts.ts', + 'shared-group': 'src/shared/alerts.ts', + 'shared-api': 'src/shared/checks.ts', + 'shared-browser': 'src/shared/browser.ts', + 'legacy-shared': 'src/shared/legacy-group.ts', + 'src/shared/shared-home.test.ts': 'src/shared/legacy-group.ts', + 'a': 'src/a.check.ts', + 'b': 'src/b.check.ts', + // Subscriptions are created by the check's constructor, so they belong + // to the check's file. + 'check-alert-channel-subscription#a#ops': 'src/a.check.ts', + 'check-alert-channel-subscription#b#ops': 'src/b.check.ts', + // A helper function declares what it creates; a subclass does not. + 'factory-browser': 'src/lib/factory.ts', + 'legacy': 'src/lib/groups.ts', + 'src/legacy-home.test.ts': 'src/lib/groups.ts', + 'team': 'src/team.check.ts', + }) + }) + + it('resolves a relative entrypoint next to the check file first, then next to the declaring module', () => { + // The factory's entrypoint exists next to the calling check file. + expect(payloadOf(output, 'factory-browser')).toMatchObject({ scriptPath: 'src/factory-home.test.ts' }) + // The shared module's entrypoint exists only next to the module. + expect(payloadOf(output, 'shared-browser')).toMatchObject({ scriptPath: 'src/shared/homepage.test.ts' }) + }) + + it('globs a testMatch next to the check file first, then next to the declaring module', () => { + // The helper's testMatch matches next to the calling check file. + expect(payloadOf(output, 'src/legacy-home.test.ts')).toMatchObject({ scriptPath: 'src/legacy-home.test.ts' }) + // The shared module's testMatch matches only next to the module. + expect(payloadOf(output, 'src/shared/shared-home.test.ts')).toMatchObject({ scriptPath: 'src/shared/shared-home.test.ts' }) + }) + + it('does not depend on which check file loads the shared modules first', async () => { + const bOnly = await parseProject(fixt, '--config', 'checkly.b-only.config.ts') + + expect(bOnly.diagnostics.fatal).toBe(false) + expect(sourceFiles(bOnly)).toMatchObject({ + 'ops': 'src/shared/alerts.ts', + 'shared-group': 'src/shared/alerts.ts', + 'shared-api': 'src/shared/checks.ts', + 'shared-browser': 'src/shared/browser.ts', + 'legacy-shared': 'src/shared/legacy-group.ts', + 'b': 'src/b.check.ts', + }) + expect(sourceFiles(bOnly)).not.toHaveProperty('a') + }, PARSE_TIMEOUT) +}) diff --git a/packages/cli/src/services/__tests__/test-filters.spec.ts b/packages/cli/src/services/__tests__/test-filters.spec.ts index 29ff2bbd6..0a236522d 100644 --- a/packages/cli/src/services/__tests__/test-filters.spec.ts +++ b/packages/cli/src/services/__tests__/test-filters.spec.ts @@ -1,6 +1,33 @@ import { describe, expect, test } from 'vitest' -import { filterByFileNamePattern, filterByCheckNamePattern, filterByTags } from '../test-filters.js' +import { filterByCheckFiles, filterByFileNamePattern, filterByCheckNamePattern, filterByTags } from '../test-filters.js' + +describe('filterByCheckFiles()', () => { + // A check declared in src/lib/factory.ts on behalf of src/checks/a.check.ts. + const files = { sourceFile: 'src/lib/factory.ts', loadedFrom: 'src/checks/a.check.ts' } + + test('selects a check by the file that declares it', () => { + expect(filterByCheckFiles(['lib/factory'], files)).toBe(true) + }) + + test('selects a check by the check file that loaded it', () => { + expect(filterByCheckFiles(['a.check'], files)).toBe(true) + }) + + test('selects a browser check by its script', () => { + expect(filterByCheckFiles(['home.spec'], { ...files, entrypoint: 'src/checks/home.spec.ts' })).toBe(true) + }) + + test('leaves out a check none of the files name', () => { + expect(filterByCheckFiles(['b.check'], { ...files, entrypoint: 'src/checks/home.spec.ts' })).toBe(false) + expect(filterByCheckFiles(['a.check'], {})).toBe(false) + }) + + test('never matches a file it does not know', () => { + expect(filterByCheckFiles(['undefined'], files)).toBe(false) + expect(filterByCheckFiles(['def'], {})).toBe(false) + }) +}) describe('filterByCheckNamePattern()', () => { type TestTuple = [string, string, boolean] diff --git a/packages/cli/src/services/checkly-config-loader.ts b/packages/cli/src/services/checkly-config-loader.ts index f7f3fbe63..ffa2cea76 100644 --- a/packages/cli/src/services/checkly-config-loader.ts +++ b/packages/cli/src/services/checkly-config-loader.ts @@ -492,17 +492,20 @@ export async function loadChecklyConfig ( } catch { continue } - // Constructs declared in the config file capture the file they come - // from, like constructs in check files do. Deploys report it as the - // construct's sourceFile, and relative paths on config-declared - // constructs (a check group's testMatch, a setup script entrypoint) - // resolve against the config file's directory instead of failing. - // Only the absolute path is set: Session.checkFilePath drives - // `checkly test --files` filtering and must stay unset here. + // Constructs read their declaring file off the call stack; the + // session's current file is the fallback for constructs created + // without user code on the stack, and while the config loads that is + // the config file itself. Deploys report it as the construct's + // sourceFile, and relative paths on config-declared constructs (a + // check group's testMatch, a setup script entrypoint) resolve against + // the config file's directory. const previousCheckFileAbsolutePath = Session.checkFileAbsolutePath - Session.checkFileAbsolutePath = path.resolve(filePath) + // Loaded by its physical path, so the path constructs read off the + // call stack and the fallback agree. + const physicalPath = await fs.realpath(filePath) + Session.checkFileAbsolutePath = physicalPath try { - config = await Session.loadFile(filePath) + config = await Session.loadFile(physicalPath) } finally { Session.checkFileAbsolutePath = previousCheckFileAbsolutePath } diff --git a/packages/cli/src/services/project-parser.ts b/packages/cli/src/services/project-parser.ts index 19620948e..2efffc602 100644 --- a/packages/cli/src/services/project-parser.ts +++ b/packages/cli/src/services/project-parser.ts @@ -1,3 +1,4 @@ +import fs from 'node:fs/promises' import * as path from 'path' import Debug from 'debug' import { @@ -127,7 +128,7 @@ async function findBasePath ( export async function parseProject (opts: ProjectParseOpts): Promise { const { - directory, + directory: givenDirectory, checkMatch = '**/*.check.{js,ts}', checkFilter, includeTestOnlyChecks = false, @@ -152,6 +153,14 @@ export async function parseProject (opts: ProjectParseOpts): Promise { enableWorkspaces = true, } = opts + // Constructs learn their declaring file from the call stack, and module + // loaders report files at their physical location, with symlinks resolved. + // Every path derived from the directory below (the base path, the glob + // results, the check-file paths) must be physical too, or a check file + // reached through a symlink would compare unequal to the same file seen + // from a construct. + const directory = await fs.realpath(givenDirectory) + const project = new Project(projectLogicalId, { name: projectName, repoUrl, @@ -178,6 +187,7 @@ export async function parseProject (opts: ProjectParseOpts): Promise { Session.project = project Session.basePath = basePath Session.contextPath = contextPath + Session.checkFilesDirectory = directory Session.checkDefaults = Object.assign({}, BASE_CHECK_DEFAULTS, checkDefaults) Session.checkFilter = checkFilter Session.browserCheckDefaults = browserCheckDefaults @@ -198,8 +208,9 @@ export async function parseProject (opts: ProjectParseOpts): Promise { if (!loadPlaywrightChecksOnly) { await loadAllCheckFiles(directory, checkMatch, ignoreDirectories) - // Load sequentially because otherwise Session.checkFileAbsolutePath and - // Session.checkFilePath are going to be subject to race conditions. + // Load sequentially: Session.checkFileAbsolutePath names the file being + // loaded, which constructs without a user frame on the call stack fall + // back to. await loadAllBrowserChecks(directory, browserCheckMatch, ignoreDirectories, project) await loadAllMultiStepChecks(directory, multiStepCheckMatch, ignoreDirectories, project) } @@ -211,17 +222,17 @@ export async function parseProject (opts: ProjectParseOpts): Promise { return project } -function setCheckFilePaths (checkFile: string, directory: string): string { - const relPath = pathToPosix(path.relative(directory, checkFile)) - +/** + * Marks `checkFile` as the file being loaded and returns its path relative + * to the parse directory, with posix separators. + */ +function setCheckFilePaths (checkFile: string): string { Session.checkFileAbsolutePath = checkFile - Session.checkFilePath = relPath - return relPath + return Session.relativeCheckFilePath(checkFile)! } function resetCheckFilePaths () { - Session.checkFilePath = undefined Session.checkFileAbsolutePath = undefined } @@ -236,7 +247,7 @@ async function loadPlaywrightChecks ( try { for (const playwrightCheckProps of playwrightChecks) { const configPath = getPlaywrightConfigPath(playwrightCheckProps, playwrightConfigPath, directory) - setCheckFilePaths(configPath, directory) + setCheckFilePaths(configPath) // eslint-disable-next-line @typescript-eslint/no-unused-vars const playwrightCheck = new PlaywrightCheck(playwrightCheckProps.logicalId, { ...playwrightCheckProps, @@ -252,7 +263,7 @@ async function loadPlaywrightChecks ( if (!playwrightConfigPath) { return } - setCheckFilePaths(playwrightConfigPath, directory) + setCheckFilePaths(playwrightConfigPath) const resolvedPlaywrightConfigPath = path.resolve(directory, playwrightConfigPath) const basePath = path.basename(resolvedPlaywrightConfigPath) // eslint-disable-next-line @typescript-eslint/no-unused-vars @@ -275,7 +286,7 @@ async function loadAllCheckFiles ( const checkFiles = await findFilesWithPattern(directory, checkFilePattern, ignorePattern) for (const checkFile of checkFiles) { try { - setCheckFilePaths(checkFile, directory) + setCheckFilePaths(checkFile) await Session.loadFile(checkFile) } finally { resetCheckFilePaths() @@ -319,7 +330,7 @@ async function loadAllBrowserChecks ( for (const checkFile of checkFiles) { try { - const relPath = setCheckFilePaths(checkFile, directory) + const relPath = setCheckFilePaths(checkFile) // Don't create an additional check if the checkFile was already added // to a check in loadAllCheckFiles. if (preexistingCheckFiles.has(relPath)) { @@ -353,7 +364,7 @@ async function loadAllMultiStepChecks ( for (const checkFile of checkFiles) { try { - const relPath = setCheckFilePaths(checkFile, directory) + const relPath = setCheckFilePaths(checkFile) // Don't create an additional check if the checkFile was already added // to a check in loadAllCheckFiles. if (preexistingCheckFiles.has(relPath)) { diff --git a/packages/cli/src/services/test-filters.ts b/packages/cli/src/services/test-filters.ts index bc712d335..be5248767 100644 --- a/packages/cli/src/services/test-filters.ts +++ b/packages/cli/src/services/test-filters.ts @@ -10,6 +10,31 @@ export function filterByFileNamePattern (filePatterns: Array = [], path: }) } +/** The files a check can be selected by with `checkly test `. */ +export interface CheckFiles { + /** The file that declares the check, relative to the parse directory. */ + sourceFile?: string + /** + * The check file that was being loaded when the check was created, + * relative to the parse directory; differs from `sourceFile` when a + * module the check file imports, or a helper it calls, declares the check. + */ + loadedFrom?: string + /** A browser or multistep check's script, relative to the base path. */ + entrypoint?: string +} + +/** + * Whether one of the file patterns names the check: by the file that + * declares it, by the check file that loaded it, or by its script. + */ +export function filterByCheckFiles (filePatterns: Array = [], files: CheckFiles): boolean { + // A file that is unknown is not matched by anything, not even a pattern + // that happens to match the word "undefined". + return [files.entrypoint, files.sourceFile, files.loadedFrom] + .some(file => file !== undefined && filterByFileNamePattern(filePatterns, file)) +} + export function filterByTags (targetTags: string[][], tags: string[] | undefined): boolean { if (targetTags?.length > 0 && tags) { return targetTags.some(targetTagSet => {