test(backend/shared): de-XML-ify stale compile/library-build suites - #918
Conversation
These suites predate the XML/xml2st → in-process JSON-transpiler migration and
never ran in CI (the unit-tests job was broken), so they drifted:
- compile/pipeline.test.ts: rename transpileXmlToSt→transpileToSt, assert the
current {projectData} call contract, drop the dead XmlGenerator mock + XML
stage test, add a simulator-no-binary edge test.
- library/build-pipeline.test.ts: drop the XmlGenerator mock and the removed
XML-generation/empty-data paths; assert prepareXmlForLibraryBuild's current
{projectData,knownPous,manifest}|{error} contract.
- library/library-build-orchestrator.test.ts: fake port uses transpileToSt
({projectData}, log); remove plc.xml intermediates/logs; restore coverage.
All three source files back to 100% funcs/lines/statements. 95 tests pass.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
WalkthroughTest suites for the compile pipeline and library build pipeline/orchestrator are refactored to remove XmlGenerator/transpileXmlToSt mocking and instead mock/assert the transpileToSt port method. New failure-mode tests cover transpiler errors, missing hex/programSt outputs, verification-cache resilience, and .stlib write failures. ChangesTest suite migration to transpileToSt
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Test as Test Harness
participant Orchestrator as runLibraryBuildPipeline
participant Port as LibraryBuildPort
participant Cache as VerifyCache
Test->>Orchestrator: run pipeline
Orchestrator->>Port: prepareXmlForLibraryBuild
Port-->>Orchestrator: projectData
Orchestrator->>Port: transpileToSt(projectData, log)
Port-->>Orchestrator: { ok: true, programSt: FAKE_PROGRAM_ST }
Orchestrator->>Cache: compute MD5(FAKE_PROGRAM_ST), check cache
Cache-->>Orchestrator: cache hit/miss
Orchestrator-->>Test: stage events + result
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/backend/shared/library/__tests__/library-build-orchestrator.test.ts (1)
174-184: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
arrayContainingdoesn't verify stage order despite the test's intent.The comment says "Stage messages flow through in order," but
expect.arrayContainingonly checks that all listed messages are present anywhere in the array — it ignores ordering and permits extra entries. This test can't catch a regression where stages fire out of sequence (e.g., transpile running after verification).Consider filtering
eventsdown to just the messages under test and comparing with a plaintoEqual(order-sensitive), or asserting index positions directly.♻️ Suggested fix
- expect(events.map((e) => e.message)).toEqual( - expect.arrayContaining([ - 'Starting library build...', - 'Manifest OK — building "lib" v0.1.0.', - 'Transpiling project to Structured Text', - 'Verifying with OpenPLC Simulator (avr-gcc)...', - 'Compiling library archive...', - 'Library built successfully: build/lib.stlib', - ]), - ) + const canonicalMessages = [ + 'Starting library build...', + 'Manifest OK — building "lib" v0.1.0.', + 'Transpiling project to Structured Text', + 'Verifying with OpenPLC Simulator (avr-gcc)...', + 'Compiling library archive...', + 'Library built successfully: build/lib.stlib', + ] + expect(events.map((e) => e.message).filter((m) => canonicalMessages.includes(m))).toEqual(canonicalMessages)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/shared/library/__tests__/library-build-orchestrator.test.ts` around lines 174 - 184, The stage-order assertion in the library build orchestrator test is too loose because library-build-orchestrator.test.ts uses expect.arrayContaining on events.map((e) => e.message), which does not enforce sequencing. Update the test around the events/message assertions to compare the relevant messages with an order-sensitive check, or assert their relative indices directly, so the flow through build stages is verified in the intended order.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/backend/shared/library/__tests__/library-build-orchestrator.test.ts`:
- Around line 174-184: The stage-order assertion in the library build
orchestrator test is too loose because library-build-orchestrator.test.ts uses
expect.arrayContaining on events.map((e) => e.message), which does not enforce
sequencing. Update the test around the events/message assertions to compare the
relevant messages with an order-sensitive check, or assert their relative
indices directly, so the flow through build stages is verified in the intended
order.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 60115ac9-4d88-44fe-a82d-e17a27f3adfa
📒 Files selected for processing (3)
src/backend/shared/compile/__tests__/pipeline.test.tssrc/backend/shared/library/__tests__/build-pipeline.test.tssrc/backend/shared/library/__tests__/library-build-orchestrator.test.ts
1 participant
Follow-up to #917 (which unblocked the unit-tests CI job). With the job now running, it surfaced three
backend/sharedsuites that predate the XML/xml2st → in-process JSON-transpiler migration and never ran in CI, so they'd drifted against renamed/removed APIs.transpileXmlToSt→transpileToSt; assert the current{ projectData }call contract (not the removedxml2stArgs); drop the deadXmlGeneratormock + obsolete XML-stage failure test; add a simulator-no-.hexedge test to hold the gate.XmlGeneratormock and the removed XML-generation / empty-data paths; assertprepareXmlForLibraryBuild's current{ projectData, knownPous, manifest } | { error }contract.transpileToSt({ projectData }, log); removeplc.xmlintermediates/logs; restore coverage.All three source files back to 100% functions/lines/statements (branches gate is 0). 95 tests pass (editor jest). Source untouched — test-only.
Not addressed: the VPP alias-sync failure (#4 in the earlier report) — left for separate triage per the maintainer.
🤖 Generated with Claude Code
Summary by CodeRabbit