chore(bench): add javac compilation step to Java fixture for javacg-static comparison#1332
Conversation
|
Claude finished @carlos-alm's task —— View job PR Review: chore(bench): add javac compilation step to Java fixture for javacg-static comparison
Root Cause AnalysisProblem Being Solved: The Java fixture in Why This Approach: Consistent with the pattern established for JS/TS comparisons (ACG, Jelly) — a standalone comparison script, not integrated into CI or the test suite. Risk Assessment: Low risk. No production code changed, no test assertions modified, no linting rules relaxed. Purely additive. Phase 2: Configuration ChangesNone. No Phase 3: Test IntegrityNo test files modified. The new files are a standalone script and build tooling — not wired into Phase 4: Code Quality
Significant concern — constructor target name mapping inconsistency:
// Line 81-85: target is bare ClassName (class instantiation)
{ "name": "InMemoryUserRepository", "file": "InMemoryUserRepository.java" }
// Line 87-92: target is bare ClassName again
{ "name": "UserService", "file": "UserService.java" }
// Line 22-26: target is also bare ClassName
{ "name": "Validator", "file": "Validator.java" }This matches However, // expected-edges.json line 20-26
{ "name": "UserService.UserService", ... }That matches the ground truth convention ✓. Potential false negative in if (sourceName === targetName && sourceFile === targetFile) continue;This filters edges where the resolved name AND file match. But
Makefile concern ( javac -d . $(SRCS)
jar cf $(JAR) benchmark/
Documentation placeholder in | javacg-static (CHA) | — | — | — | — | — |The table row is intentionally left empty pending a run. This is clearly communicated and consistent with how the JS table was initially populated. ✓ Phase 5: Backlog ComplianceZero-dep: ✓ — No new Foundation-aligned: ✓
Problem-fit: 2/5 — This is benchmark instrumentation infrastructure. It helps quantify codegraph's Java call resolution recall gaps vs javacg-static, which indirectly surfaces actionable recall improvements (noted in the docs: Breaking: No — purely additive. No CLI output changes, no API changes, no DB schema changes. Tier: 1 (zero-dep ✓ + foundation-aligned ✓) Critical ConcernsNone blocking. One minor issue worth considering before merge:
Final Assessment |
Codegraph Impact Analysis9 functions changed → 2 callers affected across 1 files
|
Greptile SummaryThis PR completes the javacg-static comparison infrastructure for the Java call-graph benchmark: a
Confidence Score: 5/5Safe to merge — adds a self-contained benchmark tooling script and Makefile with no impact on production code paths. All changes are additive benchmark infrastructure (a build script, a comparison script, a .gitignore, and documentation). The logic in compare-javacg.mjs is correct: the edge-key format matches between loadGroundTruth and parseJavacgOutput, the JVM name mapping handles constructors properly, the CLI validation covers the --jar-without-path case, and the clean-build rm -rf benchmark/ in the Makefile prevents stale artefacts. No production code is touched. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Makefile
participant javac
participant jar
participant compare as compare-javacg.mjs
participant javacg as javacg-static JAR
participant fs as expected-edges.json
User->>Makefile: make
Makefile->>Makefile: rm -rf benchmark/
Makefile->>javac: "javac -d . *.java"
javac-->>Makefile: "benchmark/*.class"
Makefile->>jar: jar cf fixture.jar benchmark/
jar-->>Makefile: fixture.jar
User->>compare: node compare-javacg.mjs --jar javacg.jar
compare->>compare: findJavacgJar()
compare->>compare: buildClassFileMap(fixtureDir)
compare->>fs: loadGroundTruth()
fs-->>compare: Set of edge keys (17 edges)
compare->>javacg: java -jar javacg.jar fixture.jar
javacg-->>compare: M:class:method (T)class:method lines
compare->>compare: parseJavacgOutput() → predicted edge Set
compare->>compare: computeMetrics(predicted, groundTruth)
compare-->>User: precision/recall table (or --json)
Reviews (8): Last reviewed commit: "Merge remote-tracking branch 'origin/cho..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| $(JAR): $(SRCS) | ||
| javac -d . $(SRCS) | ||
| jar cf $(JAR) benchmark/ |
There was a problem hiding this comment.
.class files from deleted Java sources. If a fixture .java file is removed and then make is run (without make clean), javac -d . *.java updates only the files it knows about, but the old .class file stays in benchmark/ and gets bundled into the JAR. Deleting the output directory before each compile guarantees a clean artefact.
| $(JAR): $(SRCS) | |
| javac -d . $(SRCS) | |
| jar cf $(JAR) benchmark/ | |
| $(JAR): $(SRCS) | |
| rm -rf benchmark/ | |
| javac -d . $(SRCS) | |
| jar cf $(JAR) benchmark/ |
There was a problem hiding this comment.
Fixed — added rm -rf benchmark/ before javac so each rebuild starts from a clean slate. Stale .class files from deleted sources can no longer slip into the JAR.
| const args = process.argv.slice(2); | ||
| const jsonFlag = args.includes('--json'); | ||
| const jarArgIdx = args.indexOf('--jar'); | ||
| const jarArgPath = jarArgIdx !== -1 ? args[jarArgIdx + 1] : null; |
There was a problem hiding this comment.
--jar is the last token on the command line (no path follows), args[jarArgIdx + 1] evaluates to undefined. Since undefined is falsy, findJavacgJar silently falls through to the env-var and scripts/lib/ lookups instead of surfacing the malformed invocation. The user ends up seeing "javacg-static JAR not found" with no indication that --jar was recognised but incomplete.
| const jarArgPath = jarArgIdx !== -1 ? args[jarArgIdx + 1] : null; | |
| const jarArgPath = jarArgIdx !== -1 ? (args[jarArgIdx + 1] ?? null) : null; | |
| if (jarArgIdx !== -1 && !jarArgPath) { | |
| console.error('Error: --jar requires a path argument'); | |
| process.exit(1); | |
| } |
There was a problem hiding this comment.
Fixed — added explicit validation: if --jar is present but no path follows, the script now exits immediately with Error: --jar requires a path argument rather than silently falling through to the env-var/lib lookups.
Incremental make runs without a clean could silently bundle old .class files from deleted .java sources into fixture.jar. Deleting benchmark/ before javac guarantees a clean artefact on every rebuild.
When --jar is the last argument, args[jarArgIdx + 1] is undefined and findJavacgJar silently falls through to the env-var/lib lookups, giving a confusing "JAR not found" message instead of flagging the bad invocation. Fail fast with a clear diagnostic.
…se Javadoc matches
|
Fixed — anchored the |
…into chore/bench-javacg-static-1307
|
Fixed the flag-like string edge case raised in the Greptile summary: |
1 participant
Summary
tests/benchmarks/resolution/fixtures/java/Makefile— compiles all.javasources tofixture.jarviajavac -d . *.java && jar cf fixture.jar benchmark/scripts/compare-javacg.mjs— runs javacg-static on the compiled JAR, parses itsM:pkg.Class:method (T)pkg.Class:methodoutput, maps toClassName.methodform, and computes precision/recall vsexpected-edges.jsondocs/benchmarks/RESOLUTION-COMPARISON.mdwith the javacg-static methodology, a placeholder comparison table, and reproduction instructionsReproducing
Name mapping
javacg-static uses
pkg.ClassName:method(JVM-descriptors)form. The script maps:<init>→ClassName.ClassName(constructor-as-method, matching expected-edges.json)<init>→ClassName(constructor target = class name only)HashMap,String,System.out, …) are filtered out — only edges between fixture classes countCloses #1307