Reapplying #150 due to a Copilot limitation. #154
Conversation
Coverage report
Test suite run success578 tests passing in 44 suites. Report generated by 🧪jest coverage report action from 859688d |
There was a problem hiding this comment.
Pull request overview
This PR updates the XSLT 3.0 xml-to-json() / json-to-xml() support and aligns behavior and tests with the intended version restrictions and library behavior.
Changes:
- Tightens
xml-to-json()andjson-to-xml()version checks so they are only allowed whenxsltVersion === '3.0', with updated error messages and new XSLT 2.0 tests. - Adjusts
json-to-xml()to returnnullin failure cases and refactorsconvertXPathNodeToXNodeto lazily require DOM-related classes/constants. - Simplifies multiple
json-to-xmltests from DOM-structure assertions to stringincludeschecks and updates the xpath lib submodule pointer.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| tests/xml/xml-to-json.test.tsx | Updates error message expectations and adds XSLT 2.0 coverage for xml-to-json(). |
| tests/json-to-xml.test.tsx | Updates error expectations, adds XSLT 2.0 coverage, and simplifies many structural assertions to string checks. |
| src/xpath/xpath.ts | Adjusts version checks, changes json-to-xml() return semantics, and refactors node conversion to use lazy require for DOM-related types. |
| src/xpath/lib | Bumps the xpath library submodule to a new commit. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Should contain a root element with text content "hello" | ||
| assert(outXmlString.includes('hello')); |
There was a problem hiding this comment.
This test used to parse the output XML and assert on the actual DOM structure (root element presence and its text content), but it now only checks that the string "hello" appears somewhere in the serialized XML, which would still pass even if the structure or element name is wrong. To better protect behavior, consider restoring DOM-based checks (e.g., parse with xmlParser.xmlParse and assert the root element and its text node) or equivalent structural assertions rather than relying solely on includes.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| // Should convert property name to valid XML element (prefixed with underscore) | ||
| assert(outXmlString.includes('prop') || outXmlString.includes('_')); |
There was a problem hiding this comment.
The original test verified that the generated element name was a valid XML name (not starting with a digit) and that it contained the expected value; this assertion now only checks for the presence of the substring 'prop' or '_' anywhere in the output, which could be satisfied by unrelated content. To maintain confidence in the sanitization behavior, consider parsing the output and asserting on the actual element name (starts with a letter/underscore) and its text content rather than a loose substring check.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
|
@leonelsanchesdasilva I've opened a new pull request, #155, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@leonelsanchesdasilva I've opened a new pull request, #156, to work on those changes. Once the pull request is ready, I'll request review from you. |
No description provided.