[G2J] call site instrumentation plugin #10462
Conversation
Migration from Groovy to Java for unit tests
jpbempel
added
comp: testing
BenchmarksStartupParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 63 metrics, 8 unstable metrics. Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.59.0-SNAPSHOT~d85492f88d, baseline=1.59.0-SNAPSHOT~949e5f91b3
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.058 s) : 0, 1057622
Total [baseline] (8.677 s) : 0, 8677376
Agent [candidate] (1.059 s) : 0, 1059465
Total [candidate] (8.697 s) : 0, 8697020
section iast
Agent [baseline] (1.225 s) : 0, 1224738
Total [baseline] (9.383 s) : 0, 9383077
Agent [candidate] (1.228 s) : 0, 1227750
Total [candidate] (9.327 s) : 0, 9326533
gantt
title insecure-bank - break down per module: candidate=1.59.0-SNAPSHOT~d85492f88d, baseline=1.59.0-SNAPSHOT~949e5f91b3
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.183 ms) : 0, 1183
crashtracking [candidate] (1.18 ms) : 0, 1180
BytebuddyAgent [baseline] (622.782 ms) : 0, 622782
BytebuddyAgent [candidate] (624.065 ms) : 0, 624065
AgentMeter [baseline] (28.801 ms) : 0, 28801
AgentMeter [candidate] (28.84 ms) : 0, 28840
GlobalTracer [baseline] (256.996 ms) : 0, 256996
GlobalTracer [candidate] (257.768 ms) : 0, 257768
AppSec [baseline] (32.863 ms) : 0, 32863
AppSec [candidate] (32.706 ms) : 0, 32706
Debugger [baseline] (62.719 ms) : 0, 62719
Debugger [candidate] (60.194 ms) : 0, 60194
Remote Config [baseline] (637.814 µs) : 0, 638
Remote Config [candidate] (620.909 µs) : 0, 621
Telemetry [baseline] (11.583 ms) : 0, 11583
Telemetry [candidate] (9.843 ms) : 0, 9843
Flare Poller [baseline] (4.524 ms) : 0, 4524
Flare Poller [candidate] (8.503 ms) : 0, 8503
section iast
crashtracking [baseline] (1.175 ms) : 0, 1175
crashtracking [candidate] (1.191 ms) : 0, 1191
BytebuddyAgent [baseline] (789.077 ms) : 0, 789077
BytebuddyAgent [candidate] (793.676 ms) : 0, 793676
AgentMeter [baseline] (11.185 ms) : 0, 11185
AgentMeter [candidate] (11.113 ms) : 0, 11113
GlobalTracer [baseline] (247.419 ms) : 0, 247419
GlobalTracer [candidate] (247.362 ms) : 0, 247362
AppSec [baseline] (33.434 ms) : 0, 33434
AppSec [candidate] (32.976 ms) : 0, 32976
Debugger [baseline] (67.383 ms) : 0, 67383
Debugger [candidate] (66.52 ms) : 0, 66520
Remote Config [baseline] (550.037 µs) : 0, 550
Remote Config [candidate] (540.39 µs) : 0, 540
Telemetry [baseline] (8.66 ms) : 0, 8660
Telemetry [candidate] (8.561 ms) : 0, 8561
Flare Poller [baseline] (3.508 ms) : 0, 3508
Flare Poller [candidate] (3.474 ms) : 0, 3474
IAST [baseline] (26.873 ms) : 0, 26873
IAST [candidate] (26.743 ms) : 0, 26743
Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.59.0-SNAPSHOT~d85492f88d, baseline=1.59.0-SNAPSHOT~949e5f91b3
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.061 s) : 0, 1061016
Total [baseline] (11.002 s) : 0, 11001600
Agent [candidate] (1.06 s) : 0, 1060417
Total [candidate] (11.009 s) : 0, 11008635
section appsec
Agent [baseline] (1.231 s) : 0, 1230725
Total [baseline] (10.898 s) : 0, 10897782
Agent [candidate] (1.236 s) : 0, 1235752
Total [candidate] (11.051 s) : 0, 11050680
section iast
Agent [baseline] (1.226 s) : 0, 1226497
Total [baseline] (11.15 s) : 0, 11150322
Agent [candidate] (1.229 s) : 0, 1229088
Total [candidate] (11.21 s) : 0, 11209957
section profiling
Agent [baseline] (1.185 s) : 0, 1185278
Total [baseline] (10.808 s) : 0, 10808309
Agent [candidate] (1.212 s) : 0, 1212288
Total [candidate] (11.023 s) : 0, 11022764
gantt
title petclinic - break down per module: candidate=1.59.0-SNAPSHOT~d85492f88d, baseline=1.59.0-SNAPSHOT~949e5f91b3
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.183 ms) : 0, 1183
crashtracking [candidate] (1.184 ms) : 0, 1184
BytebuddyAgent [baseline] (623.677 ms) : 0, 623677
BytebuddyAgent [candidate] (623.716 ms) : 0, 623716
AgentMeter [baseline] (28.905 ms) : 0, 28905
AgentMeter [candidate] (28.835 ms) : 0, 28835
GlobalTracer [baseline] (257.716 ms) : 0, 257716
GlobalTracer [candidate] (257.816 ms) : 0, 257816
AppSec [baseline] (33.042 ms) : 0, 33042
AppSec [candidate] (33.01 ms) : 0, 33010
Debugger [baseline] (62.139 ms) : 0, 62139
Debugger [candidate] (62.346 ms) : 0, 62346
Remote Config [baseline] (638.047 µs) : 0, 638
Remote Config [candidate] (627.645 µs) : 0, 628
Telemetry [baseline] (9.156 ms) : 0, 9156
Telemetry [candidate] (10.676 ms) : 0, 10676
Flare Poller [baseline] (9.108 ms) : 0, 9108
Flare Poller [candidate] (6.799 ms) : 0, 6799
section appsec
crashtracking [baseline] (1.183 ms) : 0, 1183
crashtracking [candidate] (1.184 ms) : 0, 1184
BytebuddyAgent [baseline] (651.555 ms) : 0, 651555
BytebuddyAgent [candidate] (653.142 ms) : 0, 653142
AgentMeter [baseline] (11.812 ms) : 0, 11812
AgentMeter [candidate] (11.982 ms) : 0, 11982
GlobalTracer [baseline] (257.188 ms) : 0, 257188
GlobalTracer [candidate] (259.052 ms) : 0, 259052
AppSec [baseline] (167.15 ms) : 0, 167150
AppSec [candidate] (167.818 ms) : 0, 167818
Debugger [baseline] (67.48 ms) : 0, 67480
Debugger [candidate] (67.769 ms) : 0, 67769
Remote Config [baseline] (670.082 µs) : 0, 670
Remote Config [candidate] (688.002 µs) : 0, 688
Telemetry [baseline] (9.162 ms) : 0, 9162
Telemetry [candidate] (9.358 ms) : 0, 9358
Flare Poller [baseline] (3.658 ms) : 0, 3658
Flare Poller [candidate] (3.724 ms) : 0, 3724
IAST [baseline] (25.585 ms) : 0, 25585
IAST [candidate] (25.605 ms) : 0, 25605
section iast
crashtracking [baseline] (1.177 ms) : 0, 1177
crashtracking [candidate] (1.191 ms) : 0, 1191
BytebuddyAgent [baseline] (789.846 ms) : 0, 789846
BytebuddyAgent [candidate] (792.348 ms) : 0, 792348
AgentMeter [baseline] (11.16 ms) : 0, 11160
AgentMeter [candidate] (11.196 ms) : 0, 11196
GlobalTracer [baseline] (248.217 ms) : 0, 248217
GlobalTracer [candidate] (248.82 ms) : 0, 248820
AppSec [baseline] (34.11 ms) : 0, 34110
AppSec [candidate] (34.582 ms) : 0, 34582
Debugger [baseline] (66.909 ms) : 0, 66909
Debugger [candidate] (66.042 ms) : 0, 66042
Remote Config [baseline] (570.007 µs) : 0, 570
Remote Config [candidate] (561.786 µs) : 0, 562
Telemetry [baseline] (8.58 ms) : 0, 8580
Telemetry [candidate] (8.554 ms) : 0, 8554
Flare Poller [baseline] (3.457 ms) : 0, 3457
Flare Poller [candidate] (3.423 ms) : 0, 3423
IAST [baseline] (27.018 ms) : 0, 27018
IAST [candidate] (26.878 ms) : 0, 26878
section profiling
ProfilingAgent [baseline] (99.534 ms) : 0, 99534
ProfilingAgent [candidate] (101.332 ms) : 0, 101332
crashtracking [baseline] (1.206 ms) : 0, 1206
crashtracking [candidate] (1.245 ms) : 0, 1245
BytebuddyAgent [baseline] (676.21 ms) : 0, 676210
BytebuddyAgent [candidate] (692.637 ms) : 0, 692637
AgentMeter [baseline] (8.637 ms) : 0, 8637
AgentMeter [candidate] (8.809 ms) : 0, 8809
GlobalTracer [baseline] (216.714 ms) : 0, 216714
GlobalTracer [candidate] (221.049 ms) : 0, 221049
AppSec [baseline] (32.316 ms) : 0, 32316
AppSec [candidate] (33.303 ms) : 0, 33303
Debugger [baseline] (67.651 ms) : 0, 67651
Debugger [candidate] (69.157 ms) : 0, 69157
Remote Config [baseline] (593.722 µs) : 0, 594
Remote Config [candidate] (607.479 µs) : 0, 607
Telemetry [baseline] (8.805 ms) : 0, 8805
Telemetry [candidate] (8.894 ms) : 0, 8894
Flare Poller [baseline] (3.804 ms) : 0, 3804
Flare Poller [candidate] (3.871 ms) : 0, 3871
Profiling [baseline] (100.115 ms) : 0, 100115
Profiling [candidate] (101.924 ms) : 0, 101924
LoadParameters
See matching parameters
SummaryFound 1 performance improvements and 1 performance regressions! Performance is the same for 18 metrics, 16 unstable metrics.
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.59.0-SNAPSHOT~d85492f88d, baseline=1.59.0-SNAPSHOT~949e5f91b3
dateFormat X
axisFormat %s
section baseline
no_agent (18.907 ms) : 18715, 19100
. : milestone, 18907,
appsec (18.363 ms) : 18178, 18547
. : milestone, 18363,
code_origins (17.739 ms) : 17563, 17914
. : milestone, 17739,
iast (17.622 ms) : 17446, 17797
. : milestone, 17622,
profiling (19.482 ms) : 19284, 19680
. : milestone, 19482,
tracing (17.58 ms) : 17408, 17753
. : milestone, 17580,
section candidate
no_agent (16.984 ms) : 16811, 17157
. : milestone, 16984,
appsec (18.275 ms) : 18088, 18462
. : milestone, 18275,
code_origins (17.547 ms) : 17371, 17722
. : milestone, 17547,
iast (17.614 ms) : 17439, 17788
. : milestone, 17614,
profiling (18.956 ms) : 18765, 19148
. : milestone, 18956,
tracing (17.569 ms) : 17397, 17741
. : milestone, 17569,
Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.59.0-SNAPSHOT~d85492f88d, baseline=1.59.0-SNAPSHOT~949e5f91b3
dateFormat X
axisFormat %s
section baseline
no_agent (1.202 ms) : 1190, 1214
. : milestone, 1202,
iast (3.232 ms) : 3190, 3275
. : milestone, 3232,
iast_FULL (5.863 ms) : 5804, 5922
. : milestone, 5863,
iast_GLOBAL (3.497 ms) : 3447, 3548
. : milestone, 3497,
profiling (2.086 ms) : 2068, 2104
. : milestone, 2086,
tracing (1.865 ms) : 1849, 1881
. : milestone, 1865,
section candidate
no_agent (1.188 ms) : 1176, 1199
. : milestone, 1188,
iast (3.164 ms) : 3125, 3204
. : milestone, 3164,
iast_FULL (5.71 ms) : 5653, 5768
. : milestone, 5710,
iast_GLOBAL (3.677 ms) : 3623, 3732
. : milestone, 3677,
profiling (1.952 ms) : 1935, 1969
. : milestone, 1952,
tracing (1.811 ms) : 1795, 1826
. : milestone, 1811,
DacapoParameters
See matching parameters
SummaryFound 1 performance improvements and 0 performance regressions! Performance is the same for 10 metrics, 1 unstable metrics.
Execution time for biojavagantt
title biojava - execution time [CI 0.99] : candidate=1.59.0-SNAPSHOT~d85492f88d, baseline=1.59.0-SNAPSHOT~949e5f91b3
dateFormat X
axisFormat %s
section baseline
no_agent (14.778 s) : 14778000, 14778000
. : milestone, 14778000,
appsec (15.007 s) : 15007000, 15007000
. : milestone, 15007000,
iast (18.613 s) : 18613000, 18613000
. : milestone, 18613000,
iast_GLOBAL (18.263 s) : 18263000, 18263000
. : milestone, 18263000,
profiling (14.958 s) : 14958000, 14958000
. : milestone, 14958000,
tracing (14.518 s) : 14518000, 14518000
. : milestone, 14518000,
section candidate
no_agent (15.782 s) : 15782000, 15782000
. : milestone, 15782000,
appsec (15.03 s) : 15030000, 15030000
. : milestone, 15030000,
iast (18.201 s) : 18201000, 18201000
. : milestone, 18201000,
iast_GLOBAL (18.03 s) : 18030000, 18030000
. : milestone, 18030000,
profiling (14.753 s) : 14753000, 14753000
. : milestone, 14753000,
tracing (14.482 s) : 14482000, 14482000
. : milestone, 14482000,
Execution time for tomcatgantt
title tomcat - execution time [CI 0.99] : candidate=1.59.0-SNAPSHOT~d85492f88d, baseline=1.59.0-SNAPSHOT~949e5f91b3
dateFormat X
axisFormat %s
section baseline
no_agent (1.477 ms) : 1465, 1488
. : milestone, 1477,
appsec (3.784 ms) : 3560, 4007
. : milestone, 3784,
iast (2.262 ms) : 2192, 2331
. : milestone, 2262,
iast_GLOBAL (2.294 ms) : 2225, 2364
. : milestone, 2294,
profiling (2.079 ms) : 2025, 2134
. : milestone, 2079,
tracing (2.06 ms) : 2006, 2113
. : milestone, 2060,
section candidate
no_agent (1.476 ms) : 1464, 1487
. : milestone, 1476,
appsec (2.541 ms) : 2484, 2599
. : milestone, 2541,
iast (2.271 ms) : 2201, 2340
. : milestone, 2271,
iast_GLOBAL (2.311 ms) : 2241, 2381
. : milestone, 2311,
profiling (2.489 ms) : 2276, 2703
. : milestone, 2489,
tracing (2.078 ms) : 2025, 2132
. : milestone, 2078,
|
| testImplementation(libs.bundles.groovy) | ||
| testImplementation(libs.bundles.spock) |
There was a problem hiding this comment.
❔ question: Are groovy and spock bundles still needed?
tooling/move-groovy-to-java.sh
Outdated
| # | ||
| # Finds all directories matching */src/test/groovy under the start folder, | ||
| # ensures corresponding src/test/java exists, mirrors missing subdirs, | ||
| # moves files with `git mv` (preserving history) and commits the changes. |
There was a problem hiding this comment.
🔨 issue: As raised in Slack, the script won't help to get changes marked as moved.
There was a problem hiding this comment.
Yep... Looks like we have to review code as if it completely new code.
not sure how reviewers can compare java and groovy code :(
| testImplementation(libs.bundles.groovy) | ||
| testImplementation(libs.bundles.spock) |
There was a problem hiding this comment.
We can delete groovy and spock :)
| CompilationUnit javaFile; | ||
| try { | ||
| javaFile = parseJavaFile(file); | ||
| } catch (FileNotFoundException e) { | ||
| throw new RuntimeException(e); | ||
| } |
There was a problem hiding this comment.
minor: I think we not need to wrap with try/catch in tests, we can simply propagate exceptions via throws ddeclaration.
| if (javaFile.getParsed() != Node.Parsedness.PARSED) { | ||
| throw new IllegalStateException("Failed to parse file: " + file); | ||
| } |
There was a problem hiding this comment.
I believe this should be assertEquals()?
| Method enabled = null; | ||
| Set<String> enabledArgs = null; | ||
| Object[] enabledDeclaration = getEnabledDeclaration(targetType, interfaces); | ||
| if (enabledDeclaration != null) { | ||
| enabled = (Method) enabledDeclaration[0]; | ||
| enabledArgs = (Set<String>) enabledDeclaration[1]; | ||
| } |
There was a problem hiding this comment.
not sure that we need to have check for null in test?
Groovy code had no checks...
There was a problem hiding this comment.
Groovy code was using a tuple for returning 2 objects and it was converted to a object array.
I don't see a problem here, the method can return null and assignment to the local var is only done when != null
|
Hi! 👋 Thanks for your pull request! 🎉 To help us review it, please make sure to:
If you need help, please check our contributing guidelines. |
| public void statements(String... values) { | ||
| assertArrayEquals(values, statements.toArray(new String[0])); | ||
| } |
There was a problem hiding this comment.
idea: possible candidate for test-util, something like assertArraysAndListEquals() ?
There was a problem hiding this comment.
In my opinion it's not worth it, we should better use AssertJ in that case. But it can be done later.
| Method enabled = null; | ||
| Set<String> enabledArgs = null; | ||
| Object[] enabledDeclaration = getEnabledDeclaration(targetType, interfaces); | ||
| if (enabledDeclaration != null) { | ||
| enabled = (Method) enabledDeclaration[0]; | ||
| enabledArgs = (Set<String>) enabledDeclaration[1]; | ||
| } |
There was a problem hiding this comment.
Can we just retrun array of [null, null] as it was in Groovy?
and code will be simple as:
Method enabled = (Method) enabledDeclaration[0];
Set<String> enabledArgs = (Set<String>) enabledDeclaration[1];
OR
may be it will be a good idea to introduce utility Tuple<K, V> class and make things typed.
| lastDot = current.lastIndexOf('.', lastDot - 1); | ||
| } | ||
| } | ||
| throw new RuntimeException(new ClassNotFoundException(qualifiedName)); |
There was a problem hiding this comment.
Just curious if this can be simplified with do... while? I feel that we can have one try/catch less?
| protected Object[] getEnabledDeclaration( | ||
| ClassOrInterfaceDeclaration type, Set<Class<?>> interfaces) { | ||
| if (!interfaces.contains(CallSites.HasEnabledProperty.class)) { | ||
| return null; |
There was a problem hiding this comment.
Here we can return new Object[] {null, null}; or event Tuple (mentioned in other comment).
| protected Set<Class<?>> interfaces; | ||
| protected Set<Class<?>> spi; | ||
| protected Set<Class<?>> helpers; | ||
| protected List<AdviceAssert> advices; | ||
| protected Method enabled; | ||
| protected Set<String> enabledArgs; |
There was a problem hiding this comment.
nitpick: lets reorder lines, sets, list, method? just for aesthetic :), here and other places
Also: can these fields be private final?
There was a problem hiding this comment.
This is the way it was declared in Groovy. I prefer to keep it like this. Free to refactor this later.
| static Stream<Arguments> testThatExtensionOnlyAppliesToIastAdvicesProvider() { | ||
| return Stream.of( | ||
| Arguments.of(CallSites.class.getName(), false), | ||
| Arguments.of(IastExtension.IAST_CALL_SITES_FQCN, true)); | ||
| } | ||
|
|
||
| @ParameterizedTest | ||
| @MethodSource("testThatExtensionOnlyAppliesToIastAdvicesProvider") |
There was a problem hiding this comment.
Can we try to use @CsvSource? it is almost same as Spock tables with params:
See for example here:
| if (!result.isSuccess()) { | ||
| throw new IllegalArgumentException("Error with call site " + IastExtensionCallSite.class); | ||
| } |
| static Stream<Arguments> aroundAdviceReturnTypeProvider() { | ||
| return Stream.of( | ||
| Arguments.of(MessageDigest.class, 1), | ||
| Arguments.of(Object.class, 0), | ||
| Arguments.of(String.class, 0)); | ||
| } | ||
|
|
||
| @ParameterizedTest |
There was a problem hiding this comment.
Same here, probably we may use @CsvSource? Here and similar places...
There was a problem hiding this comment.
I think the issue is converting the param to the actual instance.
There was a problem hiding this comment.
correct, it is not obviously better here
|
|
||
| @Test | ||
| void testResolvePrimitive() { | ||
| TypeResolverPool resolver = new TypeResolverPool(); |
There was a problem hiding this comment.
Just a question: can we have one instance of TypeResolverPool resolver = new TypeResolverPool(); for all tests? To simplify code a bit?
tooling/move-groovy-to-java.sh
Outdated
| # | ||
| # Finds all directories matching */src/test/groovy under the start folder, | ||
| # ensures corresponding src/test/java exists, mirrors missing subdirs, | ||
| # moves files with `git mv` (preserving history) and commits the changes. |
There was a problem hiding this comment.
Yep... Looks like we have to review code as if it completely new code.
not sure how reviewers can compare java and groovy code :(
bric3
left a comment
bric3
left a comment
There was a problem hiding this comment.
It's nice! There's two point that I think are worth tackling soon.
-
I believe that using snake case for the test methods should be addressed now, because this PR comes from the original groovy files (in this project), and it compensate a major selling point of groovy, readability.
-
Assertions use JUnit's assertion API, but it requires numerous type conversion, extraction etc. I believe such conversion would benefit highly from telling Claude to use AssertJ instead which has a richer and expressive assertion API.
| public void statements(String... values) { | ||
| assertArrayEquals(values, statements.toArray(new String[0])); | ||
| } |
There was a problem hiding this comment.
In my opinion it's not worth it, we should better use AssertJ in that case. But it can be done later.
| getHelpers(targetType), | ||
| getAdvices(targetType), | ||
| enabled, | ||
| enabledArgs); |
There was a problem hiding this comment.
suggestion: Maybe add to the skill that variable and assignment should be joined. E.g.
CompilationUnit javaFile = parseJavaFile(file);Method enabled = (Method) enabledDeclaration[0];Set<String> enabledArgs = (Set<String>) enabledDeclaration[1];
...trumentation-plugin/src/test/java/datadog/trace/plugin/csi/impl/assertion/AssertBuilder.java
Show resolved
Hide resolved
| .getBody() | ||
| .get() | ||
| .getStatements() | ||
| .get(0) |
There was a problem hiding this comment.
note: FYI when working previous on bumping the Gradle JDK I discovered there's a getFirst() in some part of the JavaParser api, like this one.
| "datadog.trace.agent.tooling.csi.CallSites | false", | ||
| "datadog.trace.agent.tooling.iast.IastCallSites | true" | ||
| }) | ||
| void testThatExtensionOnlyAppliesToIastAdvices(String typeName, boolean expected) { |
There was a problem hiding this comment.
suggestion: For readability, and for the whole Goal to convert to Groovy I think it is advisable to tell claude to use snake case.
| void testThatExtensionOnlyAppliesToIastAdvices(String typeName, boolean expected) { | |
| void test_that_extension_only_applies_to_iast_advices(String typeName, boolean expected) { |
For comparison the original groovy test:
void 'test that extension only applies to iast advices'() {There was a problem hiding this comment.
I am fine with CamelCase instead of snake_case. It's more Java style.
Want the input from others about this to have consensus
There was a problem hiding this comment.
I would agree for regular code, but tests have a different angle. One express scenario which makes it tougher to quickly grasp.
Note that even in java some code use underscore. E.g., code that use MethodHandle, or else, can use _MH a lot.
There was a problem hiding this comment.
with this instructions, forw groovy migration all the tests will have the snake_case not just "some"
I agree that in some cases it make sense to have some underscores, I am using it also time to time
| static Stream<Arguments> aroundAdviceReturnTypeProvider() { | ||
| return Stream.of( | ||
| Arguments.of(MessageDigest.class, 1), | ||
| Arguments.of(Object.class, 0), | ||
| Arguments.of(String.class, 0)); | ||
| } | ||
|
|
||
| @ParameterizedTest |
There was a problem hiding this comment.
I think the issue is converting the param to the actual instance.
|
|
||
| When converting groovy code to java code make sure that: | ||
| - the Java code generated is compatible with JDK 8 | ||
| - when translating Spock test, favor using `@CsvSource` with `|` delimiters |
There was a problem hiding this comment.
suggestion: New point:
when using a `@MethodSource`, use the test method name, and suffix it with `_parameters`
There was a problem hiding this comment.
Wouldn't be arguments according to JUnit naming?
There was a problem hiding this comment.
Yes possibly, both are fine by me, but let's keep it closer to JUnit for this one.
| 3. make sure the tests are still passing after migration | ||
| 4. remove groovy files | ||
|
|
||
| When converting groovy code to java code make sure that: |
There was a problem hiding this comment.
New point:
- Use snake case for converted test methods.
| Optional<CallSiteSpecification> result = specificationBuilder.build(advice); | ||
|
|
||
| assertFalse(result.isPresent()); |
There was a problem hiding this comment.
thought: What do you think abou tusing AssertJ instead of JUnit assert as a default conversion choice. This could avoid conversions for lists or arrays in particular, and provide a richer assertion API.
| - when converting tuples always returns object array | ||
| - Instead of checking a state and throwing an exception, use JUnit asserts | ||
| - Do not wrap checked exception and throwing a Runtime exception, prefer adding a throws clause at method declaration | ||
| - Do not mark local variables `final` |
There was a problem hiding this comment.
suggestion: What about using AssertJ's richer assertion API rather than using JUnit ones. This would avoid unnecessary type conversions and would ease reading the code.
|
|
||
| When converting groovy code to java code make sure that: | ||
| - the Java code generated is compatible with JDK 8 | ||
| - when translating Spock test, favor using `@CsvSource` with `|` delimiters |
There was a problem hiding this comment.
Wouldn't be arguments according to JUnit naming?
| When converting groovy code to java code make sure that: | ||
| - the Java code generated is compatible with JDK 8 | ||
| - when translating Spock test, favor using `@CsvSource` with `|` delimiters | ||
| - when converting tuples always returns object array |
There was a problem hiding this comment.
Create light dedicated structure instead to keep the typing system
| 2. convert groovy files to Java using Junit 5 | ||
| 3. make sure the tests are still passing after migration | ||
| 4. remove groovy files | ||
|
|
There was a problem hiding this comment.
🎯 suggestion:
| 5. avoid duplicating code and refactor if possible to keep the codebase as light as possible |
There was a problem hiding this comment.
Cool! Did you get a better code with this skill? Can we have some instructions how to use it?
There was a problem hiding this comment.
just type /migrate-groovy-to-java in claude code prompt
There was a problem hiding this comment.
Regarding suggestion: Prefer to keep refactoring separate from migration
I am fine with CamelCase (Java style), unless consensus emerge that we should use snake_case
It's introducing a new library. I want consensus on this before adding AssertJ. Personally I prefer to stick to Junit assertions. otherwise it will mix both assertions types and we need to teach Claude when to use one or the other, no? |
|
Don't we already have assertj. If not I believe using assertj as a reflex should be on the table if we want to migrate from spock to java. This library is a solid tool in the Java testing world. |
don't know, not using it |
AlexeyKuznetsov-DD
left a comment
AlexeyKuznetsov-DD
left a comment
There was a problem hiding this comment.
This can be a first variant that we can improve in follow up PRs.
But lets' get approval from others.
modified SKILL
What Does This Do
Migration from Groovy to Java for unit tests
Motivation
Additional Notes
Tests run using:
./gradlew :buildSrc:call-site-instrumentation-plugin:test -PrunBuildSrcTestsTo review migration groovy -> java use this commit
Contributor Checklist
type:and (comp:orinst:) labels in addition to any useful labelsclose,fixor any linking keywords when referencing an issue.Use
solvesinstead, and assign the PR milestone to the issueJira ticket: [PROJ-IDENT]