[jdk17u-dev] RFR: 8336942: Improve test coverage for class loading elements with annotations of different retentions [v2]
Liam Miller-Cushon
cushon at openjdk.org
Sat Nov 9 15:15:45 UTC 2024
On Sat, 9 Nov 2024 08:24:06 GMT, Thomas Fitzsimmons <duke at openjdk.org> wrote:
>> @fitzsim I tried with jtreg `857ed6167418cb4ebe2844fd536461a1649bdced` and `17.0.13+11` and I can reproduce a test failure with that change. Could you double-check you're still unable to reproduce that failure?
>>
>>
>> diff --git a/test/langtools/tools/javac/processing/model/type/BasicAnnoTests.java b/test/langtools/tools/javac/processing/model/type/BasicAnnoTests.java
>> index d4540107022..e22c3d132b9 100644
>> --- a/test/langtools/tools/javac/processing/model/type/BasicAnnoTests.java
>> +++ b/test/langtools/tools/javac/processing/model/type/BasicAnnoTests.java
>> @@ -423,7 +423,7 @@ public class BasicAnnoTests extends JavacTestingAbstractProcessor {
>> // vary position of one annotated
>> // the three above with the corner case of the ambiguos decl + type anno added
>>
>> - @Test(posn=0, annoType=TA.class, expect="1")
>> + @Test(posn=0, annoType=TA.class, expect="33")
>> public @TA(1) int f1;
>>
>> @Test(posn=0, annoType=TA.class, expect="11")
>>
>>
>>
>> make test TEST="jtreg:test/langtools/tools/javac/processing/model/type/BasicAnnoTests.java"
>> ...
>> ./src/jdk17u-dev/test/langtools/tools/javac/processing/model/type/BasicAnnoTests.java:427: error: Unexpected value: 1, expected: 33
>> public @TA(1) int f1;
>> ^
>> 1 error
>
> @cushon I should have included a patch for context; I was modifying the first of the two `Test` annotations _added by this pull request_. Can you try this patch, on top of this pull request?
>
>
> diff --git a/test/langtools/tools/javac/processing/model/type/BasicAnnoTests.java b/test/langtools/tools/javac/processing/model/type/BasicAnnoTests.java
> index d4540107022..06bd32bcfbb 100644
> --- a/test/langtools/tools/javac/processing/model/type/BasicAnnoTests.java
> +++ b/test/langtools/tools/javac/processing/model/type/BasicAnnoTests.java
> @@ -535,7 +535,7 @@ public class BasicAnnoTests extends JavacTestingAbstractProcessor {
> @Test(posn=1, annoType=TA.class, expect="23")
> public Set<@TA(23) ? super Object> f9;
>
> - @Test(posn=0, annoType=TA.class, expect="1")
> + @Test(posn=0, annoType=TA.class, expect="33")
> @Test(posn=0, annoType=TD.class, expect="2")
> public @TA(1) @TD(2) int f10;
>
>
> I get:
>
>
> make test TEST="jtreg:test/langtools/tools/javac/processing/model/type/BasicAnnoTests.java"
> ...
> Passed: tools/javac/processing/model/type/BasicAnnoTests.java
> Test results: passed: 1
> ...
> TEST SUCCESS
>
> but I would expect a failure.
@fitzsim thanks for clarifying, and for the catch! I think that may be a long-standing bug in the test harness, it's grouping the annotations by position and if there are multiple annotations with the same position it only ends up looking at one of them.
I can reproduce the expected failure with your change and something like the following.
I think this issue with the test should be fixed at head, I will follow up on that.
The motivation for originally adding the test in JDK-8336942 that's being backported here was that this example would cause a crash if the implementation failed to handle two annotations with different retentions at the same location, so there's some value in this even without the fix to the test harness.
diff --git a/test/langtools/tools/javac/processing/model/type/BasicAnnoTests.java b/test/langtools/tools/javac/processing/m
odel/type/BasicAnnoTests.java
index d4540107022..718b4aae95a 100644
--- a/test/langtools/tools/javac/processing/model/type/BasicAnnoTests.java
+++ b/test/langtools/tools/javac/processing/model/type/BasicAnnoTests.java
@@ -139,7 +139,7 @@ public class BasicAnnoTests extends JavacTestingAbstractProcessor {
*/
class TestTypeScanner extends TypeScanner<Void, Void> {
Element elem;
- NavigableMap<Integer, AnnotationMirror> toBeFound;
+ NavigableMap<Integer, List<AnnotationMirror>> toBeFound;
int count = 0;
Set<TypeMirror> seen = new HashSet<>();
@@ -147,10 +147,10 @@ public class BasicAnnoTests extends JavacTestingAbstractProcessor {
super(types);
this.elem = elem;
- NavigableMap<Integer, AnnotationMirror> testByPos = new TreeMap<>();
+ NavigableMap<Integer, List<AnnotationMirror>> testByPos = new TreeMap<>();
for (AnnotationMirror test : tests) {
for (int pos : getPosn(test)) {
- testByPos.put(pos, test);
+ testByPos.computeIfAbsent(pos, ArrayList::new).add(test);
}
}
this.toBeFound = testByPos;
@@ -172,17 +172,18 @@ public class BasicAnnoTests extends JavacTestingAbstractProcessor {
out.println("scan " + count + ": " + t);
if (toBeFound.size() > 0) {
if (toBeFound.firstKey().equals(count)) {
- AnnotationMirror test = toBeFound.pollFirstEntry().getValue();
- String annoType = getAnnoType(test);
- AnnotationMirror anno = getAnnotation(t, annoType);
- if (anno == null) {
- error(elem, "annotation not found on " + count + ": " + t);
- } else {
- String v = getValue(anno, "value").toString();
- if (v.equals(getExpect(test))) {
- out.println("found " + anno + " as expected");
+ for (AnnotationMirror test : toBeFound.pollFirstEntry().getValue()) {
+ String annoType = getAnnoType(test);
+ AnnotationMirror anno = getAnnotation(t, annoType);
+ if (anno == null) {
+ error(elem, "annotation not found on " + count + ": " + t);
} else {
- error(elem, "Unexpected value: " + v + ", expected: " + getExpect(test));
+ String v = getValue(anno, "value").toString();
+ if (v.equals(getExpect(test))) {
+ out.println("found " + anno + " as expected");
+ } else {
+ error(elem, "Unexpected value: " + v + ", expected: " + getExpect(test));
+ }
}
}
} else if (count > toBeFound.firstKey()) {
-------------
PR Comment: https://git.openjdk.org/jdk17u-dev/pull/2955#issuecomment-2466253349
More information about the jdk-updates-dev
mailing list