diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 523785232..ab187ad9e 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -37,7 +37,7 @@ jobs: strategy: fail-fast: false matrix: - os: [ubuntu-latest, windows-latest-x64] + os: [ubuntu-latest, blacksmith-4vcpu-windows-2025] name: test - ${{ matrix.os }} runs-on: ${{ matrix.os }} steps: @@ -66,7 +66,7 @@ jobs: strategy: fail-fast: false matrix: - os: [ubuntu-latest, windows-latest-x64] + os: [ubuntu-latest, blacksmith-4vcpu-windows-2025] name: e2e - checkly - ${{ matrix.os }} runs-on: ${{ matrix.os }} steps: @@ -93,7 +93,7 @@ jobs: strategy: fail-fast: false matrix: - os: [ubuntu-latest, windows-latest-x64] + os: [ubuntu-latest, blacksmith-4vcpu-windows-2025] name: e2e - create-checkly - ${{ matrix.os }} runs-on: ${{ matrix.os }} steps: diff --git a/packages/cli/e2e/__tests__/test.spec.ts b/packages/cli/e2e/__tests__/test.spec.ts index 284f2410c..0305ff2a7 100644 --- a/packages/cli/e2e/__tests__/test.spec.ts +++ b/packages/cli/e2e/__tests__/test.spec.ts @@ -174,8 +174,10 @@ describe('test', { timeout: 45000 }, () => { await runTest(fixt, []) } catch (err) { if (err instanceof ExecaError) { - expect((err.stderr as unknown as string).replace(/(\n {4})/gm, '')) - .toContain('Error: Resource of type \'check-group\' with logical id \'my-check-group\' already exists.') + // A duplicate logicalId is now reported as a fatal validation + // diagnostic (printed to stdout) rather than a thrown error. + expect((err.stdout as unknown as string).replace(/(\n {4})/gm, '')) + .toContain('A check-group with logicalId "my-check-group" already exists.') expect(err.exitCode).toBe(1) } else { throw err diff --git a/packages/cli/src/constructs/__tests__/alert-channel.spec.ts b/packages/cli/src/constructs/__tests__/alert-channel.spec.ts index efbbfcb45..5a7818533 100644 --- a/packages/cli/src/constructs/__tests__/alert-channel.spec.ts +++ b/packages/cli/src/constructs/__tests__/alert-channel.spec.ts @@ -19,19 +19,24 @@ class TestAlertChannel extends AlertChannel { } describe('AlertChannel', () => { - it('should throw if the same logicalId is used twice', () => { - Session.project = new Project('project-id', { + it('should produce a diagnostic if the same logicalId is used twice', async () => { + const project = new Project('project-id', { name: 'Test Project', repoUrl: 'https://github.com/checkly/checkly-cli', }) + Session.project = project - const add = () => { - new TestAlertChannel('foo', { - }) - } + new TestAlertChannel('foo', {}) + new TestAlertChannel('foo', {}) - expect(add).not.toThrow() - expect(add).toThrow('already exists') + const diagnostics = new Diagnostics() + await project.validate(diagnostics) + expect(diagnostics.isFatal()).toBe(true) + expect(diagnostics.observations).toEqual(expect.arrayContaining([ + expect.objectContaining({ + message: expect.stringContaining('already exists'), + }), + ])) }) it('should not throw if the same fromId() is used twice', () => { diff --git a/packages/cli/src/constructs/__tests__/check-group.spec.ts b/packages/cli/src/constructs/__tests__/check-group.spec.ts index 5ee4bfef9..9ffa667f1 100644 --- a/packages/cli/src/constructs/__tests__/check-group.spec.ts +++ b/packages/cli/src/constructs/__tests__/check-group.spec.ts @@ -11,20 +11,28 @@ describe('CheckGroup', () => { expect(CheckGroupV1).toBe(CheckGroup) }) - it('should throw if the same logicalId is used twice', () => { - Session.project = new Project('project-id', { + it('should produce a diagnostic if the same logicalId is used twice', async () => { + const project = new Project('project-id', { name: 'Test Project', repoUrl: 'https://github.com/checkly/checkly-cli', }) + Session.project = project - const add = () => { - new CheckGroupV1('foo', { - name: 'Test', - }) - } + new CheckGroupV1('foo', { + name: 'Test', + }) + new CheckGroupV1('foo', { + name: 'Test', + }) - expect(add).not.toThrow() - expect(add).toThrow('already exists') + const diagnostics = new Diagnostics() + await project.validate(diagnostics) + expect(diagnostics.isFatal()).toBe(true) + expect(diagnostics.observations).toEqual(expect.arrayContaining([ + expect.objectContaining({ + message: expect.stringContaining('already exists'), + }), + ])) }) it('should not throw if the same fromId() is used twice', () => { @@ -84,20 +92,28 @@ describe('CheckGroup', () => { }) describe('v2', () => { - it('should throw if the same logicalId is used twice', () => { - Session.project = new Project('project-id', { + it('should produce a diagnostic if the same logicalId is used twice', async () => { + const project = new Project('project-id', { name: 'Test Project', repoUrl: 'https://github.com/checkly/checkly-cli', }) + Session.project = project - const add = () => { - new CheckGroupV2('foo', { - name: 'Test', - }) - } + new CheckGroupV2('foo', { + name: 'Test', + }) + new CheckGroupV2('foo', { + name: 'Test', + }) - expect(add).not.toThrow() - expect(add).toThrow('already exists') + const diagnostics = new Diagnostics() + await project.validate(diagnostics) + expect(diagnostics.isFatal()).toBe(true) + expect(diagnostics.observations).toEqual(expect.arrayContaining([ + expect.objectContaining({ + message: expect.stringContaining('already exists'), + }), + ])) }) it('should not throw if the same fromId() is used twice', () => { diff --git a/packages/cli/src/constructs/__tests__/early-diagnostics.spec.ts b/packages/cli/src/constructs/__tests__/early-diagnostics.spec.ts new file mode 100644 index 000000000..aa47a3959 --- /dev/null +++ b/packages/cli/src/constructs/__tests__/early-diagnostics.spec.ts @@ -0,0 +1,77 @@ +import { describe, it, expect, beforeEach } from 'vitest' + +import { Session } from '../session.js' +import { Construct } from '../construct.js' +import { Diagnostics, ErrorDiagnostic } from '../diagnostics.js' +import { ConstructDiagnostics } from '../construct-diagnostics.js' + +class EarlyConstruct extends Construct { + constructor (logicalId: string) { + super('test', logicalId) + this.earlyDiagnostics.add(new ErrorDiagnostic({ + title: 'Early problem', + message: 'Something the constructor noticed.', + error: new Error('Something the constructor noticed.'), + })) + } + + describe (): string { + return `Test:${this.logicalId}` + } + + synthesize () { + return null + } +} + +describe('Construct early diagnostics', () => { + beforeEach(() => { + Session.reset() + Session.project = { addResource: () => {} } as any + }) + + it('surfaces diagnostics recorded in the constructor via validate()', async () => { + const construct = new EarlyConstruct('my-check') + const diagnostics = new Diagnostics() + await construct.validate(diagnostics) + expect(diagnostics.isFatal()).toBe(true) + expect(diagnostics.observations).toEqual(expect.arrayContaining([ + expect.objectContaining({ + message: expect.stringContaining('Something the constructor noticed.'), + }), + ])) + }) + + it('attributes early diagnostics to the construct when validated via ConstructDiagnostics', async () => { + const construct = new EarlyConstruct('my-check') + const diagnostics = new ConstructDiagnostics(construct) + await construct.validate(diagnostics) + expect(diagnostics.observations).toEqual(expect.arrayContaining([ + expect.objectContaining({ + title: expect.stringContaining('[Test:my-check]'), + }), + ])) + }) + + it('does not record any diagnostics when the constructor adds none', async () => { + class QuietConstruct extends Construct { + constructor (logicalId: string) { + super('test', logicalId) + } + + describe (): string { + return `Quiet:${this.logicalId}` + } + + synthesize () { + return null + } + } + + const construct = new QuietConstruct('my-check') + const diagnostics = new Diagnostics() + await construct.validate(diagnostics) + expect(diagnostics.isFatal()).toBe(false) + expect(diagnostics.observations).toHaveLength(0) + }) +}) diff --git a/packages/cli/src/constructs/__tests__/private-location.spec.ts b/packages/cli/src/constructs/__tests__/private-location.spec.ts index 3be899e49..ca69dbbab 100644 --- a/packages/cli/src/constructs/__tests__/private-location.spec.ts +++ b/packages/cli/src/constructs/__tests__/private-location.spec.ts @@ -5,21 +5,30 @@ import { Project } from '../project.js' import { Session } from '../session.js' describe('PrivateLocation', () => { - it('should throw if the same logicalId is used twice', () => { - Session.project = new Project('project-id', { + it('should produce a diagnostic if the same logicalId is used twice', async () => { + const project = new Project('project-id', { name: 'Test Project', repoUrl: 'https://github.com/checkly/checkly-cli', }) + Session.project = project - const add = () => { - new PrivateLocation('foo', { - name: 'Test', - slugName: 'test', - }) - } + new PrivateLocation('foo', { + name: 'Test', + slugName: 'test', + }) + new PrivateLocation('foo', { + name: 'Test', + slugName: 'test', + }) - expect(add).not.toThrow() - expect(add).toThrow('already exists') + const diagnostics = new Diagnostics() + await project.validate(diagnostics) + expect(diagnostics.isFatal()).toBe(true) + expect(diagnostics.observations).toEqual(expect.arrayContaining([ + expect.objectContaining({ + message: expect.stringContaining('already exists'), + }), + ])) }) it('should not throw if the same fromId() is used twice', () => { diff --git a/packages/cli/src/constructs/__tests__/project.spec.ts b/packages/cli/src/constructs/__tests__/project.spec.ts index 3e5d1c035..47f38afa5 100644 --- a/packages/cli/src/constructs/__tests__/project.spec.ts +++ b/packages/cli/src/constructs/__tests__/project.spec.ts @@ -1,11 +1,12 @@ import { describe, it, expect, beforeEach } from 'vitest' +import { Project } from '../project.js' import { Session } from '../session.js' import { Construct } from '../construct.js' import { Diagnostics } from '../diagnostics.js' class TestConstruct extends Construct { - constructor (logicalId: string) { + constructor (logicalId: any) { super('test', logicalId) } @@ -74,4 +75,36 @@ describe('Construct logicalId validation', () => { }), ])) }) + + it('should produce a diagnostic for a non-string logicalId instead of throwing', async () => { + const construct = new TestConstruct(123) + expect(construct.logicalId).toBe('123') + const diagnostics = new Diagnostics() + await construct.validate(diagnostics) + expect(diagnostics.isFatal()).toBe(true) + expect(diagnostics.observations).toEqual(expect.arrayContaining([ + expect.objectContaining({ + message: expect.stringContaining('Expected a string but received type "number"'), + }), + ])) + }) + + it('should produce a diagnostic for a duplicate logicalId instead of throwing', async () => { + Session.reset() + const project = new Project('test-project', { name: 'Test' }) + Session.project = project + + project.addResource('check', 'duplicate-id', new TestConstruct('duplicate-id')) + project.addResource('check', 'duplicate-id', new TestConstruct('duplicate-id')) + + const diagnostics = new Diagnostics() + await project.validate(diagnostics) + expect(diagnostics.isFatal()).toBe(true) + expect(diagnostics.observations).toEqual(expect.arrayContaining([ + expect.objectContaining({ + title: expect.stringContaining('[Test:duplicate-id]'), + message: expect.stringContaining('A check with logicalId "duplicate-id" already exists'), + }), + ])) + }) }) diff --git a/packages/cli/src/constructs/__tests__/status-page-service.spec.ts b/packages/cli/src/constructs/__tests__/status-page-service.spec.ts index 11f97ab26..aaa7bd8dd 100644 --- a/packages/cli/src/constructs/__tests__/status-page-service.spec.ts +++ b/packages/cli/src/constructs/__tests__/status-page-service.spec.ts @@ -5,20 +5,28 @@ import { Project } from '../project.js' import { Session } from '../session.js' describe('StatusPageService', () => { - it('should throw if the same logicalId is used twice', () => { - Session.project = new Project('project-id', { + it('should produce a diagnostic if the same logicalId is used twice', async () => { + const project = new Project('project-id', { name: 'Test Project', repoUrl: 'https://github.com/checkly/checkly-cli', }) + Session.project = project - const add = () => { - new StatusPageService('foo', { - name: 'foo', - }) - } + new StatusPageService('foo', { + name: 'foo', + }) + new StatusPageService('foo', { + name: 'foo', + }) - expect(add).not.toThrow() - expect(add).toThrow('already exists') + const diagnostics = new Diagnostics() + await project.validate(diagnostics) + expect(diagnostics.isFatal()).toBe(true) + expect(diagnostics.observations).toEqual(expect.arrayContaining([ + expect.objectContaining({ + message: expect.stringContaining('already exists'), + }), + ])) }) it('should not throw if the same fromId() is used twice', () => { diff --git a/packages/cli/src/constructs/construct.ts b/packages/cli/src/constructs/construct.ts index b96410371..58016f329 100644 --- a/packages/cli/src/constructs/construct.ts +++ b/packages/cli/src/constructs/construct.ts @@ -57,6 +57,13 @@ export abstract class Construct implements Validate, Bundle { member: boolean /** Absolute path to the check file that created this construct */ checkFileAbsolutePath?: string + /** + * Diagnostics recorded during construction. A constructor cannot perform the + * async work that validate() can, so any issue it notices (e.g. an argument + * of the wrong type) can be added here and is merged into the caller's + * Diagnostics by validate(). + */ + readonly earlyDiagnostics = new Diagnostics() /** * Creates a new construct instance. @@ -67,6 +74,13 @@ export abstract class Construct implements Validate, Bundle { * @param member Whether this construct is a member of the project */ constructor (type: string, logicalId: string, physicalId?: string | number, member?: boolean) { + if (typeof logicalId !== 'string') { + this.earlyDiagnostics.add(new InvalidPropertyValueDiagnostic( + 'logicalId', + new Error(`Expected a string but received type "${typeof logicalId}".`), + )) + logicalId = String(logicalId) + } this.logicalId = logicalId this.type = type this.physicalId = physicalId @@ -128,6 +142,8 @@ export abstract class Construct implements Validate, Bundle { */ // eslint-disable-next-line require-await async validate (diagnostics: Diagnostics): Promise { + diagnostics.extend(this.earlyDiagnostics) + if (!LOGICAL_ID_PATTERN.test(this.logicalId)) { diagnostics.add(new InvalidPropertyValueDiagnostic( 'logicalId', diff --git a/packages/cli/src/constructs/project.ts b/packages/cli/src/constructs/project.ts index d2be3dfc0..271db5335 100644 --- a/packages/cli/src/constructs/project.ts +++ b/packages/cli/src/constructs/project.ts @@ -9,7 +9,7 @@ import { StatusPage, StatusPageService, } from './/index.js' import { Diagnostics } from './diagnostics.js' -import { ConstructDiagnostics, InvalidPropertyValueDiagnostic } from './construct-diagnostics.js' +import { ConstructDiagnostic, ConstructDiagnostics, InvalidPropertyValueDiagnostic } from './construct-diagnostics.js' import { ProjectBundle, ProjectDataBundle } from './project-bundle.js' import { Bundler } from '../services/check-parser/bundler.js' import { Session } from './session.js' @@ -126,7 +126,19 @@ export class Project extends Construct { return } - throw new Error(`Resource of type '${type}' with logical id '${logicalId}' already exists.`) + // The duplicate resource is intentionally not stored, so it is never + // visited by the validate() fan-out over project data. Record the + // diagnostic on the project itself so it surfaces during validation, + // wrapping it in a ConstructDiagnostic so the output names the + // offending construct. + this.earlyDiagnostics.add(new ConstructDiagnostic( + resource, + new InvalidPropertyValueDiagnostic( + 'logicalId', + new Error(`A ${type} with logicalId "${logicalId}" already exists.`), + ), + )) + return } this.data[type as keyof ProjectData][logicalId] = resource diff --git a/packages/cli/src/constructs/session.ts b/packages/cli/src/constructs/session.ts index 43912f408..94bc6a398 100644 --- a/packages/cli/src/constructs/session.ts +++ b/packages/cli/src/constructs/session.ts @@ -5,7 +5,6 @@ import { CheckConfigDefaults } from '../services/checkly-config-loader.js' import { type EngineDetectionResult } from '../services/engine-detector.js' import { Parser } from '../services/check-parser/parser.js' import { Construct } from './construct.js' -import { ValidationError } from './validator-error.js' import { PrivateLocationApi } from '../rest/private-locations.js' import { FileLoader, @@ -158,10 +157,6 @@ export class Session { } static validateCreateConstruct (construct: Construct) { - if (typeof construct.logicalId !== 'string') { - throw new ValidationError(`The "logicalId" of a ${construct.type} construct must be a string (logicalId=${construct.logicalId} [${typeof construct.logicalId}])`) - } - if (construct.type === PROJECT_CONSTRUCT_TYPE) { // Creating the construct is allowed - We're creating the project. } else if (Session.project) { diff --git a/packages/cli/src/constructs/validator-error.ts b/packages/cli/src/constructs/validator-error.ts deleted file mode 100644 index 9595a161f..000000000 --- a/packages/cli/src/constructs/validator-error.ts +++ /dev/null @@ -1 +0,0 @@ -export class ValidationError extends Error {}