Migrate SQLCommenterTest from Groovy/Spock to JUnit 5 Java#11735
Migrate SQLCommenterTest from Groovy/Spock to JUnit 5 Java#11735dougqh wants to merge 3 commits into
Conversation
Faithful 1:1 migration of the jdbc SQLCommenterTest: all 91 cases preserved (8 getFirstWord + 68 inject + 12 base-hash + 3 peer-service). Spock `where:` tables become @ParameterizedTest + @MethodSource (the data carries embedded quotes, whitespace-significant, and null cells that @TableTest can't represent), each row led by a human-readable scenario for the display name. Config injection uses the imperative WithConfigExtension.injectSysConfig (values are parameterized). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
dougqh
added
tag: no release notes
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
ValentinZakharov
left a comment
ValentinZakharov
left a comment
There was a problem hiding this comment.
Please check the comment; there may be a problem
Otherwise, everything looks good
| arguments( | ||
| "append postgres optimizer hint leading duplicate", | ||
| "/*+ SeqScan(foo) */ SELECT * FROM foo", | ||
| "SqlCommenter", | ||
| "Test", | ||
| "my-service", | ||
| "postgres", | ||
| "h", | ||
| "n", | ||
| "TestVersion", | ||
| true, | ||
| TRACE_PARENT, | ||
| "/*+ SeqScan(foo) */ SELECT * FROM foo /*ddps='SqlCommenter',dde='Test',ddpv='TestVersion',dddbs='my-service',ddh='h',dddb='n',traceparent='00-00000000000000007fffffffffffffff-000000024cb016ea-00'*/"), |
There was a problem hiding this comment.
This is identical to the previous one. I think the idea was to check that applying the comment a second time is safe, but for that the input SQL needs to already contain the DD comment
There was a problem hiding this comment.
Okay, I just let Claude do the conversion, but I'll double check.
There was a problem hiding this comment.
Good catch. That duplicate row came over verbatim from the Groovy data table (it had two identical leading-hint append cases), so the migration copied it faithfully — but it was dead weight. I've repurposed it to test the intended behavior: re-injecting already-commented SQL is a no-op (the input now carries the DD comment, expected unchanged). Passes, and it confirms hasDDComment skips already-commented SQL.
(This reply and fix are from Claude.)
The migration faithfully copied a duplicate row that already existed in the Groovy (two identical leading-hint append cases). Per review, repurpose it to verify the intended behavior: re-injecting already-commented SQL is a no-op (hasDDComment skips it). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2 participants
What Does This Do
Migrates
dd-java-agent/instrumentation/jdbc/src/test/groovy/SQLCommenterTest.groovyto JUnit 5 Java (SQLCommenterTest.java), per the project's "no new Groovy/Spock tests; migrate existing ones" convention.Faithful 1:1 migration — all 91 cases preserved (8
getFirstWord+ 68inject+ 12 base-hash + 3 peer-service), all passing.Motivation
Moving away from Groovy to JUnit - in advance of some SQL integration optimizations
Notes
where:tables →@ParameterizedTest+@MethodSource.@TableTestisn't viable here: theinjectdata carries embedded single-quotes, commas, slashes, whitespace-significant cells, empty strings, andnulls that the pipe-delimited/trimmed format can't represent. Each row leads with a human-readablescenariostring for the test display name.InstrumentationSpecification→AbstractInstrumentationTest; imperativeWithConfigExtension.injectSysConfig(values are parameterized, so not@WithConfig);runUnderTracelambda for the peer-service case.ProcessTags.reset(Config.get())(the no-arg overload is package-private and just delegates), andthrows Exceptionon therunUnderTracetest (itsCallabledeclares it).Validated with
/review-groovy-migration(no findings) and/techdebt(clean).🤖 Generated with Claude Code