fix(tests): resolve failing tests in TemplateArchiveProcessor and JavaScriptEvaluator (#65)#79
Conversation
Signed-off-by: Mohamed Dawoud <mhmaddawoud20@gmail.com>
Signed-off-by: Mohamed Dawoud <mhmaddawoud20@gmail.com>
mttrbrts
left a comment
mttrbrts
left a comment
There was a problem hiding this comment.
LGTM! Clean fix for the test failures from #65. Thank you @mdawoud27!
This comment was generated by AI on behalf of @mttrbrts.
There was a problem hiding this comment.
Pull request overview
Fixes two failing tests by addressing a child-process IPC race in JS evaluation and correcting JSON-path generation for TemplateMark root nodes.
Changes:
- Update
src/worker.jsto delay process exit until after the IPC send callback fires (to reduce flakiness inJavaScriptEvaluatorstress tests). - Update
getJsonPathinsrc/TemplateMarkInterpreter.tsto ignore the top-level node name"top"(fixing incorrect JSON paths leading to validation failures).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/worker.js |
Adds process.send callbacks + delayed exits to avoid losing IPC messages before worker termination. |
src/TemplateMarkInterpreter.ts |
Adjusts JSON path construction to skip the "top" node name. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| process.send({ result }, () => { | ||
| setTimeout(() => { | ||
| process.exit(); | ||
| }, 50); | ||
| }); |
There was a problem hiding this comment.
The extra 50ms delay after process.send adds a fixed latency to every child-process evaluation and can significantly slow down/stress timeouts under load. The process.send callback already indicates the message has been flushed to the IPC channel, so consider exiting immediately in the callback (or prefer process.exitCode = 0/1 + process.disconnect() to let the process exit naturally) instead of setTimeout(..., 50).
| process.send({ message: err.toString() }, () => { | ||
| setTimeout(() => { | ||
| process.exit(1); | ||
| }, 50); | ||
| }); |
There was a problem hiding this comment.
Same as the success path: delaying exit by 50ms after sending the error message adds avoidable per-request latency and can impact throughput. Consider exiting immediately in the process.send callback (or set process.exitCode = 1 and process.disconnect()), rather than sleeping for 50ms.
| } | ||
|
|
||
| if (currentNode.name !== 'this') { | ||
| if (currentNode.name !== "this" && currentNode.name !== "top") { |
There was a problem hiding this comment.
This line introduces double-quoted string literals in a file that otherwise consistently uses single quotes (e.g., obj.name !== 'top' above). For consistency and to reduce noisy diffs, use single quotes here as well.
| if (currentNode.name !== "this" && currentNode.name !== "top") { | |
| if (currentNode.name !== 'this' && currentNode.name !== 'top') { |
|
@copilot apply changes based on the comments in this thread |
Address Copilot review comments: - Remove setTimeout delays in process.exit() callbacks - Use single quotes for consistency in TemplateMarkInterpreter.ts The process.send callback already indicates message delivery, so exiting immediately is safe and improves throughput. Signed-off-by: Matt Roberts <code@rbrts.uk>
|
I've pushed a commit to address the Copilot review comments: Changes made:
All 50 tests still pass. |
Coverage Report for CI Build 24966232529Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Warning No base build found for commit Coverage: 63.844%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
Fixes #65
Resolves two test failures:
worker.js): Added a callback and delay toprocess.send()to ensure IPC messages are flushed before the worker exits. Fixes flakyJavaScriptEvaluatorstress tests.TemplateMarkInterpreter.ts): UpdatedgetJsonPathto ignore the top-level node name'top', fixing incorrect JSON paths that causedValidationExceptioninTemplateArchiveProcessor.Verified that all tests pass
Signed-off-by: Mohamed Dawoud mhmaddawoud20@gmail.com