JDK-8013497 : Write type-annotation tests for core reflection
Type:Bug
Component:core-libs
Sub-Component:java.lang:reflect
Affected Version:8
Priority:P3
Status:In Progress
Resolution:Unresolved
Submitted:2013-04-29
Updated:2014-05-22
The Version table provides details related to the release that this issue/RFE will be addressed.
Unresolved : Release in which this issue/RFE will be addressed. Resolved: Release in which this issue/RFE has been resolved. Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.
Need tests for added core reflection support for type-annotations.
Comments
http://sqeweb.us.oracle.com/jsn/users/charlie/type-annotations/test/type-annotations/
Hi Charlie,
I think this is much better!! but we can still make some improvements. Some comments:
These tests will go into the tl forrest, so please add them to a tl/jdk repo. A good location is probably jdk/test/java/lang/annotations/typeAnnotations/combo/
Please generate webrevs, it makes it much easier to comment on code since line numbers are present for example.
Some comments on the code:
ClassGetAnnotatedSuperclassTest:
{ TypeAnnoCls01.class, new AnnotatedTypeResult() },
{ TypeAnnoCls02.class, new AnnotatedTypeResult() },
I would prefer you added a method to get an empty AnnotatedTypeResult, because that is what the new does right?
{ TypeAnnoCls07.class,
new AnnotatedTypeResult().add(new AnnotationResult(
"TypeAnno3Container")
.addRepeatedAnno("TypeAnno3", null)
.addRepeatedAnno("TypeAnno3", null)) } };
In general I like this fluent style/builder style of constructing the data, but can you get rid of the "new" calls here as that would make the code more readable. Can you add factory methods so that you don't need the constructor calls?
Helper:
In general, there are a lot for return null; That always makes me uneasy.
public class Helper {
// Compare AnnotatedType with expect result
public static boolean compareAT(AnnotatedTypeResult atRt, AnnotatedType at)
It is better Java style to make the methods instance methods and create a Helper instance in the tests. That might also prove to be useful to implement error out dumping, IE you can give a file to the Helper File where it would dump a repro.
@Override
public boolean equals(Object o) {
assertTrue(o instanceof AnnotationResult);
AnnotationResult anno = (AnnotationResult) o;
In general you should use an instanceof here and return false if it fails. Or better yet, since you don't really fix hashCode() why not add a isEqualResult(AnnotationResult r) and skip overriding equals and hashcode. Will work better with Maps as well.
cheers
/Joel
18-10-2013
people's comments(9/24/2013)
Hi Charlie,
Again I'm sorry you have ended up between multiplier reviewers.
In my last mail I was unclear, let me rephrase:
There are a handful of openjdk reviewers [1][2] that will review changes to reflection. I am certain none of them will approve this without talking to me, and I will say the code is not ready. Your chances of finding someone to push this in to TL for you are close to 0. I might as well have said, "this code is not going into TL in its current form, period".
Please take some time understanding the rules of openjdk as explained here: http://openjdk.java.net/contribute/ and here: http://openjdk.java.net/guide/ . Basically "many rounds of reviews in type-annotations-dev" and "people approved it" does not mean it is ready for TL. All code that goes from a project repo into the master repositories will be reviewed (again) when being committed to the master repositories.
This leaves you with 2 practical options:
1) Keep improving the tests until an openjdk jdk8 reviewer agrees the tests are ready for TL.
2) Put the tests somewhere else.
I prefer option 1).
Perhaps a compromise would be to have Steve help you get this into langtools-sqe nightly and you can continue to improve the tests until they are ready to be pushed to TL, but this is something Steve would have to OK.
[1] http://openjdk.java.net/bylaws#reviewer
[2] http://openjdk.java.net/census#jdk8
cheers
/Joel
27-09-2013
people's comments(9/23/13)
Hi Charlie,
Taking this of core-libs-dev but adding Michel.
I'm sorry, but this code is not going into TL anytime soon.
I'll try to explain why:
1) I can't verify it. If you are writing tests that generate source or
has a combinatorial approach you need to be able to verify it. How do I
do that with this code?
Take a look at Amy's tests that I helped push:
http://hg.openjdk.java.net/jdk8/jdk8/jdk/rev/cd0ea5563523
Each test case is annotated with its expected values. How can I find out
what the code in this framework expects?
I did send Amy's tests to a colleague that helped verify that the
expected correct answers were indeed correct. I didn't explain anything
to him, but he understood the annotations anyway. I can't do that in
this case. To be honest I can't even find out the what the tests
considers the correct answer.
If I can't verify the tests, they could equally well be testing nothing,
I have no way of knowing.
2) The code is simply not finished. It does not have production quality.
Here are a few examples:
- grep for println(23).
- in ClassGetAnnotatedInterface
207 for (String clsName : TestCase.testInput.keySet()) {
208 if (null == clsName) {
209 continue;
210 }
Code like this looks very suspect.
- A minor thing, tests should be in a tl/jdk subdirectory, relative
@library paths won't work when you move them there.
3) If you do a source generation framework of this size and complexity,
you should test the framework.
I'm sorry this has gone through multiple reviews but that does not
enable me to approve it. It does not matter that other people have
reviewed the code and you have fixed their comments. I won't push this
to TL until I am satisfied. I would guess you are halfway there, there
is a significant amount of work to be done before I will approve this.
So you have three options. Either you find another reviewer/sponsor, or
you push this to a repository somewhere else, or you work with me to
make the code better so that I can push it for to TL. I vote for option
3: make this code better.
If you chose that, here are absolute requirements on the code:
1) It MUST be easy for me as a reviewer to see:
a) What source you are testing
b) What annotations you are expecting to see on that source
c) The source of a test that fails
d) Why a failing test fails
- Currently it is so hard to see a) that you have included a set of
text files that describe the source. This is not ok.
- I can't even begin to find out about b).
- c) I suspect you have fixed, and I'm not sure about d).
- But a) and b) are _very_ important. A test case which no-one
understands is worse than no test case.
2) If you use a template framework/source generation it MUST be
reasonably easy for me to understand that framework.
If a source generation framework is complex it should be tested. This
is done for the lambda test framework, where there are separate tests
that test the framework.
Helper is 1000 lines long and very hard to understand. I must be able
to understand it in order to review it.
To fix this, here is what I would focus on:
i) Delete the source generation framework. You already have generated
the source once, I don't mind if you have a subdirectory with 50
javafiles that have been generated before being checked in.
ii) Improve how you encode the expected behaviour. Use a golden file
containing the answers. Steve can help you with this. But essentially I
need to be able to look at the answer file and say "Ok, this is what the
spec says".
iii) Simplify the checking logic. This will probably be easy if you do the
above.
iv) Simplify, simplify, simplify. The burden is on you to prove these
tests correct. That will be much easier if the code is simple.
This will enable me to:
A) Look at the source and understand what source you are running the
tests on.
B) Look at the answer file and make sure that the expected answers are
what the spec says.
C) Verify your checking algorithm, making sure that you check for common
errors, and different failure modes.
Cheers
/Joel
people's comments(9/13/13)
Can you make it so is displays or leave on disk the test source for failed test cases by default apart from DEBUG?
Since we're really most interested in failed test cases and the ability to easily reproduce them with a small(er) test case, that would be the desired behavior.
I.E., we don't want to say, "here's bug, to reproduce, run the whole test", or even "you can set DEBUG and go fish for the failed test case from the thousands in the debug output"....put in the most unfriendly terms. (I do smile when I write these things).
-steve
16-09-2013
people's comments(9/11/13)
Hi Charlie,
These tests should go into TL, I can commit them there. The test review
cycle should be on core-libs-dev@openjdk.java.net.
However the test are not ready yet.
If you run the tests vs jdk8-b106 or a recent copy of TL there is a few
failures. How can I as a developer see the exact source which caused the
failure?
Further, there is an @author tag between two jtreg tags, @build and
@run in almost every file. Please don't do that.
As a user of these tests there needs to be a way for me to:
1) Understand the tests / ensure that the tests are correct
2) Run the tests
3) Understand what causes tests failures
Out of these only 2) is currently easy. 1) is hard and 3) looks
impossible.
Here is what you need to do to bring this forward:
- At a minimum you need to document how the tests work. It is not clear
at the moment.
- Please explain why are the helpers split out into 2 interfaces and 4
classes?
- You also need to improve failure friendliness. If a tests fail, it
must be possible to see the java source of the generated program that
fails together with a decent explanation of what went wrong.
If this is currently available, please indicate how to do it.
cheers
/Joel
16-09-2013
people's comments(8/29/13)
Charlie,
I can't devote any more time to these tests. Sergey and I have given multiple rounds of detailed feedback on the architecture of your test framework and on individual lines of code. You own these tests; it is your responsibility to take feedback and improve them.
Alex
16-09-2013
people's comments(8/24/13)
I saw a few comments from Sergey Grinev that you might want to consider.
I do not believe you can put code back directly, but you will need to either
1. have Werner put them back via the type-annotation repo (he is not a committer to jdk8/tl either)
2. have someone put them back to jdk8/tl/jdk (not sure who at the moment...Alex is also not a committer)
However, either way, you need to clone a jdk (probably http://hg.openjdk.java.net/jdk8/tl/jdk/) repository and create a webrev with your addition to that. Then you will better be prepare to ask someone to do the push.
-steve
16-09-2013
people's comments(8/23/13)
Hi, all.
Sorry if this review covers same issue I reported on reviewboard one, but it seems few important point were not fixed:
1. public Map<Helper.Declaration, Object> lhm = new LinkedHashMap<>();
and
public synchronized static String genDeclaration(
Map<Helper.Declaration, Object> input, Declaration... srcType)
- fields used as a method-local temporary variables are error-prone and confusing. Users/readers of that code are not aware when this field can be used or when this field can be cleared, also using such field is not thread-safe. Imagine someone added one more ENUM value to TestCase, used lhm and didn't call lhm.clear() -- things will break and very different place this person didn't even touch.
- Map claims it stores Objects but actually they are only String and String[]. Thus your getDeclaration() has invalid signature and is error-prone. Also you calls instanceof several times to resolve this ambiguity which is considered to be an anti-pattern as well.
You can solve these problems by introducing separate entity responsible for declaration generation:
public class DeclarationGenerator {
private final Map<Helper.Declaration, String[]> lhm = new LinkedHashMap<>();
public void add(Helper.Declaration key, String... value) {
lhm.put(key, value);
}
public String genDeclaration(Declaration... srcType) {
//...
for (Declaration st : Declaration.values()) {
if (input.containsKey(st))
result.append(st.getSrc(input.get(st)));
}
//...
}
}
with this helper class instead of
lhm.clear();
lhm.put(Helper.Declaration.IMPORT, null);
lhm.put(Helper.Declaration.CLASS, str[0]);
testCode += Helper.genDeclaration(lhm);
you can write
DeclarationGenerator dg = new DeclarationGenerator(); // not to reuse, create new one for each generation
dg.put(Helper.Declaration.IMPORT, null);
dg.put(Helper.Declaration.CLASS, str[0]);
testCode += dg.genDeclaration();
having clean design without field used as temporary variable and type ambiguity.
2. several TestCase enum entries, especially TESTBASECLSSTART and TESTBASECLS are copypasted in several places.
3. public static Map<String, String> testInput = new LinkedHashMap<>();
why this field in static?
-- Sergey
Also few minor comments:
1. Helper#getAnno()
StringBuffer result = new StringBuffer("");
using StringBuilder is recommended for thread-safe methods
2. List Helper.getMultipleAT()
this method uses List where List<String> can be specified
3. Helper.compareAnnoWithDiffSeq()
this can be done much shorted and without copying array to list:
String[] split1 = anno1.trim().split("\\s+");
String[] split2 = anno2.trim().split("\\s+");
Arrays.sort(split1);
Arrays.sort(split2);
return Arrays.equals(split1, split2);
4. Helper.compileCode():697-704 can be simplified using jdk8 streams:
ok = diagnostics.getDiagnostics().stream().anyMatch(d -> d.getKind() == Diagnostic.Kind.ERROR);
5. Same for Helper.isPkgInfoPresent:
return files.stream().map(JavaFileObject::getName).anyMatch(name -> name.contains("package-info") || name.contains("PkgAnno"));
6. AnnotatedArrayTypeGetAnnotatedGenericComponentTypeTest:175
You are calling Helper.getArrAT(at) then split it by ";" and work with result.
while Helper.getArrAT() takes a list and convert it into string:
return getAnno(annotations) + ";" + ret;
It may be more efficient to work directly with lists.
7. DeclarationGenerator interface is never used as interface
8. Helper.Declaration.replaceAnnoStr should be static
9. Helper.Declaration.replaceAnnoStr/getFirstAnno return result as String[] array, which is considered to be a bad practice (you are using variable length structure to store exactly 2 values). It would be cleaner to introduce intermediate structure of 2 string to store return value.
-- Sergey
16-09-2013
people's comments(8/9/13)
- AnnotationTest.i/clsIdx: it wasn't the name of the variable I was objecting to. It was the fact that you're relying on a mutable public static variable at all.
- GenTestCode should take Class<? extends TestCaseGenerator>, so you don't need to cast to TestCaseGenerator in GenTestCode. (Same comment for AnnotationTest.compileCode).
- AnnotationTest.testInput is a Map<Object,String> but I'm sure you can do better than Object for keys.'
- ExecutableGetAnnotatedParameterTypesTest.testInput hides AnnotationTest.testInput, but swaps the order of the type arguments! Map<String,Object> is either wrong, or should not use Object.
Alex
16-09-2013
people's comments(8/8/13)
- AnnotationTest line 49 is:
// index number for test class names
public static int i = 0;
That just doesn't smell right.,
- Still some blank lines before licenses.
- GenTestCode is now much much simpler! Could it be pulled up into AnnotationTest, then overridden in the various *Test classes (calling super() then doing additional work) where necessary?
- checkResult methods all have similar debugPrint calls which should be extracted into a Helper method. ("falty" is spelled wrong.)
Alex
16-09-2013
people's comments(8/3/13)
Hi Charlie,
This is much more understandable with the *.txt files, but still seems brittle. What would happen if you added a new String to annotationCombinations in AnnotationTest, or changed an existing String? Helper.Declaration is very precise about how many annotations are used.
Also:
- Some source files like TestCaseGenerator.java have text before the license comment, which I'm pretty sure is a no-no.
- The introductory comment in each *Test class refers to annotationCombinations, but it would help to say AnnotationTest.annotationCombinations.
- Sometimes the TestCase enum is private for no reason.
- Many checkResult methods have code which could be shared.
- The GenTestCode methods are strange. Each has to step through the TestCase values very carefully, and it's weird to see an enhanced-for loop with a counter variable (i). You should pass any values needed by TestCase.genTestCase to the constructor of the enum constant.
Alex
people's comments(6/20/13)
This is far too wrong, think about your code is pushing into openjdk, also compare two collection with better performance is easy to be implemented
=============================================================================================================
I don't think it's worthwhile to write an method of comparing algorithm to imporve 2 3-element arrays' comparison from O(n^2) to O(n log n).
But I'm willing to hear your opinions on other performance issues.
=============================================================================================================
The explanation is worse, that means you're testing unpredictable result
Perhaps the comment is not clear. It's not a negative test. If invoke
value() on an annotation without value, NoSuchMethodException is thrown.
But no exception for ones with value. In order to unify getting values on all kinds annotations, I put a catch phrase there.
=============================================================================================================
Since 7 we have the diamond elimination,
public static Map<Object, String> testInput = new LinkedHashMap<Object, String>(); // should be = new LinkedHashMap<>();
You could do much better when you do refactor and it has to be done
Tristan
16-09-2013
people's comments(6/20/13)
Ok, here is the 1st review
1. too many copy/paste, like getSrc, f.toString().replace("\\", "/") + "/") method
2. Many unnecessary cdoe
JavaFileObject o = null; // could be removed
try {
o = new JavaStringFileObject(name, code); // can be changed into JavaFileObject o... and return;
} catch (URISyntaxException e) {
throw new RuntimeException(e);
}
return o;
boolean ok = false;
JavaCompiler compiler = ToolProvider.getSystemJavaCompiler();
if (compiler == null) {
throw new RuntimeException("can't get javax.tools.JavaCompiler!");
}
JavaFileObject file = getFile(className, contents);
Iterable<? extends JavaFileObject> compilationUnit = Arrays.asList(file);
CompilationTask task = compiler.getTask(null, null, diagnostics, null,
null, compilationUnit);
ok = task.call(); // return directly
//why you just return ""
if (0 == annoStr.length) {
return "";
} else {
return "";
}
3. many performance issues, like compare two collecrion, you're using O(n^2) comparation
list1.containsAll(list2) && list2.containsAll(list1);
4. Put different test target into one test, like negative tests exist in to positive test
try {
Method method = annotation.annotationType().getMethod("value",
new Class[]{});
method.setAccessible(true);
value = method.invoke(annotation, new Object[]{});
} catch (NoSuchMethodException e) {
//expected exception for annotations without value
}
5. missed a lot in OOP, no capsulation, not using new Java syntax
Like public static Map<Object, String> testInput =
new LinkedHashMap<Object, String>();
6. Unclear test purpose
Basically I think you need refactor your code before you send out your code review request.
- Tristan
16-09-2013
people's comments(6/20/13)
- Please provide a text file for the generated code of each java/lang/* test.
- Two kinds of TestXYZGenerator is confusing. TestSourceGenerator should be called DeclarationGenerator. That's what it generates: declarations of classes, interfaces, fields, etc which are gathered up by the test case generators. Helper.SrcType should be called Helper.Declaration. The name of an enum type should describe what kind of things are represented by the enum constants, which here means declarations (of classes, interfaces, fields, etc). And you have a method called "genTestCode" in Helper as well as a method called "GenTestCode" in each test file (e.g. ClassGetAnnotatedInterfaceTest.java).
- It is extremely subtle that TYPE_ANNOTATION_1/2/3 represents both the declaration of an annotation type (to be part of a test case) _and_ an annotation (to be applied wherever #ANNO appears in the declaration under test). Please rename TYPE_ANNOTATION_1/2/3 to ANNOTATION_TYPE_1/2/3 (and, TYPE_ANNOTATION_CONTAINER_1/2/3 to CONTAINING_ANNOTATION_TYPE_1/2/3).
- There is too much duplication in the java/lang/* classes, and generally too much use of static fields. These classes should each be a subclass of an abstract class AnnotationTest, which declares test() and the bulk of the code in checkResult(). AnnotationTest should have a variable "annotationCombinations" (not "annoComb") to enumerate the annotations to be applied.
- Raw type Map in signature of Helper#genTestCode.
- ClassGetAnnotatedInterfaceTest -> missing an 's' after Interface. ClassGetAnnotatedSuperClassTest should be Superclass not SuperClass.
- @author should be a full name, not a first name.
Alex
16-09-2013
people's comments(6/13/13)
Thanks Charlie. I can see the combinatorial approach emerging, but the
abstraction level still needs to be raised to make the code
approachable. This can be achieved with more interfaces and less
duplication, as suggested below.
- Some of the files have their last line missing.
- Please check the jtreg guidelines for how to write @test, @bug, and
@author.
- PkgGetDeclaredAnnotations should end in Test. Also, don't abbreviate -
there is no value saying Cls rather than Class, Pkg rather than Package,
and Var rather than Variable, it just trips over the reader.
- Here's a bunch of stuff about organization. First, please separate the
infrastructure code into a different package than the Test classes.
Second, the Test classes themselves should be in named packages, not in
unnamed packages. Third, the directory test/java/lang/typeannoreflection
is really painful because there is no package
java.lang.typeannoreflection. Ideally you would have
test/type-annotations/java/lang/Class for the test classes which
exercise java.lang.Class, and
test/type-annotations/java/lang/reflect/Executable for the test classes
which exercise j.l.r.Executable methods, and so on.
- annoComb seems to be identical in every Test class. Why not centralize
it? Methods like compareAnnoWithDiffSeq and debugPrint are also
duplicated in various classes.
- The TestCase enum constants are all intended to implement a
getTestCase method, so please declare an interface for TestCase to
implement.
- Style: Taking this as an example: "static LinkedHashMap lhm = new
LinkedHashMap();" ... 1) It is not acceptable to use raw types in Java
code written in 2013, and 2) Declare variables using interfaces
(Map<...>) not classes (LinkedHashMap<..>) wherever possible.
Alex
16-09-2013
people's comments(6/4/13)
Hi Charlie,
First, I think there is some clever stuff going on here, but it's hard to figure out what it is. It's not enough to sprinkle comments like "// input should be like ["@A() @B()", "@C() @D()"...]" in SrcType. I would like an explanation of what a test case like ClsGetAnnotatedInterTest is trying to do. The TestCase enum type looks like a good place to start. Then, I would like to know how Helper.SrcType supports that with search-and-replace on keys like #ANNO. To be clear, I'm not looking for three bullet points; I'm looking for paragraphs that can be added to your code's javadoc and be useful to someone in ten years time.
Second, in the interest of increasing the abstraction of Helper.SrcType, you should introduce an interface with a getSrc method and have it be implemented by the SrcType enum type. You should also remove duplicated code throughout SrcType's enum constant bodies (e.g. the code for "// get @anno for [#ANNO1] ...").
Third, please don't shorten terms which are part of the Java language. METHODHEAD should be METHOD_HEADER. METHODPARAM should be METHOD_PARAMETER. Inter should be Interface. TYPEANNO1 should be TYPE_ANNOTATION_1. PKGANNOTATION1 should be PACKAGE_ANNOTATION_1. And so on. Rigor really matters when dealing with the Java language.
Alex
16-09-2013
people's comments(5/22/13)
I concur that using combinatorial tests is appropriate for this domain. The basic approach is to programatically generate both the expected result and the code to test and verify that the computed answer is consistent with the expected one. The regression tests currently in JDK 8 have code that allows source code to be generated at runtime, loading in by a class loader, and then run.
HTH,
-Joe
16-09-2013
People's comments:(5/22/13):
Hi Charlie,
I would suggest you do what I do when I get your tests, but start with that instead.
Clone http://hg.openjdk.java.net/jdk8/tl/jdk/
and add your tests to that.
As it is, I am updating my clone of your workspace, copying the tests into my clone of the tl/jdk workspace, then creating a webrev from there.
I was curious about the " 27 * @(#) GetAnnotatedInterfacesTest.java " too.
Are you using sccs (teamware) ?
-steve
16-09-2013
People's comments(5/22/13):
These tests contain a curious line in the test description, such as the following.
27 * @(#) GetAnnotatedInterfacesTest.java
Last I heard, we stopped using SCCS when we converted to using Mercurial back in 2007. I'm not sure where you're getting your template from but these lines can/should be deleted.
-- Jon
16-09-2013
People's comments(5/22/13):
Please keep one source file per tested API method, because it is easy to navigate. The problem is the body of the test() method in each source file. It should call utility methods to inspect annotations, rather that repeating more or less the same 'for' loops as every other test() method.
As for data providers, I believe they are a TestNG thing. Please add comments to each source file to describe what the data provider represents.
Finally, when you make a "little update", you should give a short description of what has changed. If you don't, someone who looked at the previous version has no idea what changed and will have to look at everything again.
Alex