refactor EclipseCollectionsCodeGenerator #1322
Conversation
|
I wasn't familiar with the ClassGraph library before. I like how using it makes it possible to delete so much code from FileUtils. To me, it seems like introducing ClassGraph is the essence of this change. In addition, there are more changes to production code and templates. However, the PR and commit message only mentions adding unit tests. |
sullis
changed the title
I have updated the PR title, PR description, and commit message. The description now contains a better explanation of my work. |
1) simplify EclipseCollectionsCodeGenerator constructor parameters 2) add unit test for EclipseCollectionsCodeGenerator 3) verify that all templates produce valid Java source code 4) verify the generated file count
sullis
force-pushed
the
codegen-unit-test
branch
from
e3a75d5
to
0c45428
Compare
|
I read the test and now understand why you have this. The test just attempts to generate everything. It executes in the code generator module, so we're not even up to compiling the hand-written files yet. |
| public void validateTestTemplates() throws Exception | ||
| { | ||
| validateTemplates("test", 2342); | ||
| } |
There was a problem hiding this comment.
I'd prefer not to assert an exact expected number of files, or people will be forced to change it all the time. Asserting it's not an empty collection would be fine IMO.
| private static void validateTemplates(String templateDirectory, int expectedFileCount) throws Exception | ||
| { | ||
| BasicErrorListener errorListener = new BasicErrorListener(); | ||
| File outputDirectory = Files.createTempDirectory("temp-").toFile(); |
There was a problem hiding this comment.
We can use JUnit's TemporaryFolder rule so that the files are deleted during tearDown of the test rather than the JVM.
| ParseResult<CompilationUnit> parseResult = parser.parse(code); | ||
| assertTrue(String.valueOf(parseResult.getProblems()) + "\n" + code, | ||
| parseResult.isSuccessful()); | ||
| } |
There was a problem hiding this comment.
I think it's kind of cool to have a test assertion like this that asserts a string is valid Java code. But I don't think we really need it either. Our builds generate code and compile it with javac from Java 8, so if this assertion were to fail our build would already be broken. Thoughts?
There are other assertions I could imagine that would be pretty interesting. Are there any places we declare skipBoolean = true but the generated source code matches the hand-written source code? So we don't need to skip boolean generation?
|
My overall impression is that I like Classgraph and how it helps reduce code. I'm not sure about the test because it's going to run slowly and doesn't seem like it will catch much that the build doesn't already catch. And then there are some refactorings that are only necessary to support the test. I'm curious if anyone else wants to weigh in @donraab @prathasirisha etc |
|
@motlin @sullis I think it makes sense to break this into separate PRs. The skipBoolean change looks straightforward and we should review that separately. If this means we are removing currently generated classes/interfaces then we'll have to think about this, but since 12.0 is a major release I don't have as many concerns about this. On the Classgraph front, looks like it should be ok, but would ask @nikhilnanivadekar if he thinks this will need Eclipse Foundation approval. It's a build time dependency only, and MIT license, so should be ok I think. |
This PR refactors EclipseCollectionsCodeGenerator and adds a new unit test (EclipseCollectionsCodeGeneratorTest).
EclipseCollectionsCodeGeneratorTest validates the generated Java source files for:
Classgraph library
This PR uses the classgraph library to load resources from the classpath.
When I started this PR, I tried casting the system class loader to URLClassLoader. This approach worked on Java 8 but did not work on later versions of Java.
Reference: https://stackoverflow.com/questions/54618721/migration-to-java11-classcastexception
skipBoolean flag
As I was writing the unit test, I learned that some of the STG templates do not produce valid Java code. I observed that "boolean" is not handled by the templates. I added the skipBoolean flag to these templates: