[#665] Fixed 'XmlTrait' XML-format assertions ignoring fixture content.#667
Conversation
WalkthroughXmlTrait's XML format validity assertions now resolve content from fixture-set data ($xmlTestContent) when available, falling back to page content, instead of always reading live page content. New Behat scenarios verify this behavior, and STEPS.md documentation is updated with corresponding examples. ChangesXmlTrait Fixture Content Resolution
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Scenario
participant XmlTrait
participant Session
Scenario->>XmlTrait: the response should be in XML format
XmlTrait->>XmlTrait: xmlEnsureDocument()
alt xmlTestContent set
XmlTrait->>XmlTrait: use fixture content
else
XmlTrait->>Session: getPage().getContent()
Session-->>XmlTrait: page content
end
XmlTrait-->>Scenario: assertion result
Scenario->>XmlTrait: the response should not be in XML format
alt xmlTestContent set
XmlTrait->>XmlTrait: use fixture content
else
XmlTrait->>Session: getPage().getContent()
Session-->>XmlTrait: page content
end
XmlTrait-->>Scenario: assertion result
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. 🔧 markdownlint-cli2 (0.22.1)STEPS.mdmarkdownlint-cli2 wrapper config was not available before execution Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/XmlTrait.php (1)
149-164: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDuplicated content-resolution and XML-load logic.
Line 153 re-implements the "fixture content first, else page content" fallback that
xmlEnsureDocument()(lines 614-631) already encodes, and lines 155-160 duplicate the load/error-check logic fromxmlLoadDocument()(584-604). Two independent implementations of the same rule increase the risk of drift if the fallback semantics change later.Consider extracting a shared helper (e.g.
xmlResolveContent(): string) for the fixture-vs-page fallback, and/or a helper that attempts to load XML and returns the resulting\LibXMLError[]without throwing, reused by bothxmlLoadDocument()andxmlAssertResponseIsNotXml().♻️ Suggested refactor
+ /** + * Resolve the XML content to validate. + * + * Prefers content set by fixture steps, falling back to the page content. + */ + protected function xmlResolveContent(): string { + return $this->xmlTestContent ?? (string) $this->getSession()->getPage()->getContent(); + } + #[Then('the response should not be in XML format')] public function xmlAssertResponseIsNotXml(): void { - // Resolve content the same way as xmlEnsureDocument(): prefer content set - // by the fixture steps, falling back to the live page content. - $content = $this->xmlTestContent ?? $this->getSession()->getPage()->getContent(); + $content = $this->xmlResolveContent();🤖 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/XmlTrait.php` around lines 149 - 164, The XML response check duplicates both the fixture-vs-page content fallback and the XML parsing/error collection already handled by xmlEnsureDocument() and xmlLoadDocument(), so refactor xmlAssertResponseIsNotXml() to reuse shared helpers instead of re-implementing that logic. Extract a common content resolver such as xmlResolveContent() for the xmlTestContent/getPage()->getContent() fallback, and reuse the existing XML load path or a non-throwing helper that returns libxml errors so the behavior stays consistent in XmlTrait.
🤖 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.
Outside diff comments:
In `@src/XmlTrait.php`:
- Around line 149-164: The XML response check duplicates both the
fixture-vs-page content fallback and the XML parsing/error collection already
handled by xmlEnsureDocument() and xmlLoadDocument(), so refactor
xmlAssertResponseIsNotXml() to reuse shared helpers instead of re-implementing
that logic. Extract a common content resolver such as xmlResolveContent() for
the xmlTestContent/getPage()->getContent() fallback, and reuse the existing XML
load path or a non-throwing helper that returns libxml errors so the behavior
stays consistent in XmlTrait.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7f1a7ae4-2c92-4467-b664-6f74a862f179
📒 Files selected for processing (3)
STEPS.mdsrc/XmlTrait.phptests/behat/features/xml.feature
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #667 +/- ##
==========================================
- Coverage 96.85% 96.85% -0.01%
==========================================
Files 46 46
Lines 3819 3818 -1
==========================================
- Hits 3699 3698 -1
Misses 120 120 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
AlexSkrypnyk
added
the
Needs review
1 participant
Closes #665
Summary
XmlTraitprovides two fixture steps,Given the response content from the file :filenameandGiven the response content is the following:, that store content in$xmlTestContentso assertions can run without an HTTP request. Every other assertion in the trait resolves content throughxmlEnsureDocument(), which prefers$xmlTestContentand falls back to the live page - except the two format-validity assertions,Then the response should be in XML formatandThen the response should not be in XML format, which read$this->getSession()->getPage()->getContent()directly and therefore ignored fixture-set content, validating only the live page (and throwing a MinkDriverExceptionwhen no page had been visited yet).Changes
xmlAssertResponseIsXml()now delegates to$this->xmlEnsureDocument(), reusing the same fixture-or-page resolution and document cache as every other assertion, while still throwing the existing "Failed to load XML" error on invalid content.xmlAssertResponseIsNotXml()now resolves content as$this->xmlTestContent ?? $this->getSession()->getPage()->getContent(), keeping its "expect invalid" behaviour and the "The response is valid XML, but it should not be." exception.tests/behat/features/xml.featurethat visit a page with one validity, then override it with a fixture holding the other, asserting the fixture wins - covering both assertions via the file fixture plus one via a PyString.@codedocblock examples on both methods to show fixture usage, and regeneratedSTEPS.mdvia the docs generator.Screenshots
N/A - internal Behat trait change with no rendered UI.
Before / After
Updated
XmlTraitso XML format assertions now honor fixture-provided content first, then fall back to the live page when no fixture content is set. Added Behat coverage for fixture-driven valid/invalid XML checks, clarified the step examples in docblocks, and regeneratedSTEPS.md.No step-definition rule violations were identified in the reviewed changes.