From 9f6cd61ca30eea7d7eda1512ef78518c30e8dd71 Mon Sep 17 00:00:00 2001 From: Marlon Kranz Date: Wed, 30 Sep 2026 22:35:25 +0200 Subject: [PATCH] Name the trigger in a name-less signal handler's error, and test untested command paths `on signal { }` reported "A message handler needs the message's name". The handler check now names its own trigger, as the start-event and await checks already do. The extension tests now cover open-and-decompile, declining the overwrite prompt, unsaved edits winning over the file on disk, and decompile with nothing selected. The CLI tests now cover exit code 2 for a missing input, an unreadable input and an unwritable output. CONTRIBUTING.md records why `.nvmrc` pins Node 24.19.0: 24.20.0 and 24.21.0 fail `langium generate` with `ERR_INVALID_URL`. --- CONTRIBUTING.md | 1 + packages/cli/test/build-parse.smoke.test.ts | 43 +++++++++++ packages/extension/test/conversion.test.ts | 76 +++++++++++++++++++ .../language/src/bpmn-script-validator.ts | 9 ++- packages/language/test/validating.test.ts | 10 +-- packages/transform/src/ast-to-ir.ts | 4 +- 6 files changed, 133 insertions(+), 10 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index cbcca93..707ccb5 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -11,6 +11,7 @@ ### Prerequisites - [Node.js](https://nodejs.org/) >= 22 (the project uses Node 24 via `.nvmrc`; [nvm](https://github.com/nvm-sh/nvm)'s `nvm use` reads it) + `.nvmrc` pins `24.19.0` because 24.20.0 and 24.21.0 fail `langium generate` with `ERR_INVALID_URL`; move it back to `24` once a later 24.x generates cleanly. - [VS Code](https://code.visualstudio.com/) with the recommended extensions (VS Code will prompt you on first open) - [Docker](https://www.docker.com/) for end-to-end tests (alternatives: [Podman](https://podman.io/), [Rancher Desktop](https://rancherdesktop.io/)) diff --git a/packages/cli/test/build-parse.smoke.test.ts b/packages/cli/test/build-parse.smoke.test.ts index a4b4a95..f7ac311 100644 --- a/packages/cli/test/build-parse.smoke.test.ts +++ b/packages/cli/test/build-parse.smoke.test.ts @@ -459,6 +459,49 @@ describe('bpmns build / bpmns parse: guards against destructive or unclear failu line: `Error: ${dir} is a directory`, }), ], + [ + 'build: a missing input is refused', + 'build', + (dir) => { + const inputPath = path.join(dir, 'missing.bpmnscript'); + return { + inputPath, + opts: {}, + exit: 2, + line: `Error: file not found: ${inputPath}`, + }; + }, + ], + [ + 'parse: an unreadable input is refused', + 'parse', + (dir) => { + const { inputPath } = writeInput(dir, 'parse'); + fs.chmodSync(inputPath, 0o000); + return { + inputPath, + opts: {}, + exit: 2, + line: `Error: could not read ${inputPath}: EACCES: permission denied, open '${inputPath}'`, + }; + }, + ], + [ + 'build: an unwritable output is refused', + 'build', + (dir) => { + const { inputPath } = writeInput(dir, 'build'); + const readOnly = path.join(dir, 'read-only'); + fs.mkdirSync(readOnly, 0o555); + const outputPath = path.join(readOnly, 'order.bpmn'); + return { + inputPath, + opts: { output: outputPath }, + exit: 2, + line: `Error: could not write output to ${outputPath}: EACCES: permission denied, open '${outputPath}'`, + }; + }, + ], [ 'build: an -o naming a directory writes inside it under the default basename', 'build', diff --git a/packages/extension/test/conversion.test.ts b/packages/extension/test/conversion.test.ts index 583c2f4..01e5250 100644 --- a/packages/extension/test/conversion.test.ts +++ b/packages/extension/test/conversion.test.ts @@ -59,6 +59,7 @@ import type { import { compileCommand, decompileCommand, + pickBpmnAndDecompileCommand, } from '../src/extension/conversion.js'; function notifications(): Record<'info' | 'warning' | 'error', string[]> { @@ -220,6 +221,18 @@ describe('conversion commands: what the author is shown', () => { returns: undefined, }, ], + [ + 'decompile with nothing selected and no editor open says so and stops', + 'decompile', + undefined, + undefined, + { + warning: [ + 'BPMNscript: No file selected. Open a .bpmn file or select one in the Explorer.', + ], + returns: undefined, + }, + ], [ 'compile takes an upper-case extension', 'compile', @@ -269,3 +282,66 @@ describe('conversion commands: what the author is shown', () => { expect(core).toHaveBeenCalledTimes(result === undefined ? 0 : 1); }); }); + +describe('conversion commands: what reaches the disk', () => { + beforeEach(() => { + vi.clearAllMocks(); + vi.mocked(compileDslToBpmn).mockResolvedValue({ + ok: true, + output: '', + }); + }); + + test('unsaved edits are converted, not the stale file on disk', async () => { + const uri = vscode.Uri.file('/tmp/example.bpmnscript'); + const openDocs = vscode.workspace.textDocuments as vscode.TextDocument[]; + openDocs.push({ uri, getText: () => 'EDITED' } as vscode.TextDocument); + try { + await compileCommand('0.0.1')(uri); + } finally { + openDocs.length = 0; + } + expect(compileDslToBpmn).toHaveBeenCalledWith('EDITED', '0.0.1'); + expect(vscode.workspace.fs.readFile).not.toHaveBeenCalled(); + }); + + test.each([ + ['declining the prompt writes nothing', undefined, undefined, 0], + [ + "answering 'Overwrite' writes the output", + 'Overwrite', + '/tmp/example.bpmn', + 1, + ], + ])('an existing output: %s', async (_title, answer, returns, writes) => { + vi.mocked(vscode.workspace.fs.stat).mockResolvedValueOnce( + {} as vscode.FileStat, + ); + mocks.showWarningMessage.mockResolvedValueOnce(answer); + + const returned = await compileCommand('0.0.1')( + vscode.Uri.file('/tmp/example.bpmnscript'), + ); + + expect(returned?.fsPath).toBe(returns); + expect(vscode.workspace.fs.writeFile).toHaveBeenCalledTimes(writes); + }); + + test.each([ + ['a cancelled dialog decompiles nothing', undefined, []], + [ + 'the picked file is decompiled', + [vscode.Uri.file('/tmp/example.bpmn')], + ['/tmp/example.bpmn'], + ], + ])('open and decompile: %s', async (_title, picked, decompiled) => { + vi.mocked(vscode.window.showOpenDialog).mockResolvedValueOnce(picked); + const decompile = vi.fn(); + + await pickBpmnAndDecompileCommand(decompile)(); + + expect( + decompile.mock.calls.map((call) => (call[0] as vscode.Uri).fsPath), + ).toEqual(decompiled); + }); +}); diff --git a/packages/language/src/bpmn-script-validator.ts b/packages/language/src/bpmn-script-validator.ts index b7674fa..0f64c47 100644 --- a/packages/language/src/bpmn-script-validator.ts +++ b/packages/language/src/bpmn-script-validator.ts @@ -3745,10 +3745,11 @@ export class BpmnScriptValidator { const payload = payloadItemOf(handler.items); if (rule.code === 'required') { if (!code) { - accept('error', nameRequiredMessage('A message handler', 'message'), { - node: handler, - property: 'trigger', - }); + accept( + 'error', + nameRequiredMessage(`A ${handler.trigger} handler`, handler.trigger), + { node: handler, property: 'trigger' }, + ); } } else if (rule.code === 'optional') { checkEmptyCode(code, handler.items, accept); diff --git a/packages/language/test/validating.test.ts b/packages/language/test/validating.test.ts index 79ecb5d..7edaa67 100644 --- a/packages/language/test/validating.test.ts +++ b/packages/language/test/validating.test.ts @@ -431,8 +431,8 @@ const HANDLER_PLACEMENT = const HANDLER_TRAILING = 'Event handlers read like catch blocks: move it after the last step of ' + 'this body.'; -const MESSAGELESS_NAME = - "A message handler needs the message's name: the engine matches messages by name."; +const nameless = (trigger: string) => + `A ${trigger} handler needs the ${trigger}'s name: the engine matches ${trigger}s by name.`; const CONDITION_REQUIRED = "A condition handler needs its condition: 'on condition(amount > 100)'."; const CONDITION_NO_CODE = @@ -2517,12 +2517,12 @@ checks('Validation - event handlers', [ [ 'a name-less message handler asks for the name', `process p { on message { user A } }`, - [noFlowSteps('p'), MESSAGELESS_NAME], + [noFlowSteps('p'), nameless('message')], ], [ 'a name-less signal handler asks for the name', `process p { on signal { user A } }`, - [noFlowSteps('p'), MESSAGELESS_NAME], + [noFlowSteps('p'), nameless('signal')], ], [ 'a payload-less timer handler asks how to read the time', @@ -2547,7 +2547,7 @@ checks('Validation - event handlers', [ [ 'a timer key on a signal handler belongs to a timer', `process p { on signal(at: "PT1H") { user A } }`, - [noFlowSteps('p'), MESSAGELESS_NAME, PARTICLE_ONLY], + [noFlowSteps('p'), nameless('signal'), PARTICLE_ONLY], ], [ 'a code string on a condition handler is not the condition', diff --git a/packages/transform/src/ast-to-ir.ts b/packages/transform/src/ast-to-ir.ts index f7058df..9d09bce 100644 --- a/packages/transform/src/ast-to-ir.ts +++ b/packages/transform/src/ast-to-ir.ts @@ -3,7 +3,9 @@ * static position in the block tree, never a traversal counter), so lowering * `irToDsl` output again yields identical ids. A sub-process body is rooted at * its coordinate, not its name: gateway ids skip `resolveCollision`, so a - * sub-process named like a coordinate could otherwise duplicate one. + * sub-process named like a coordinate could otherwise duplicate one. The + * validator's reserved `Gateway_..._split` pattern relies on that too: it is + * end-anchored, so a `_2` collision suffix would slip past it. * * Throws only on a program the validator rejects. */