You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@groovy.apache.org by "paulk-asert (via GitHub)" <gi...@apache.org> on 2026/04/19 08:10:00 UTC
[PR] GROOVY-6908: Groovy Ant task should inherit Ant properties when in "f… [groovy]
paulk-asert opened a new pull request, #2476:
URL: https://github.com/apache/groovy/pull/2476
…orked" mode
--
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: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-6908: Groovy Ant task should inherit Ant properties when in "f… [groovy]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #2476:
URL: https://github.com/apache/groovy/pull/2476#issuecomment-4275521890
## [Codecov](https://app.codecov.io/gh/apache/groovy/pull/2476?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `83.33333%` with `3 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 66.9879%. Comparing base ([`4a57272`](https://app.codecov.io/gh/apache/groovy/commit/4a57272e43772bc4991712df921a49ff8eb0de36?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`e3b13d4`](https://app.codecov.io/gh/apache/groovy/commit/e3b13d45f6f268abdaf2d3f19af9275a92461de1?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 4 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/groovy/pull/2476?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [.../src/main/java/org/codehaus/groovy/ant/Groovy.java](https://app.codecov.io/gh/apache/groovy/pull/2476?src=pr&el=tree&filepath=subprojects%2Fgroovy-ant%2Fsrc%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fant%2FGroovy.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VicHJvamVjdHMvZ3Jvb3Z5LWFudC9zcmMvbWFpbi9qYXZhL29yZy9jb2RlaGF1cy9ncm9vdnkvYW50L0dyb292eS5qYXZh) | 83.3333% | [2 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/groovy/pull/2476?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
[](https://app.codecov.io/gh/apache/groovy/pull/2476?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #2476 +/- ##
==================================================
- Coverage 67.1472% 66.9879% -0.1593%
- Complexity 30944 30949 +5
==================================================
Files 1438 1440 +2
Lines 120148 120468 +320
Branches 21311 21364 +53
==================================================
+ Hits 80676 80699 +23
- Misses 32724 33023 +299
+ Partials 6748 6746 -2
```
| [Files with missing lines](https://app.codecov.io/gh/apache/groovy/pull/2476?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [.../src/main/java/org/codehaus/groovy/ant/Groovy.java](https://app.codecov.io/gh/apache/groovy/pull/2476?src=pr&el=tree&filepath=subprojects%2Fgroovy-ant%2Fsrc%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fant%2FGroovy.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VicHJvamVjdHMvZ3Jvb3Z5LWFudC9zcmMvbWFpbi9qYXZhL29yZy9jb2RlaGF1cy9ncm9vdnkvYW50L0dyb292eS5qYXZh) | `40.4459% <83.3333%> (+2.6080%)` | :arrow_up: |
... and [16 files with indirect coverage changes](https://app.codecov.io/gh/apache/groovy/pull/2476/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-6908: Groovy Ant task should inherit Ant properties when in "f… [groovy]
Posted by "paulk-asert (via GitHub)" <gi...@apache.org>.
paulk-asert merged PR #2476:
URL: https://github.com/apache/groovy/pull/2476
--
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: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GROOVY-6908: Groovy Ant task should inherit Ant properties when in "f… [groovy]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@groovy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org