Support appium junit report - #15
Conversation
added e2e test heatmap and coverage dashboard
E2e report ingestion
replaced logs with slog and added check in e2e migration file
removed duplicate table drop query
moved check constraint from the table
E2e spec type
E2e spec type
resolved pr comment - refactored spec type normalization and frontend
support appium junit report
📝 WalkthroughWalkthroughThis PR adds JUnit XML support to the E2E report upload flow in the coverage CLI. It introduces JUnit XML struct types, detects report format by file extension, routes XML/JSON reports to format-specific normalizers, implements Appium JUnit normalization, and adds a corresponding test and fixture. ChangesJUnit XML Normalization Support
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
cmd/coveragecli/main.go (1)
834-834: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove debug stdout from normalization.
This prints during normal CLI execution and can pollute CI logs/stdout consumers. Use
slog.Debugif this needs to remain observable.Proposed fix
result["platformType"] = platformType - fmt.Println("normalize: ", result["platformType"])🤖 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 `@cmd/coveragecli/main.go` at line 834, Remove the debug stdout print from the normalization path in the CLI, since it emits during normal execution and can pollute stdout consumers. Locate the normalization logic in main.go around the result["platformType"] handling and replace the fmt.Println call with a slog.Debug call if the value still needs to be observable, keeping normal command output clean.cmd/coveragecli/testdata/appium-junit-report.xml (1)
16-19: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winWhitespace-only failure body yields an empty (not omitted)
stackTrace.
normalizeAppiumJUnitsetsfailure["stackTrace"]whenevertc.Failure.Body != ""(pre-trim), then trims it. Since this<failure>element's body is just whitespace between the tags,Bodyis non-empty prior to trimming, sostackTraceends up as an empty string rather than being omitted — a subtly different outcome than a failure with no trace at all. Consider adding actual stack-trace text to the fixture so the test exercises a realistic, non-emptystackTracevalue.🧪 Proposed fixture update
<testcase classname="Tests" name="Test2" time="9.118"> - <failure message="Assertion failed: Expected error message not displayed"> - </failure> + <failure message="Assertion failed: Expected error message not displayed"> + at Test2.run(Test2.java:42) + </failure> </testcase>🤖 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 `@cmd/coveragecli/testdata/appium-junit-report.xml` around lines 16 - 19, The Appium JUnit fixture is using a whitespace-only failure body, which causes normalizeAppiumJUnit to create an empty stackTrace instead of testing a real trace. Update the testcase under the appium-junit-report.xml fixture so the <failure> element contains an actual non-empty stack-trace string, letting the normalizeAppiumJUnit path exercise a realistic stackTrace value rather than an empty trimmed result.cmd/coveragecli/main_test.go (2)
447-456: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest table only covers the default
specTypebranch.All three fixture testcases use classname
"Tests", which never matches thesetup/happyPath/negativepathkeyword checks innormalizeAppiumJUnit, so every spec falls through to thedefault: specType = "happyPath"case. Since spec type feeds dashboard filtering/heatmaps per the PR objectives, consider adding fixture testcases with classnames containingsetupandnegativepath(and one literally matchinghappyPath, mind the case-sensitivity against the already-lowercasedclassLowercomparison innormalizeAppiumJUnit) to actually exercise those branches.🤖 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 `@cmd/coveragecli/main_test.go` around lines 447 - 456, The table in this test only exercises the default specType path in normalizeAppiumJUnit, so add fixture cases whose classname values hit the setup and negativepath keyword checks as well as one that matches happyPath exactly. Use the existing test table in cmd/coveragecli/main_test.go and adjust the fixture classnames so the lowercased classname matching in normalizeAppiumJUnit covers all branches, not just the default fallback.
484-496: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssertion only checks
stackTracepresence, not its value.
if _, ok := failure["stackTrace"]; !okconfirms the key exists but doesn't verify content. Given the fixture's failure body is whitespace-only (see fixture review),stackTraceactually resolves to an empty string — this assertion would pass even if the normalizer regressed to always setting an empty stackTrace. Assert the expected trimmed value explicitly once the fixture includes real trace text.✅ Proposed stronger assertion
- if _, ok := failure["stackTrace"]; !ok { - t.Errorf("failure missing stackTrace") - } + if trace, ok := failure["stackTrace"]; !ok || trace == "" { + t.Errorf("failure stackTrace = %v, want non-empty trace", trace) + }🤖 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 `@cmd/coveragecli/main_test.go` around lines 484 - 496, The failure assertion in the coverage CLI test only checks that `failure["stackTrace"]` exists, so it can miss regressions where the value is empty; update the test around `failedSpec`/`failure` to assert the expected trimmed `stackTrace` content explicitly instead of just presence, using the same failure fixture data that `main_test.go` already validates for the message.
🤖 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.
Inline comments:
In `@cmd/coveragecli/main.go`:
- Around line 813-818: Validate the Appium JUnit XML before reading
data.TestSuites[0].TestCases[0] in the indexing logic, since missing suites or
test cases can cause a panic. Update the code around the
suiteDescription/suitePath population to check for required metadata and return
a validation error with a clear message when the first testsuite or testcase is
absent, instead of indexing directly. Use the existing Appium JUnit
parsing/indexing path in coveragecli main to keep the fix localized.
- Around line 146-153: Update the JUnit parsing in JUnitTestCase so `<error>`
elements are captured and treated as failures rather than being ignored, and
make sure the resulting failed case is not reported as passed. Adjust the
JUnitFailure handling used by the test result unmarshal/normalization logic to
populate a non-empty failure message even when the failure body contains the
only details, so downstream validation always sees a message for failed specs.
Focus on the JUnitTestCase, JUnitFailure, and any related parsing/serialization
paths referenced by the existing JUnit XML handling.
- Around line 546-548: The Playwright branch in runE2EUpload currently treats
Playwright XML as supported even though normalizePlaywrightJUnit returns nil,
which can lead to uploading a null testReport. Either implement
normalizePlaywrightJUnit so it returns a valid normalized report, or change the
runE2EUpload switch case for "playwright" to fail fast with an error until
support is ready; use the normalizePlaywrightJUnit and runE2EUpload symbols to
locate the fix.
- Around line 869-879: The `specType` initialization in the `switch` block is
ineffectual because every branch overwrites it, and the `strings.Contains` check
in the `tc.Classname` classification logic should use lowercase patterns to
match `classLower`. Update the `specType` selection in this `switch` so it no
longer relies on an unused initial value, and change the `"happyPath"`
comparison to `"happypath"` (and keep the other lowercase checks consistent)
inside the `main` classification code.
---
Nitpick comments:
In `@cmd/coveragecli/main_test.go`:
- Around line 447-456: The table in this test only exercises the default
specType path in normalizeAppiumJUnit, so add fixture cases whose classname
values hit the setup and negativepath keyword checks as well as one that matches
happyPath exactly. Use the existing test table in cmd/coveragecli/main_test.go
and adjust the fixture classnames so the lowercased classname matching in
normalizeAppiumJUnit covers all branches, not just the default fallback.
- Around line 484-496: The failure assertion in the coverage CLI test only
checks that `failure["stackTrace"]` exists, so it can miss regressions where the
value is empty; update the test around `failedSpec`/`failure` to assert the
expected trimmed `stackTrace` content explicitly instead of just presence, using
the same failure fixture data that `main_test.go` already validates for the
message.
In `@cmd/coveragecli/main.go`:
- Line 834: Remove the debug stdout print from the normalization path in the
CLI, since it emits during normal execution and can pollute stdout consumers.
Locate the normalization logic in main.go around the result["platformType"]
handling and replace the fmt.Println call with a slog.Debug call if the value
still needs to be observable, keeping normal command output clean.
In `@cmd/coveragecli/testdata/appium-junit-report.xml`:
- Around line 16-19: The Appium JUnit fixture is using a whitespace-only failure
body, which causes normalizeAppiumJUnit to create an empty stackTrace instead of
testing a real trace. Update the testcase under the appium-junit-report.xml
fixture so the <failure> element contains an actual non-empty stack-trace
string, letting the normalizeAppiumJUnit path exercise a realistic stackTrace
value rather than an empty trimmed result.
🪄 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 Plus
Run ID: ccfaf492-6518-4dac-9367-3b2f1640ccdb
📒 Files selected for processing (3)
cmd/coveragecli/main.gocmd/coveragecli/main_test.gocmd/coveragecli/testdata/appium-junit-report.xml
| type JUnitTestCase struct { | ||
| Classname string `xml:"classname,attr,omitempty"` | ||
| Name string `xml:"name,attr"` | ||
| Time float64 `xml:"time,attr,omitempty"` | ||
| Status string `xml:"status,attr,omitempty"` | ||
| Failure *JUnitFailure `xml:"failure,omitempty"` | ||
| Skipped *JUnitSkipped `xml:"skipped,omitempty"` | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Treat JUnit <error> cases as failed and ensure failure messages are non-empty.
JUnit reports can use <error> instead of <failure>; those are currently unmarshaled away and reported as passed. Also, body-only failures produce an empty failure.message, which downstream validation rejects for failed specs.
Proposed fix
type JUnitTestCase struct {
Classname string `xml:"classname,attr,omitempty"`
Name string `xml:"name,attr"`
Time float64 `xml:"time,attr,omitempty"`
Status string `xml:"status,attr,omitempty"`
Failure *JUnitFailure `xml:"failure,omitempty"`
+ Error *JUnitFailure `xml:"error,omitempty"`
Skipped *JUnitSkipped `xml:"skipped,omitempty"`
}- if tc.Failure != nil {
+ failureNode := tc.Failure
+ if failureNode == nil {
+ failureNode = tc.Error
+ }
+ if failureNode != nil {
state = "failed"
} else if tc.Skipped != nil {- if tc.Failure != nil {
+ if failureNode != nil {
+ message := strings.TrimSpace(failureNode.Message)
+ if message == "" {
+ message = strings.TrimSpace(failureNode.Body)
+ }
failure := map[string]any{
- "message": tc.Failure.Message,
+ "message": message,
}
- if tc.Failure.Body != "" {
- failure["stackTrace"] = strings.TrimSpace(tc.Failure.Body)
+ if failureNode.Body != "" {
+ failure["stackTrace"] = strings.TrimSpace(failureNode.Body)
}
spec["failure"] = failure
}Also applies to: 850-866, 891-898
🤖 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 `@cmd/coveragecli/main.go` around lines 146 - 153, Update the JUnit parsing in
JUnitTestCase so `<error>` elements are captured and treated as failures rather
than being ignored, and make sure the resulting failed case is not reported as
passed. Adjust the JUnitFailure handling used by the test result
unmarshal/normalization logic to populate a non-empty failure message even when
the failure body contains the only details, so downstream validation always sees
a message for failed specs. Focus on the JUnitTestCase, JUnitFailure, and any
related parsing/serialization paths referenced by the existing JUnit XML
handling.
| specType := "happyPath" | ||
| classLower := strings.ToLower(tc.Classname) | ||
| switch { | ||
| case strings.Contains(classLower, "setup"): | ||
| specType = "setup" | ||
| case strings.Contains(classLower, "happyPath"): | ||
| specType = "happyPath" | ||
| case strings.Contains(classLower, "negativepath"): | ||
| specType = "negativePath" | ||
| default: | ||
| specType = "happyPath" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the ineffectual specType assignment.
Every switch branch assigns specType, so the initial value is unused and triggers ineffassign. Also compare against lowercase "happypath" since classLower is already lowercased.
Proposed fix
- specType := "happyPath"
+ var specType string
classLower := strings.ToLower(tc.Classname)
switch {
case strings.Contains(classLower, "setup"):
specType = "setup"
- case strings.Contains(classLower, "happyPath"):
+ case strings.Contains(classLower, "happypath"):
specType = "happyPath"
case strings.Contains(classLower, "negativepath"):
specType = "negativePath"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| specType := "happyPath" | |
| classLower := strings.ToLower(tc.Classname) | |
| switch { | |
| case strings.Contains(classLower, "setup"): | |
| specType = "setup" | |
| case strings.Contains(classLower, "happyPath"): | |
| specType = "happyPath" | |
| case strings.Contains(classLower, "negativepath"): | |
| specType = "negativePath" | |
| default: | |
| specType = "happyPath" | |
| var specType string | |
| classLower := strings.ToLower(tc.Classname) | |
| switch { | |
| case strings.Contains(classLower, "setup"): | |
| specType = "setup" | |
| case strings.Contains(classLower, "happypath"): | |
| specType = "happyPath" | |
| case strings.Contains(classLower, "negativepath"): | |
| specType = "negativePath" | |
| default: | |
| specType = "happyPath" |
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 869-869: ineffectual assignment to specType
(ineffassign)
🤖 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 `@cmd/coveragecli/main.go` around lines 869 - 879, The `specType`
initialization in the `switch` block is ineffectual because every branch
overwrites it, and the `strings.Contains` check in the `tc.Classname`
classification logic should use lowercase patterns to match `classLower`. Update
the `specType` selection in this `switch` so it no longer relies on an unused
initial value, and change the `"happyPath"` comparison to `"happypath"` (and
keep the other lowercase checks consistent) inside the `main` classification
code.
Source: Linters/SAST tools
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)
cmd/coveragecli/main.go (1)
848-848: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the debug stdout print.
This adds unsolicited CLI output during normalization; use structured logging if this is intentionally observable.
Proposed fix
result["platformType"] = platformType - fmt.Println("normalize: ", result["platformType"]) var specReports []map[string]any🤖 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 `@cmd/coveragecli/main.go` at line 848, Remove the debug stdout print in the normalization flow by deleting the fmt.Println call that prints result["platformType"] in the coverage CLI main logic. If this information is still needed, route it through the existing structured logger instead of writing directly to stdout so normal CLI output remains clean.
🤖 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 `@cmd/coveragecli/main.go`:
- Line 848: Remove the debug stdout print in the normalization flow by deleting
the fmt.Println call that prints result["platformType"] in the coverage CLI main
logic. If this information is still needed, route it through the existing
structured logger instead of writing directly to stdout so normal CLI output
remains clean.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 39cbdc67-9a58-4d78-a2ac-16fc8ca0edb2
📒 Files selected for processing (2)
cmd/coveragecli/main.gocmd/coveragecli/main_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/coveragecli/main_test.go
3 participants
Added normalization tool to support appium junit report
Summary by CodeRabbit