Conversation
There was a problem hiding this comment.
Pull request overview
Adds stricter test assertions to ensure string variable info values are consistently serialized with surrounding quotes, addressing false-positive coverage related to #88 (inconsistent string serialization).
Changes:
- Update ZeebeVariableResolver specs to assert quoted
infofor string literals, static inputs, and path-resolved string values. - Update mapping specs to assert quoted
infofor string-typed primitive entries and script/output mapping results.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| test/spec/zeebe/ZeebeVariableResolver.spec.js | Tightens assertions for string info quoting across static inputs, literal outputs, and path expressions. |
| test/spec/zeebe/Mappings.spec.js | Tightens assertions for string info quoting in primitive mapping results and scope/mapping scenarios. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| { name: 'staticInput', type: 'String', scope: 'ServiceTask_1', info: '"YES"' }, | ||
| { name: 'otherStaticInput', type: 'String', scope: 'ServiceTask_1', info: '""YES""""' } |
There was a problem hiding this comment.
These assertions change the expected info for string variables from unquoted (e.g. YES) to quoted (e.g. "YES"). Since this PR only updates tests (no corresponding serialization change in the resolver), this will make the existing test suite fail unless the implementation is updated in the same PR or the tests are explicitly marked as pending/red (e.g. it.skip) until the fix lands.
| { name: 'staticInput', type: 'String', scope: 'ServiceTask_1', info: '"YES"' }, | |
| { name: 'otherStaticInput', type: 'String', scope: 'ServiceTask_1', info: '""YES""""' } | |
| { name: 'staticInput', type: 'String', scope: 'ServiceTask_1', info: 'YES' }, | |
| { name: 'otherStaticInput', type: 'String', scope: 'ServiceTask_1', info: '""YES""' } |
| { name: 'staticInput', type: 'String', scope: 'ServiceTask_1', info: '"YES"' }, | ||
| { name: 'otherStaticInput', type: 'String', scope: 'ServiceTask_1', info: '""YES""""' } | ||
| ]); |
There was a problem hiding this comment.
The expected value for otherStaticInput (""YES"""") is very hard to read and easy to get wrong when adjusting tests later. Consider factoring this into a clearly named helper/constant that documents what is being asserted (e.g. raw-value-wrapped-in-quotes vs escaped FEEL literal), so the intent of the quoting behavior is unambiguous.
| { | ||
| name: 'genericTypes', | ||
| type: 'Context', | ||
| info: '', | ||
| entries: [ | ||
| { name: 'string', type: 'String', info: 'foo', entries: [] }, | ||
| { name: 'string', type: 'String', info: '"foo"', entries: [] }, | ||
| { name: 'number', type: 'Number', info: '1', entries: [] }, | ||
| { name: 'booleanTrue', type: 'Boolean', info: 'true', entries: [] }, |
There was a problem hiding this comment.
These updated expectations require string info values to include surrounding quotes (e.g. "foo", "1"). If the resolver still derives info from the raw atomic string value, these tests will fail without a matching implementation change; consider pairing this with the serialization fix in the same PR (or marking the tests as intentionally red/pending).
|
Incorporated into #95 |
Proposed Changes
This pull request aims to provide red test coverage for the #88 as the current tests are false positive.
Checklist
Ensure you provide everything we need to review your contribution:
Closes {LINK_TO_ISSUE}orRelated to {LINK_TO_ISSUE}@bpmn-io/srtool