Re-enable binary tests #332
Conversation
WalkthroughThis change removes the Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
💤 Files with no reviewable changes (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
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 |
|
Please close&re-open once konveyor/ci#124 gets merged (with fix passing maven search skip into tackle CR). |
|
Just a note, from my point of view is better to merge this PR with 2 binary analysis working and open another one when got the last failing one fixed. |
mguetta1
force-pushed
the
binaries
branch
2 times, most recently
from
fda1bd1 to
0ff8bd6
Compare
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
analysis/tc_acmeair_webapp_upload_binary.go (1)
21-424: Remove remaining SkipTest/SkipTestConfig usages in analysis before mergingrg output shows live references — remove or update these so binary tests are re-enabled:
- analysis/pkg.go:40 — type SkipTestConfig
- analysis/pkg.go:69 — SkipTest SkipTestConfig
- analysis/analysis_test.go:71-72 — testcase.SkipTest.Skip / t.Skipf(...)
🧹 Nitpick comments (6)
analysis/tc_administracion_efectivo_upload_binary.go (2)
22-71: Insights block commented out — avoid drift or update tests accordingly.If tests assert presence/contents of Insights/Incidents, they’ll now fail. Either:
- restore a minimal Insights set used by tests, or
- remove test expectations and fully drop the block (not just comment it).
929-932: Platform-specific artifact may cause CI variance.Including native-linux-x86_64 ties results to x86_64 Linux. If CI runs on other arches/OS, consider dropping or gating this entry.
analysis/tc_acmeair_webapp_upload_binary.go (4)
31-181: Large set of Incidents commented out — align with test expectations.If tests depend on the previously enumerated Incidents, this change will break them. Decide: keep a minimal representative set or update tests to assert fewer details.
182-421: Use precise package names in messages (javax.ws.rs → jakarta.ws.rs).Messages currently say “javax.ws”/“jakarta.ws”; please change to “javax.ws.rs”/“jakarta.ws.rs” for accuracy across these incidents.
- Message: "Replace the `javax.ws` import statement with `jakarta.ws`", + Message: "Replace `javax.ws.rs` imports with `jakarta.ws.rs`",
424-510: Determinism: prefer known versions over “Unknown”.Where possible, fill real versions to avoid flaky assertions when upstream detection changes.
1-541: Operational sequencing with CI.Align merge timing with konveyor/ci#124 (as suggested in PR comments) to ensure Maven search skip is propagated; otherwise tests may still fail post-merge.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
analysis/tc_acmeair_webapp_upload_binary.go(3 hunks)analysis/tc_administracion_efectivo_upload_binary.go(1 hunks)analysis/tc_tackle_testapp_public_binary.go(0 hunks)
💤 Files with no reviewable changes (1)
- analysis/tc_tackle_testapp_public_binary.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test-tier1 / e2e-api-integration-tests
- GitHub Check: test-tier2 / e2e-api-integration-tests
- GitHub Check: test-tier0 / e2e-api-integration-tests
🔇 Additional comments (3)
analysis/tc_administracion_efectivo_upload_binary.go (2)
21-21: Effort change OK — no dependent tests found.
Repo search for "administracion-efectivo" and Effort values returned only analysis/tc_administracion_efectivo_upload_binary.go:21 (Effort: 30); no tests/assertions reference the previous value.
324-327: Do not rename — keep io.quarkus:quarkus-apache-httpclient.io.quarkus:quarkus-apache-httpclient is the official Quarkus Apache HttpClient extension for Quarkus 3; there is no official io.quarkus:quarkus-apache-httpclient5 artifact. (quarkus.io)
Likely an incorrect or invalid review comment.
analysis/tc_acmeair_webapp_upload_binary.go (1)
22-22: Effort reduced to 49 — repository search shows no test assertions to update.
analysis/tc_acmeair_webapp_upload_binary.go:22 (Effort: 49)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
analysis/tc_administracion_efectivo_upload_binary.go (2)
22-71: Commented‑out Insights: confirm test intent.Removing all Insights from the expected Analysis will change assertions materially. If this is to unflake binaries for now, consider deleting the block entirely (not leaving it commented) or adding a brief comment explaining why it’s intentionally empty.
74-327: Canonicalize dependency coordinates & populate missing versions — analysis/tc_administracion_efectivo_upload_binary.go (lines 74–327)Multiple entries use filename‑style Names or embed versions in Name while Version is ""/"Unknown", which makes scanner output brittle.
- Evidence: ripgrep inventory found 28 occurrences; examples: org.eclipse.jdt.jdtcore-3.1.0 (74–76), .j2ee.jar (189–191), org.springframework.spring-context-3.0.5.RELEASE (94–96), .aopalliance-1.0.jar (289–291).
- Action: replace filename/embedded-version Names with canonical group:artifact and put the exact version in Version for determinism.
-Name: "org.springframework.spring-context-3.0.5.RELEASE", Version: "Unknown" +Name: "org.springframework:spring-context", Version: "3.0.5.RELEASE" -Name: ".aopalliance-1.0.jar", Version: "" +Name: "aopalliance:aopalliance", Version: "1.0"analysis/tc_acmeair_webapp_upload_binary.go (2)
31-171: Large block of commented incidents — prefer removal or a short rationale.If these aren’t part of the expected set anymore, delete rather than comment to reduce drift. If you plan to re‑enable soon, add a one‑liner why they’re disabled.
515-537: AnalysisTags commented out — double‑check downstream expectations.Commenting out “JPA named queries” (and Properties) across categories may change tag assertions. Confirm intended, or delete comments to avoid noise.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
analysis/tc_acmeair_webapp_upload_binary.go(3 hunks)analysis/tc_administracion_efectivo_upload_binary.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-21T09:15:20.732Z
Learnt from: mguetta1
PR: konveyor/go-konveyor-tests#332
File: analysis/tc_administracion_efectivo_upload_binary.go:304-307
Timestamp: 2025-09-21T09:15:20.732Z
Learning: The dependency name "io.quarkus.http.quarkus-http-http-core" is correct and legitimate, not a typo. When reviewing Quarkus dependencies, this specific naming convention should not be flagged as an error.
Applied to files:
analysis/tc_administracion_efectivo_upload_binary.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test-tier2 / e2e-api-integration-tests
- GitHub Check: test-tier1 / e2e-api-integration-tests
- GitHub Check: test-tier0 / e2e-api-integration-tests
🔇 Additional comments (3)
analysis/tc_administracion_efectivo_upload_binary.go (1)
21-21: Effort tweak looks fine.Lowering effort from 36→30 aligns with re‑enabling scope. No functional risk.
analysis/tc_acmeair_webapp_upload_binary.go (2)
22-22: Effort reduction acknowledged.49 is reasonable given the trimmed incidents.
426-510: Filename‑style Names, empty/Unknown versions in dependencies.Same concern as other binary: non‑canonical Names (e.g., “.aspectjweaver-1.6.8.jar”, “.asm-3.3.1.jar”), and “Unknown”/empty Version may cause brittle assertions unless you’re mirroring scanner output exactly. If not intentional, normalize to group:artifact + concrete version.
Example fixes:
-Name: ".aspectjweaver-1.6.8.jar", Version: "" +Name: "org.aspectj:aspectjweaver", Version: "1.6.8" -Name: "net.sf.cglib-2.2.2", Version: "Unknown" +Name: "cglib:cglib", Version: "2.2.2"Verification helper:
#!/bin/bash rg -nP -C1 'Name:\s*"\.|Name:\s*".*\.jar"|Version:\s*""|Version:\s*"Unknown"|spring-.*3\.1\.2' analysis/tc_acmeair_webapp_upload_binary.go
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
analysis/tc_administracion_efectivo_upload_binary.go (1)
22-71: Large commented‑out Insights block — prefer delete or feature‑flag.Keeping ~50 lines of commented JSON-like data lowers signal. Either remove it or guard behind a test flag (e.g., IncludeInsights) so diffs stay clean.
analysis/tc_acmeair_webapp_upload_binary.go (1)
31-371: Commented‑out Incidents — streamline or gate.Either remove the commented incidents or reintroduce them under a toggle (e.g., IncludeIncidents) to keep the fixture concise while preserving intent.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
analysis/tc_acmeair_webapp_upload_binary.go(3 hunks)analysis/tc_administracion_efectivo_upload_binary.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-21T09:15:20.732Z
Learnt from: mguetta1
PR: konveyor/go-konveyor-tests#332
File: analysis/tc_administracion_efectivo_upload_binary.go:304-307
Timestamp: 2025-09-21T09:15:20.732Z
Learning: The dependency name "io.quarkus.http.quarkus-http-http-core" is correct and legitimate, not a typo. When reviewing Quarkus dependencies, this specific naming convention should not be flagged as an error.
Applied to files:
analysis/tc_administracion_efectivo_upload_binary.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test-tier1 / e2e-api-integration-tests
- GitHub Check: test-tier2 / e2e-api-integration-tests
- GitHub Check: test-tier0 / e2e-api-integration-tests
🔇 Additional comments (6)
analysis/tc_acmeair_webapp_upload_binary.go (3)
426-510: Jakarta target vs legacy Spring 3.1.2 artifacts — reconcile story.You tag this TC for jakarta‑ee, yet dependencies list Spring 3.1.2 modules. If this reflects the real binary, fine; otherwise upgrade entries (Spring 6+), or drop the jakarta label to avoid mixed signals.
#!/bin/bash rg -n 'spring-.*3\.1\.2|javax\.' analysis/tc_acmeair_webapp_upload_binary.go -C1 || true
426-510: Non‑canonical dependency names and empty/Unknown versions — prefer canonical coords for stability.Jar‑style names (e.g., “.aspectjweaver-1.6.8.jar”, “.asm-3.3.1.jar”) and missing versions reduce determinism. If these mirror analyzer output, keep; otherwise normalize.
Example normalizations:
- { Name: ".aspectjweaver-1.6.8.jar", Version: "", Provider: "java" }, + { Name: "org.aspectj.aspectjweaver", Version: "1.6.8", Provider: "java" }, - { Name: ".asm-3.3.1.jar", Version: "", Provider: "java" }, + { Name: "org.ow2.asm.asm", Version: "3.3.1", Provider: "java" }, - { Name: ".aopalliance-1.0.jar", Version: "", Provider: "java" }, + { Name: "aopalliance.aopalliance", Version: "1.0", Provider: "java" }, - { Name: "net.sf.cglib-2.2.2", Version: "Unknown", Provider: "java" }, + { Name: "cglib.cglib", Version: "2.2.2", Provider: "java" },Verification helper:
#!/bin/bash rg -nP -C1 'Name:\s*"\.|Name:\s*".*\.jar"|Version:\s*""|Version:\s*"Unknown"' analysis/tc_acmeair_webapp_upload_binary.go
22-22: Effort set to 49 — verify golden data and tests (analysis/tc_acmeair_webapp_upload_binary.go:22)Effort changed to 49; re-enable tests only after verifying/updating golden fixtures/validation and running the validation tests. Repo search returned no other matches for this symbol — manual verification required.
analysis/tc_administracion_efectivo_upload_binary.go (3)
72-328: Normalize dependency entries and pin missing versionsanalysis/tc_administracion_efectivo_upload_binary.go contains multiple non‑canonical names (leading '.' or '.jar') and empty/"Unknown" versions — prefer canonical group:artifact coordinates and exact versions unless these are analyzer-produced artifacts.
Examples:
- { Name: ".javax.persistence-2.0.0.jar", Version: "", Provider: "java" }, + { Name: "javax.persistence", Version: "2.0.0", Provider: "java" }, - { Name: ".jcommon-1.0.15.jar", Version: "", Provider: "java" }, + { Name: "jfree.jcommon", Version: "1.0.15", Provider: "java" },
21-21: Effort reduced to 30 — no stale assertions found.
analysis/tc_administracion_efectivo_upload_binary.go contains Effort: 30; analysis/analysis_test.go asserts gotAnalysis.Effort == tc.Analysis.Effort (so it uses the TC value); no other literal "Effort: 30" or stale assertions were found.
330-374: Duplicated/overlapping AnalysisTags — dedupe here or confirm consumers de‑dup.Found duplicates in analysis/tc_administracion_efectivo_upload_binary.go (lines 330–374): EJB XML, Servlet, Mail, JPA named queries, JPA XML, JPA entities, Persistence units, Spring Scheduled, Java EE XML, JSF XML, Properties, JSF Page, JSF, Common Annotations — each appears 3×. If downstream expects a set, no change needed; otherwise remove duplicates here or ensure consumers explicitly de‑duplicate to avoid skewed counts.
mguetta1
added
cherry-pick/release-0.8
aufi
left a comment
aufi
left a comment
There was a problem hiding this comment.
IIUC, only currently working is tc_tackle_testapp_piblic_binary (good to merge this app test) however I'm not sure about changes in expected test output for acmeair and administracion-efectivo. Do we know if the commented/updated output is correct?
|
+1 to what @aufi mentioned - some dependency output may be different now after the recent changes in the analyzer. |
|
The commented incidents are expected, this is why I didn't remove them. |
|
@mguetta1 apologies for the late review. Just for my own knowledge, how come all incidents in the first two apps are commented out? |
Hi @eemcmullan, the incidents were commented due to the following regressions: |
Signed-off-by: Maayan Hadasi <[email protected]>
4 participants
Resolves: #329
Relates to: konveyor/analyzer-lsp#905
Relates to: konveyor/analyzer-lsp#906
Summary by CodeRabbit
Tests
Chores