support appium junit report - #14
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 a ChangesSpecType filtering across the E2E stack
JUnit XML support in coveragecli
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant UI as e2e.js
participant Handler as handlers.go
participant UseCase as e2e_usecase.go
participant Repo as e2e_test_run_repository.go
UI->>Handler: GET /api/projects/:id/e2e-test-runs?specType=happyPath
Handler->>UseCase: ListE2ERunsInput{SpecType}
UseCase->>Repo: ListByProject(..., specType)
Repo-->>UseCase: filtered runs
UseCase-->>Handler: runs
Handler-->>UI: JSON response
Possibly related PRs
Suggested reviewers: 🚥 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/application/e2e_usecase.go (1)
298-308: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPersisted
SpecTypeis not normalized the same way it's validated.
validateE2EIngestInputtrimsspec.SpecTypebefore checking it againstvalidSpecTypes, butbuildE2EEntitiesassigns the raw, untrimmedspec.SpecTypeonto the domain entity that is later persisted. A value like"happyPath "passes validation but will violate thespec_typeCHECKconstraint added inmigrations/004_add_spec_type.sql, turning valid-looking input into an unhandled insert failure inside the transaction instead of a clean 400.As per coding guidelines, "/*.go: Sanitize and validate all external input" — the sanitized (trimmed) value should be what gets validated and stored.
🛠️ Proposed fix
specPath := spec.LeafNodeText if len(spec.ContainerHierarchyTexts) > 0 { specPath = strings.Join(append(spec.ContainerHierarchyTexts, spec.LeafNodeText), " > ") } + + specType := strings.TrimSpace(spec.SpecType) var failureMessage *string @@ specResults = append(specResults, domain.E2ESpecResult{ ID: uc.ids.NewID(), SpecPath: specPath, LeafNodeText: spec.LeafNodeText, State: normalizedState, - SpecType: spec.SpecType, + SpecType: specType, DurationMS: durationMS,Also applies to: 387-391
🤖 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 `@internal/application/e2e_usecase.go` around lines 298 - 308, The persisted SpecType is using the raw input instead of the trimmed value that validateE2EIngestInput already checks, so buildE2EEntities can store an invalid string like a trailing-space variant. Update buildE2EEntities to normalize SpecType the same way validation does and assign that sanitized value when constructing domain.E2ESpecResult, ensuring the stored value matches the CHECK constraint and the validated input. Also apply the same fix anywhere else the E2E result entity is built (including the other referenced build path) so both validation and persistence use the same normalized SpecType.Source: Coding guidelines
🧹 Nitpick comments (4)
cmd/coveragecli/main.go (1)
711-768: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate specType-derivation logic vs.
normalizeAppiumJUnit.This switch (setup/happyPath/negativePath detection from
file/projectID) is nearly identical to the one innormalizeAppiumJUnit(Lines 868-880). Consider extracting a sharedderiveSpecType(s string) stringhelper to avoid divergence (e.g. the case-sensitivity mismatch already present in the Appium variant, see below).🤖 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 711 - 768, The spec_type derivation logic here duplicates the matching in normalizeAppiumJUnit, so extract a shared deriveSpecType helper and use it from both places. Move the setup/happyPath/negativePath selection based on file and projectID into that helper, then replace the local switch in this test-processing block with the shared call to keep behavior consistent and prevent future divergence.internal/domain/e2e.go (1)
55-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a typed
SpecTypesimilar toE2ESpecState.
SpecTypeis a barestringwhile the siblingStatefield uses the typedE2ESpecState. Validation of the allowed values (happyPath/negativePath/setup) currently lives only in the application layer, so the domain struct can't enforce this invariant itself.♻️ Suggested typed enum
+type E2ESpecType string + +const ( + E2ESpecTypeSetup E2ESpecType = "setup" + E2ESpecTypeHappyPath E2ESpecType = "happyPath" + E2ESpecTypeNegativePath E2ESpecType = "negativePath" +) + type E2ESpecResult struct { ID string E2ETestRunID string SpecPath string LeafNodeText string State E2ESpecState - SpecType string + SpecType E2ESpecType DurationMS int64 FailureMessage *string FailureLocationFile *string FailureLocationLine *int }As per coding guidelines, "Domain types should enforce invariants (e.g., coverage must be between 0 and 100)."
🤖 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 `@internal/domain/e2e.go` at line 55, The `E2ESpec` struct still exposes `SpecType` as a plain string while `State` already uses the typed `E2ESpecState`; update the domain model to introduce a typed enum for `SpecType` as well, similar to `E2ESpecState`, and use that type on the `E2ESpec` field. Add the allowed values (`happyPath`, `negativePath`, `setup`) to the new type and keep validation/domain invariants inside `E2ESpec` rather than relying only on application-layer checks.Source: Coding guidelines
internal/application/mock_application.go (1)
76-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCapture
specTypein the stub for test assertions.
capturedBranch/capturedStatusare recorded butspecTypeisn't, so tests can't verify the use case forwards the correct (validated/trimmed)specTypevalue into the repository call.♻️ Suggested addition
func (s *stubE2ETestRunRepository) ListByProject(ctx context.Context, projectID string, branch string, status string, environment string, specType string, from *time.Time, to *time.Time, page int, pageSize int) ([]domain.E2ETestRun, int, error) { s.capturedBranch = branch s.capturedStatus = status + s.capturedSpecType = specType if s.listErr != nil { return nil, 0, s.listErr } return s.listed, s.listTotal, nil } func (s *stubE2ETestRunRepository) HeatmapData(ctx context.Context, branch string, status string, specType string, runsPerProject int) ([]TestHeatmapRow, error) { s.capturedBranch = branch s.capturedStatus = status + s.capturedSpecType = specType if s.heatmapErr != nil { return nil, s.heatmapErr } return s.heatmapRows, nil }(add a
capturedSpecType stringfield to the struct)Also applies to: 85-92
🤖 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 `@internal/application/mock_application.go` around lines 76 - 83, The stub repository method ListByProject currently records branch and status but drops specType, so add a capturedSpecType field to stubE2ETestRunRepository and assign it inside ListByProject alongside the other captured inputs. This lets tests assert that the use case forwards the validated/trimmed specType value correctly; update the relevant stub usage in mock_application.go consistently wherever ListByProject is implemented.migrations/004_add_spec_type.sql (1)
3-7: 🚀 Performance & Scalability | 🔵 TrivialConsider lock impact for large tables.
Adding a
CHECKconstraint viaALTER TABLEvalidates all existing rows under anACCESS EXCLUSIVElock, andCREATE INDEX(non-CONCURRENTLY) blocks writes during the build. For a table that could grow large in production, considerADD CONSTRAINT ... NOT VALID+ a separateVALIDATE CONSTRAINT, andCREATE INDEX CONCURRENTLY(which requires-- +goose NO TRANSACTIONsince it can't run inside goose's default transaction).🤖 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 `@migrations/004_add_spec_type.sql` around lines 3 - 7, The migration in ALTER TABLE e2e_test_spec_results currently takes an ACCESS EXCLUSIVE lock to validate the new CHECK constraint and the plain CREATE INDEX will block writes on large tables. Update the migration to add the constraint as NOT VALID first, validate it in a separate step, and switch the index creation in e2e_test_spec_results_spec_type_idx to CREATE INDEX CONCURRENTLY; if needed for goose, mark the migration with NO TRANSACTION.
🤖 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 538-566: The Playwright JUnit XML path is returning nil from
normalizePlaywrightJUnit, which lets normalizedReport stay empty and continue
into upload. Update normalizePlaywrightJUnit to fail fast instead of silently
returning nil for unsupported or unimplemented Playwright JUnit input, and make
the caller in main() treat that path as an error so TestReport is never built
from a nil normalization result.
- Around line 891-899: The failure mapping in the coverage CLI can emit an empty
stackTrace because tc.Failure.Body is checked before trimming while the stored
value uses strings.TrimSpace. Update the tc.Failure handling in main.go so the
presence check matches the trimmed value, and only set failure["stackTrace"]
when the trimmed body is non-empty. Use the existing tc.Failure, failure map,
and spec["failure"] logic to locate the fix.
- Around line 868-880: The `specType` selection in `main.go` has a
case-sensitivity bug: `classLower` is already lowercased, so the
`strings.Contains(..., "happyPath")` check in the specType switch can never
match, and the initial `specType := "happyPath"` assignment is effectively
redundant. Update the `switch` in the spec type निर्धination block to compare
against lowercase literals consistently (for example, the `happyPath` branch
should use a lowercase keyword) and keep the fallback behavior aligned with the
`default` case so the `specType` assignment in that block is only made when
needed.
- Around line 813-818: The suitePath assignment in the JUnit parsing flow is
assuming data.TestSuites[0] and its first TestCases entry always exist, which
can panic on empty or malformed XML. Update the logic around the result-building
code to validate that data.TestSuites and the nested TestCases slice contain
elements before indexing, and return a proper error from the parser instead of
panicking. Use the existing parsing/result construction path in the coverage
CLI’s main flow to surface a clean failure when no testsuite or testcase is
present.
- Line 834: Remove the stray debug stdout print in the CLI path by deleting the
fmt.Println call in the normalization logic that references
result["platformType"]. Keep output flowing only through the existing slog-based
logging/structured CLI output so the command remains pipe-friendly and free of
leftover debug artifacts.
- Around line 798-803: The normalizePlaywrightJUnit stub is currently discarding
the error from fmt.Errorf and returning nil, which makes the caller proceed with
an empty report instead of failing. Update normalizePlaywrightJUnit to return an
error (or otherwise surface the failure) and have the caller path that builds
the TestReport handle it using the same exitErr pattern used elsewhere in
main.go. Use the normalizePlaywrightJUnit function and the TestReport
construction flow to locate the fix.
In `@cmd/frontend/web/e2eTest.html`:
- Around line 109-113: Add the missing setup option to the spec type filter
dropdowns so the UI matches the backend allow-list. Update both
`#e2eSpecTypeFilter` and `#heatmapSpecTypeFilter` in e2eTest.html to include setup
alongside happyPath and negativePath, keeping the option values consistent with
validSpecTypes and the spec_type constraint.
---
Outside diff comments:
In `@internal/application/e2e_usecase.go`:
- Around line 298-308: The persisted SpecType is using the raw input instead of
the trimmed value that validateE2EIngestInput already checks, so
buildE2EEntities can store an invalid string like a trailing-space variant.
Update buildE2EEntities to normalize SpecType the same way validation does and
assign that sanitized value when constructing domain.E2ESpecResult, ensuring the
stored value matches the CHECK constraint and the validated input. Also apply
the same fix anywhere else the E2E result entity is built (including the other
referenced build path) so both validation and persistence use the same
normalized SpecType.
---
Nitpick comments:
In `@cmd/coveragecli/main.go`:
- Around line 711-768: The spec_type derivation logic here duplicates the
matching in normalizeAppiumJUnit, so extract a shared deriveSpecType helper and
use it from both places. Move the setup/happyPath/negativePath selection based
on file and projectID into that helper, then replace the local switch in this
test-processing block with the shared call to keep behavior consistent and
prevent future divergence.
In `@internal/application/mock_application.go`:
- Around line 76-83: The stub repository method ListByProject currently records
branch and status but drops specType, so add a capturedSpecType field to
stubE2ETestRunRepository and assign it inside ListByProject alongside the other
captured inputs. This lets tests assert that the use case forwards the
validated/trimmed specType value correctly; update the relevant stub usage in
mock_application.go consistently wherever ListByProject is implemented.
In `@internal/domain/e2e.go`:
- Line 55: The `E2ESpec` struct still exposes `SpecType` as a plain string while
`State` already uses the typed `E2ESpecState`; update the domain model to
introduce a typed enum for `SpecType` as well, similar to `E2ESpecState`, and
use that type on the `E2ESpec` field. Add the allowed values (`happyPath`,
`negativePath`, `setup`) to the new type and keep validation/domain invariants
inside `E2ESpec` rather than relying only on application-layer checks.
In `@migrations/004_add_spec_type.sql`:
- Around line 3-7: The migration in ALTER TABLE e2e_test_spec_results currently
takes an ACCESS EXCLUSIVE lock to validate the new CHECK constraint and the
plain CREATE INDEX will block writes on large tables. Update the migration to
add the constraint as NOT VALID first, validate it in a separate step, and
switch the index creation in e2e_test_spec_results_spec_type_idx to CREATE INDEX
CONCURRENTLY; if needed for goose, mark the migration with NO TRANSACTION.
🪄 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: 767364d9-6fe1-42a4-b5c4-58ab82e2dca1
📒 Files selected for processing (15)
cmd/coveragecli/main.gocmd/coveragecli/main_test.gocmd/coveragecli/testdata/appium-junit-report.xmlcmd/frontend/web/assets/e2e.jscmd/frontend/web/e2eTest.htmlinternal/adapters/http/handlers.gointernal/adapters/postgres/e2e_spec_result_repository.gointernal/adapters/postgres/e2e_test_run_repository.gointernal/application/e2e_usecase.gointernal/application/e2e_usecase_test.gointernal/application/integration_usecase.gointernal/application/mock_application.gointernal/application/ports.gointernal/domain/e2e.gomigrations/004_add_spec_type.sql
| // Normalize report structure based on report type and file format | ||
| var normalizedReport map[string]any | ||
| if isXML { | ||
| var junitData JUnitTestSuites | ||
| if err := xml.Unmarshal(rawReport, &junitData); err != nil { | ||
| exitErr("parse e2e report xml", err) | ||
| } | ||
| switch *reportType { | ||
| case "playwright": | ||
| normalizedReport = normalizePlaywrightJUnit(junitData) | ||
| case "appium": | ||
| normalizedReport = normalizeAppiumJUnit(junitData) | ||
| default: | ||
| exitErr("validate input", fmt.Errorf("unsupported report type: %s", *reportType)) | ||
| } | ||
| } else { | ||
| var report map[string]any | ||
| if err := json.Unmarshal(rawReport, &report); err != nil { | ||
| exitErr("parse e2e report json", err) | ||
| } | ||
| switch *reportType { | ||
| case "playwright": | ||
| normalizedReport = normalizePlaywrightReport(report) | ||
| case "appium": | ||
| exitErr("validate input", fmt.Errorf("appium JSON report format is not yet supported; use JUnit XML (.xml)")) | ||
| default: | ||
| exitErr("validate input", fmt.Errorf("unsupported report type: %s", *reportType)) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Unimplemented Playwright JUnit path silently produces a nil report.
When reportType == "playwright" and the input is .xml, normalizePlaywrightJUnit (see Lines 798-803) returns nil without stopping execution. normalizedReport stays nil and the flow continues to build and upload a payload with TestReport: nil instead of failing fast. Root cause is in normalizePlaywrightJUnit; see comment there.
🤖 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 538 - 566, The Playwright JUnit XML
path is returning nil from normalizePlaywrightJUnit, which lets normalizedReport
stay empty and continue into upload. Update normalizePlaywrightJUnit to fail
fast instead of silently returning nil for unsupported or unimplemented
Playwright JUnit input, and make the caller in main() treat that path as an
error so TestReport is never built from a nil normalization result.
Source: Linters/SAST tools
| // normalizePlaywrightJUnit converts a Playwright JUnit XML report into the normalized map[string]any structure. | ||
| // Playwright JUnit uses classname format: "file › Suite Title › Nested Suite" | ||
| func normalizePlaywrightJUnit(data JUnitTestSuites) map[string]any { | ||
| fmt.Errorf("Playwright JUnit XML normalization is not yet implemented") | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Stub discards its error and silently returns nil.
fmt.Errorf(...) builds an error that is never used or returned, so normalizePlaywrightJUnit looks like a no-op that returns a valid (but empty) report. Combined with the caller not checking the return value (Line 547), this results in an upload with a nil TestReport instead of a hard failure. Given other error paths in this file use exitErr, apply the same pattern here.
🐛 Proposed fix
func normalizePlaywrightJUnit(data JUnitTestSuites) map[string]any {
- fmt.Errorf("Playwright JUnit XML normalization is not yet implemented")
- return nil
+ exitErr("normalize e2e report", fmt.Errorf("Playwright JUnit XML normalization is not yet implemented"))
+ return nil
}📝 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.
| // normalizePlaywrightJUnit converts a Playwright JUnit XML report into the normalized map[string]any structure. | |
| // Playwright JUnit uses classname format: "file › Suite Title › Nested Suite" | |
| func normalizePlaywrightJUnit(data JUnitTestSuites) map[string]any { | |
| fmt.Errorf("Playwright JUnit XML normalization is not yet implemented") | |
| return nil | |
| } | |
| // normalizePlaywrightJUnit converts a Playwright JUnit XML report into the normalized map[string]any structure. | |
| // Playwright JUnit uses classname format: "file › Suite Title › Nested Suite" | |
| func normalizePlaywrightJUnit(data JUnitTestSuites) map[string]any { | |
| exitErr("normalize e2e report", fmt.Errorf("Playwright JUnit XML normalization is not yet implemented")) | |
| return nil | |
| } |
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 801-801: Error return value of fmt.Errorf is not checked
(errcheck)
🤖 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 798 - 803, The normalizePlaywrightJUnit
stub is currently discarding the error from fmt.Errorf and returning nil, which
makes the caller proceed with an empty report instead of failing. Update
normalizePlaywrightJUnit to return an error (or otherwise surface the failure)
and have the caller path that builds the TestReport handle it using the same
exitErr pattern used elsewhere in main.go. Use the normalizePlaywrightJUnit
function and the TestReport construction flow to locate the fix.
Source: Linters/SAST tools
| // Use top-level testsuites name as suiteDescription | ||
| if data.Name != "" { | ||
| result["suiteDescription"] = data.Name | ||
| } | ||
| result["suitePath"] = data.TestSuites[0].TestCases[0].Classname | ||
| result["frameworkVersion"] = "" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unchecked index access can panic on malformed/empty XML.
data.TestSuites[0].TestCases[0].Classname assumes at least one <testsuite> and one <testcase> exist. A JUnit XML report with an empty <testsuites> (e.g. all specs skipped, or a malformed upload) will cause an index-out-of-range panic instead of a clean, reported error.
As per coding guidelines, "Sanitize and validate all external input" and "Return errors instead of panicking in normal control flow" for **/*.go.
🛡️ Proposed fix
- result["suitePath"] = data.TestSuites[0].TestCases[0].Classname
+ if len(data.TestSuites) > 0 && len(data.TestSuites[0].TestCases) > 0 {
+ result["suitePath"] = data.TestSuites[0].TestCases[0].Classname
+ }📝 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.
| // Use top-level testsuites name as suiteDescription | |
| if data.Name != "" { | |
| result["suiteDescription"] = data.Name | |
| } | |
| result["suitePath"] = data.TestSuites[0].TestCases[0].Classname | |
| result["frameworkVersion"] = "" | |
| // Use top-level testsuites name as suiteDescription | |
| if data.Name != "" { | |
| result["suiteDescription"] = data.Name | |
| } | |
| if len(data.TestSuites) > 0 && len(data.TestSuites[0].TestCases) > 0 { | |
| result["suitePath"] = data.TestSuites[0].TestCases[0].Classname | |
| } | |
| result["frameworkVersion"] = "" |
🤖 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 813 - 818, The suitePath assignment in
the JUnit parsing flow is assuming data.TestSuites[0] and its first TestCases
entry always exist, which can panic on empty or malformed XML. Update the logic
around the result-building code to validate that data.TestSuites and the nested
TestCases slice contain elements before indexing, and return a proper error from
the parser instead of panicking. Use the existing parsing/result construction
path in the coverage CLI’s main flow to surface a clean failure when no
testsuite or testcase is present.
Source: Coding guidelines
| } | ||
| } | ||
| result["platformType"] = platformType | ||
| fmt.Println("normalize: ", result["platformType"]) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove stray debug print.
fmt.Println("normalize: ", ...) writes directly to stdout in a CLI tool that also prints structured logs via slog; this pollutes output/piping and looks like a leftover debug artifact.
🧹 Proposed fix
- result["platformType"] = platformType
- fmt.Println("normalize: ", result["platformType"])
+ result["platformType"] = platformType📝 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.
| 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 stray debug stdout print in
the CLI path by deleting the fmt.Println call in the normalization logic that
references result["platformType"]. Keep output flowing only through the existing
slog-based logging/structured CLI output so the command remains pipe-friendly
and free of leftover debug artifacts.
| // Determine specType from classname keywords | ||
| 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.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Case-sensitivity bug makes the happyPath branch unreachable.
classLower is lowercased, but compared against the mixed-case literal "happyPath" (Line 874), so that branch can never match — it happens to coincide with the default case today, but this is fragile and matches the static-analysis "ineffectual assignment" hint on the initial specType := "happyPath" (Line 869), since every branch (including default) always reassigns.
🐛 Proposed fix
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"
default:
specType = "happyPath"
}📝 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.
| // Determine specType from classname keywords | |
| 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" | |
| } | |
| // Determine specType from classname keywords | |
| 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" | |
| } |
🧰 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 868 - 880, The `specType` selection in
`main.go` has a case-sensitivity bug: `classLower` is already lowercased, so the
`strings.Contains(..., "happyPath")` check in the specType switch can never
match, and the initial `specType := "happyPath"` assignment is effectively
redundant. Update the `switch` in the spec type निर्धination block to compare
against lowercase literals consistently (for example, the `happyPath` branch
should use a lowercase keyword) and keep the fallback behavior aligned with the
`default` case so the `specType` assignment in that block is only made when
needed.
Source: Linters/SAST tools
| if tc.Failure != nil { | ||
| failure := map[string]any{ | ||
| "message": tc.Failure.Message, | ||
| } | ||
| if tc.Failure.Body != "" { | ||
| failure["stackTrace"] = strings.TrimSpace(tc.Failure.Body) | ||
| } | ||
| spec["failure"] = failure | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
stackTrace can be set to an empty string.
The presence check uses the untrimmed tc.Failure.Body != "", but the stored value is strings.TrimSpace(tc.Failure.Body). A whitespace-only body (as in the provided fixture) passes the check yet yields an empty stackTrace value.
🐛 Proposed fix
- if tc.Failure.Body != "" {
- failure["stackTrace"] = strings.TrimSpace(tc.Failure.Body)
- }
+ if trimmed := strings.TrimSpace(tc.Failure.Body); trimmed != "" {
+ failure["stackTrace"] = trimmed
+ }📝 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.
| if tc.Failure != nil { | |
| failure := map[string]any{ | |
| "message": tc.Failure.Message, | |
| } | |
| if tc.Failure.Body != "" { | |
| failure["stackTrace"] = strings.TrimSpace(tc.Failure.Body) | |
| } | |
| spec["failure"] = failure | |
| } | |
| if tc.Failure != nil { | |
| failure := map[string]any{ | |
| "message": tc.Failure.Message, | |
| } | |
| if trimmed := strings.TrimSpace(tc.Failure.Body); trimmed != "" { | |
| failure["stackTrace"] = trimmed | |
| } | |
| spec["failure"] = failure | |
| } |
🤖 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 891 - 899, The failure mapping in the
coverage CLI can emit an empty stackTrace because tc.Failure.Body is checked
before trimming while the stored value uses strings.TrimSpace. Update the
tc.Failure handling in main.go so the presence check matches the trimmed value,
and only set failure["stackTrace"] when the trimmed body is non-empty. Use the
existing tc.Failure, failure map, and spec["failure"] logic to locate the fix.
* added e2e heatmap and coverage dashboard * removed unused imports * added unit tests * fixed typo errors * replaced the dummy playwright report with minimal fields for test * replaced logs with slog and added check in e2e migration file * removed duplicate table drop query * moved check constraint from the table * added spec type to e2e tests report * added spec type filter in the dashboard and heatmap * updated the success ratio * refactor to save the spec type * updated failed spec query * resolved pr comment - refactored spec type normalization and frontend --------- Co-authored-by: rsultana1418 <rsultana@lodgelink.com>
2 participants
Added normalization tool to support appium junit report
Summary by CodeRabbit
New Features
Bug Fixes