Copilot commented on code in PR #2476:
URL: https://github.com/apache/groovy/pull/2476#discussion_r3106490110
##########
subprojects/groovy-ant/src/test/groovy/org/codehaus/groovy/ant/GroovyTest.java:
##########
@@ -179,6 +184,49 @@ public void testFileNameInStackTrace() {
testFileNameInStackTrace("groovyErrorMsg_ExternalFile",
"GroovyTest_errorMessage.groovy");
}
+ public void testInheritAllPassesProjectProperties() {
+ Groovy task = new Groovy();
+ task.setProject(project);
+ project.setProperty("alpha", "1");
+ project.setProperty("beta", "two");
+
+ task.setInheritAll(true);
+ task.passParentAntProperties();
+
+ Map<String, String> passed = collectSysproperties(task);
+ assertEquals("1", passed.get("alpha"));
+ assertEquals("two", passed.get("beta"));
+ }
+
+ public void testInheritAllDoesNotOverrideExplicitSysproperty() {
+ Groovy task = new Groovy();
+ task.setProject(project);
+ project.setProperty("shared", "fromProject");
+
+ Environment.Variable explicit = new Environment.Variable();
+ explicit.setKey("shared");
+ explicit.setValue("fromSysproperty");
+ task.addSysproperty(explicit);
+
+ task.setInheritAll(true);
+ task.passParentAntProperties();
+
+ int sharedCount = 0;
+ for (Environment.Variable v :
task.getSysProperties().getVariablesVector()) {
+ if ("shared".equals(v.getKey())) sharedCount++;
+ }
+ assertEquals(1, sharedCount);
+ assertEquals("fromSysproperty",
collectSysproperties(task).get("shared"));
+ }
Review Comment:
The new tests validate `passParentAntProperties()` directly, but they don’t
exercise the actual forked execution path where this feature is supposed to
work (i.e., that the properties become `-D` system properties in the child
JVM). Consider adding an integration-style test (e.g., an Ant target with
`fork="true" inheritAll="true"` that writes `System.getProperty(...)` values to
an output file which the test can read) to ensure the end-to-end behavior is
covered.
##########
subprojects/groovy-ant/src/main/java/org/codehaus/groovy/ant/Groovy.java:
##########
@@ -697,6 +721,22 @@ private void createClasspathParts() {
}
}
+ // package-private for testing
+ void passParentAntProperties() {
+ Set<String> alreadySet = new HashSet<>();
+ for (Environment.Variable v :
super.getSysProperties().getVariablesVector()) {
+ alreadySet.add(v.getKey());
+ }
+ for (Map.Entry<String, Object> entry :
getProject().getProperties().entrySet()) {
+ Object value = entry.getValue();
+ if (value == null || alreadySet.contains(entry.getKey())) continue;
+ Environment.Variable var = new Environment.Variable();
+ var.setKey(entry.getKey());
+ var.setValue(value.toString());
+ super.addSysproperty(var);
+ }
+ }
Review Comment:
`passParentAntProperties()` is package-private solely to be invoked from
tests. To avoid expanding the production surface area, prefer keeping it
`private` and testing the behavior through the public Ant task API (forked
execution), or otherwise mark the method clearly as test-only (e.g., with a
dedicated annotation/comment policy used elsewhere in the codebase).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
