Fix CI: add missing data.xml dependency and fix duplicate XML declaration - #1
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9500fcdac8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| (let [xml-body (-> (xml/emit-str (xml/sexp-as-element coverage-sexp)) | ||
| (str/replace-first #"^<\?xml[^?]*\?>\s*" ""))] |
There was a problem hiding this comment.
Switching from xml/emit to xml/emit-str turns XML generation from a streaming write into full in-memory materialization of the Cobertura report. For large codebases, this can significantly increase heap usage in CI and may cause OOMs where the previous implementation would succeed, because the whole document must fit in memory before any bytes are written.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed — switched to streaming xml/emit with the :doctype keyword arg (supported in data.xml 0.2.0-alpha10). No more in-memory buffering; XML streams directly to the file writer.
Generated by Claude Code
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/cloverage/coverage/cobertura.clj (1)
151-158: Workaround approach is sound, but regex could be slightly more robust.The manual prolog + strip approach correctly addresses the duplicate XML declaration issue. The regex
#"^<\?xml[^?]*\?>\s*"works for typical output but may fail if the declaration contains a?in an encoding name (unlikely but possible). A more defensive pattern:(str/replace-first #"^<\?xml[^>]*\?>\s*" "")However, since
emit-strproduces predictable output (<?xml version="1.0" encoding="UTF-8"?>), the current regex is acceptable in practice.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cloverage/coverage/cobertura.clj` around lines 151 - 158, The regex used to strip the XML declaration when writing the coverage output in the block that calls xml/emit-str and xml/sexp-as-element is slightly brittle (it uses str/replace-first with #"^<\?xml[^?]*\?>\s*"); update that replacement to use a more robust pattern such as matching up to the closing '>' (e.g., #"^<\?xml[^>]*\?>\s*") so the declaration is always removed reliably before writing xml-body to writer; keep the surrounding logic (manual prolog, coverage-sexp, xml-body, writer writes) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@project.clj`:
- Line 7: The project declares dependency org.clojure/data.xml "0.2.0-alpha9"
which is outdated; update that dependency version in project.clj to either
"0.2.0-alpha10" if you need the newer XML namespace and ClojureScript support,
or to the stable "0.0.8" if you prefer a non-alpha release—edit the
org.clojure/data.xml entry accordingly so the project uses the chosen version.
---
Nitpick comments:
In `@src/cloverage/coverage/cobertura.clj`:
- Around line 151-158: The regex used to strip the XML declaration when writing
the coverage output in the block that calls xml/emit-str and xml/sexp-as-element
is slightly brittle (it uses str/replace-first with #"^<\?xml[^?]*\?>\s*");
update that replacement to use a more robust pattern such as matching up to the
closing '>' (e.g., #"^<\?xml[^>]*\?>\s*") so the declaration is always removed
reliably before writing xml-body to writer; keep the surrounding logic (manual
prolog, coverage-sexp, xml-body, writer writes) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4e9a0197-0247-4d75-a9b0-79ee241daa58
📒 Files selected for processing (3)
deps.ednproject.cljsrc/cloverage/coverage/cobertura.clj
@codex fix |
|
To use Codex here, create an environment for this repo. |
There was a problem hiding this comment.
Pull request overview
This PR fixes Cobertura report generation failures in CI by adding the missing clojure.data.xml dependency and adjusting Cobertura XML output generation to avoid an invalid duplicate XML declaration.
Changes:
- Add
org.clojure/data.xmlto bothproject.cljanddeps.edn. - Update Cobertura report writer to generate XML via
emit-strand strip the auto-emitted XML declaration to prevent duplicate declarations.
Reviewed changes
Copilot reviewed 1 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/cloverage/coverage/cobertura.clj |
Adjusts XML emission to avoid duplicate XML declarations in Cobertura output. |
project.clj |
Adds the missing org.clojure/data.xml dependency for Leiningen builds. |
deps.edn |
Adds the missing org.clojure/data.xml dependency for tools.deps builds. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…tion The cobertura reporter requires clojure.data.xml but it was not declared as a dependency in project.clj or deps.edn, causing runtime failures. Also fix xml/emit usage: the :xml-declaration false option is not supported by data.xml, so emit was producing a duplicate XML declaration making the output invalid. Switch to emit-str and strip the declaration manually. https://claude.ai/code/session_01WpM3eidMhXdmEvo9DRX1vH
…ta.xml - Use xml/emit with :doctype option instead of emit-str, keeping XML generation streaming rather than buffering the entire report in memory (addresses Codex P2 review comment) - Upgrade org.clojure/data.xml from 0.2.0-alpha9 to 0.2.0-alpha10 (addresses CodeRabbit review comment) https://claude.ai/code/session_01WpM3eidMhXdmEvo9DRX1vH
5ffe239 to
48218f1
Compare
The cobertura reporter requires clojure.data.xml but it was not declared
as a dependency in project.clj or deps.edn, causing runtime failures.
Also fix xml/emit usage: the :xml-declaration false option is not supported
by data.xml, so emit was producing a duplicate XML declaration making the
output invalid. Switch to emit-str and strip the declaration manually.
https://claude.ai/code/session_01WpM3eidMhXdmEvo9DRX1vH
Summary by CodeRabbit
Chores
Refactor