created simple test invoking Java compiler and running annotation#2244
Conversation
WalkthroughAdds in-memory Java compilation test infrastructure and tests for the Spring XSD annotation processor; updates generated parser/header imports and K8s helper imports; renames MainInfo accessor to getTopLevels and updates processor validations to track top-level MCElements and check top-level/name clashes; moves AbstractParser package and replaces Guava Sets usage. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Test as Test Suite
participant Helper as CompilerHelper
participant Compiler as Java Compiler
participant FM as CustomJavaFileManager
participant Proc as SpringConfigurationXSD<br/>GeneratingAnnotationProcessor
Test->>Helper: prepare in-memory Java sources
Helper->>Compiler: invoke compilation task with Proc, using FM
Compiler->>Proc: run annotation processor on sources
Proc->>FM: request output file (getJavaFileForOutput)
FM-->>Proc: provide InMemoryJavaFileObject (in-memory)
Proc->>FM: write generated artifacts (XSD/beans) into in-memory objects
Compiler-->>Helper: return CompilerResult (success + diagnostics)
Helper-->>Test: deliver result for assertions
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
annot/src/main/java/com/predic8/membrane/annot/generator/Parsers.java (2)
45-45: Keep generated headers consistent across generator outputs.You updated the header here to 2012-2025. The header in writeParsers(...) still says 2012 only; align both for consistency.
Apply within writeParsers(...) copyright literal:
- /* Copyright 2012 predic8 GmbH, www.predic8.com + /* Copyright 2012-2025 predic8 GmbH, www.predic8.com
33-33: Minor: method name typo.Consider renaming writeParserDefinitior → writeParserDefinition for clarity.
annot/src/test/java/com/predic8/membrane/annot/util/CompilerResult.java (1)
19-22: LGTM — compact result container.Record looks good. If helpful, add a convenience accessor for error diagnostics later.
annot/src/test/java/com/predic8/membrane/annot/SpringConfigurationXSDGeneratingAnnotationProcessorTest.java (1)
58-60: Make assertions resilient and more informative.Assert no ERROR diagnostics (ignore warnings), and include diagnostics in failure messages.
- assertEquals(0, result.diagnostics().getDiagnostics().size()); - assertTrue(result.compilationSuccess()); + long errorCount = result.diagnostics().getDiagnostics().stream() + .filter(d -> d.getKind() == javax.tools.Diagnostic.Kind.ERROR) + .count(); + assertEquals(0, errorCount, () -> "Diagnostics: " + result.diagnostics().getDiagnostics()); + assertTrue(result.compilationSuccess(), () -> "Compilation failed. Diagnostics: " + result.diagnostics().getDiagnostics());annot/src/test/java/com/predic8/membrane/annot/util/CustomJavaFileManager.java (2)
168-193: Implement read paths for in-memory objects.InnerFileObject lacks openInputStream/openReader; some compiler paths may call them. Implement both to avoid UnsupportedOperationException.
@Override public CharSequence getCharContent(boolean ignoreEncodingErrors) throws IOException { byte[] bytes = content.get(toUri()); if (bytes == null) return ""; return new String(bytes, UTF_8); } @Override public OutputStream openOutputStream() throws IOException { return new ByteArrayOutputStream() { @Override public void flush() throws IOException { super.flush(); content.put(toUri(), toByteArray()); log.debug("wrote " + toUri() + " : " + new String(toByteArray(), UTF_8)); } @Override public void close() throws IOException { super.close(); content.put(toUri(), toByteArray()); log.debug("wrote " + toUri() + " : " + new String(toByteArray(), UTF_8)); } }; } + + @Override + public InputStream openInputStream() throws IOException { + byte[] bytes = content.get(toUri()); + return new ByteArrayInputStream(bytes != null ? bytes : new byte[0]); + } + + @Override + public Reader openReader(boolean ignoreEncodingErrors) throws IOException { + return new StringReader(getCharContent(ignoreEncodingErrors).toString()); + }
44-44: Optional: make in-memory mode configurable.Consider toggling USE_IN_MEM via a system property (e.g., -Dannot.fm.inmem=true) to debug on-disk outputs when needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
annot/src/main/java/com/predic8/membrane/annot/generator/Parsers.java(1 hunks)annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/K8sHelperGenerator.java(0 hunks)annot/src/test/java/com/predic8/membrane/annot/SpringConfigurationXSDGeneratingAnnotationProcessorTest.java(1 hunks)annot/src/test/java/com/predic8/membrane/annot/util/CompilerHelper.java(1 hunks)annot/src/test/java/com/predic8/membrane/annot/util/CompilerResult.java(1 hunks)annot/src/test/java/com/predic8/membrane/annot/util/CustomJavaFileManager.java(1 hunks)annot/src/test/java/com/predic8/membrane/annot/util/InMemoryJavaFile.java(1 hunks)
💤 Files with no reviewable changes (1)
- annot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/K8sHelperGenerator.java
🧰 Additional context used
🧬 Code graph analysis (1)
annot/src/test/java/com/predic8/membrane/annot/SpringConfigurationXSDGeneratingAnnotationProcessorTest.java (3)
annot/src/test/java/com/predic8/membrane/annot/util/CompilerHelper.java (1)
CompilerHelper(19-40)annot/src/test/java/com/predic8/membrane/annot/util/CustomJavaFileManager.java (1)
CustomJavaFileManager(37-396)annot/src/test/java/com/predic8/membrane/annot/util/InMemoryJavaFile.java (1)
InMemoryJavaFile(19-32)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Automated tests
- GitHub Check: Analyze (java)
🔇 Additional comments (1)
annot/src/test/java/com/predic8/membrane/annot/util/InMemoryJavaFile.java (1)
19-31: LGTM — straightforward in-memory source.Works as expected.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
annot/src/test/java/com/predic8/membrane/annot/util/CompilerHelper.java (1)
23-45: Resource leak and noisy diagnostics remain unaddressed.The
compilemethod has two issues previously flagged:
- Resource leak (line 29):
CustomJavaFileManageris created but never closed, which can leak file handles.- Noisy output (line 42): Diagnostics are printed to stderr even when compilation succeeds, cluttering test output.
Apply this diff to fix both issues:
public static CompilerResult compile(Iterable<? extends JavaFileObject> sources) { JavaCompiler compiler = ToolProvider.getSystemJavaCompiler(); if (compiler == null) { throw new IllegalStateException("No system Java compiler found. Run tests with a JDK, not a JRE."); } DiagnosticCollector<JavaFileObject> diagnostics = new DiagnosticCollector<>(); - JavaFileManager fileManager = new CustomJavaFileManager(compiler.getStandardFileManager(diagnostics, null, null)); - - JavaCompiler.CompilationTask task = compiler.getTask( - null, - fileManager, - diagnostics, - List.of("-processor", "com.predic8.membrane.annot.SpringConfigurationXSDGeneratingAnnotationProcessor"), - null, - sources - ); - - boolean success = task.call(); - - diagnostics.getDiagnostics().forEach(System.err::println); - - return new CompilerResult(success, diagnostics); + try (JavaFileManager fileManager = + new CustomJavaFileManager(compiler.getStandardFileManager(diagnostics, null, null))) { + JavaCompiler.CompilationTask task = compiler.getTask( + null, + fileManager, + diagnostics, + List.of("-processor", "com.predic8.membrane.annot.SpringConfigurationXSDGeneratingAnnotationProcessor"), + null, + sources + ); + boolean success = task.call(); + if (!success) { + diagnostics.getDiagnostics().forEach(System.err::println); + } + return new CompilerResult(success, diagnostics); + } catch (java.io.IOException e) { + throw new RuntimeException("Failed to close JavaFileManager", e); + } }
🧹 Nitpick comments (2)
annot/src/test/java/com/predic8/membrane/annot/SpringConfigurationXSDGeneratingAnnotationProcessorTest.java (1)
20-24: Remove unused imports.Lines 20-24 import
JavaFileObject,ArrayList,List,Collectors, andStream, but none of these are directly used in the test class. OnlysplitSources(statically imported at line 26) and assertion methods are needed.Apply this diff to clean up unused imports:
-import javax.tools.*; -import java.util.ArrayList; -import java.util.List; -import java.util.stream.Collectors; -import java.util.stream.Stream;annot/src/test/java/com/predic8/membrane/annot/util/CompilerHelper.java (1)
53-60: Remove debug println statements.Lines 56-58 print package, class, and source to stdout for every file processed, polluting test output. Remove these debug statements or replace them with proper logging at DEBUG level if needed for troubleshooting.
Apply this diff to remove the debug output:
private static JavaFileObject toInMemoryJavaFile(String source) { String pkg = extractPackage(source); String cls = extractName(source); - System.out.println("PACKAGE: " + pkg); - System.out.println("CLASS: " + cls); - System.out.println("SOURCE: " + source); return new InMemoryJavaFile(pkg + "." + cls, source); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
annot/src/test/java/com/predic8/membrane/annot/SpringConfigurationXSDGeneratingAnnotationProcessorTest.java(1 hunks)annot/src/test/java/com/predic8/membrane/annot/util/CompilerHelper.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
annot/src/test/java/com/predic8/membrane/annot/SpringConfigurationXSDGeneratingAnnotationProcessorTest.java (2)
annot/src/test/java/com/predic8/membrane/annot/util/CompilerHelper.java (1)
CompilerHelper(22-77)annot/src/test/java/com/predic8/membrane/annot/util/InMemoryJavaFile.java (1)
InMemoryJavaFile(19-32)
🔇 Additional comments (3)
annot/src/test/java/com/predic8/membrane/annot/SpringConfigurationXSDGeneratingAnnotationProcessorTest.java (1)
32-57: LGTM! Test correctly verifies annotation processor compilation.The test creates two in-memory sources (Demo with @mcmain annotation and NamespaceHandler), compiles them with the annotation processor, and asserts successful compilation with no diagnostics. The use of
splitSourcescorrectly handles package/class name extraction, resolving the previous issue about package mismatches.annot/src/test/java/com/predic8/membrane/annot/util/CompilerHelper.java (2)
47-51: LGTM! Clean split and conversion logic.The method correctly splits multi-source strings by "---" delimiter and converts each segment into an in-memory Java file object.
62-75: LGTM! Regex extraction logic is appropriate for test utilities.Both
extractNameandextractPackageuse straightforward regex patterns with clear error messages. While they don't handle all edge cases (e.g., annotations on class declarations), they're sufficient for the controlled test sources in this codebase.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
annot/src/main/java/com/predic8/membrane/annot/AbstractParser.java (1)
18-18: LGTM! Good refactor to remove Guava dependency.Replacing Guava's
Setswith standard libraryHashSetandList.ofis a good refactor that reduces external dependencies and uses more idiomatic Java.Also applies to: 35-35
annot/src/main/java/com/predic8/membrane/annot/model/MainInfo.java (1)
70-72: Consider renaming the fieldglobalstotopLevelsfor consistency.The method rename from
getGlobals()togetTopLevels()improves clarity, but the underlying field is still namedglobals(line 36). For consistency and to avoid confusion, consider renaming the field to match the accessor name.Apply this diff to rename the field:
- private Map<String, ElementInfo> globals = new HashMap<>(); + private Map<String, ElementInfo> topLevels = new HashMap<>();public Map<String, ElementInfo> getTopLevels() { - return globals; + return topLevels; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
annot/pom.xml(1 hunks)annot/src/main/java/com/predic8/membrane/annot/AbstractParser.java(3 hunks)annot/src/main/java/com/predic8/membrane/annot/SpringConfigurationXSDGeneratingAnnotationProcessor.java(2 hunks)annot/src/main/java/com/predic8/membrane/annot/generator/Parsers.java(2 hunks)annot/src/main/java/com/predic8/membrane/annot/model/MainInfo.java(1 hunks)annot/src/test/java/com/predic8/membrane/annot/SpringConfigXSDErrorsTest.java(1 hunks)annot/src/test/java/com/predic8/membrane/annot/SpringConfigurationXSDGeneratingAnnotationProcessorTest.java(1 hunks)annot/src/test/java/com/predic8/membrane/annot/util/CompilerHelper.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- annot/src/test/java/com/predic8/membrane/annot/SpringConfigurationXSDGeneratingAnnotationProcessorTest.java
- annot/src/main/java/com/predic8/membrane/annot/generator/Parsers.java
- annot/src/test/java/com/predic8/membrane/annot/util/CompilerHelper.java
🧰 Additional context used
🧬 Code graph analysis (1)
annot/src/test/java/com/predic8/membrane/annot/SpringConfigXSDErrorsTest.java (2)
annot/src/test/java/com/predic8/membrane/annot/util/CompilerHelper.java (1)
CompilerHelper(30-124)annot/src/test/java/com/predic8/membrane/annot/SpringConfigurationXSDGeneratingAnnotationProcessorTest.java (1)
SpringConfigurationXSDGeneratingAnnotationProcessorTest(23-54)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Automated tests
- GitHub Check: Analyze (java)
🔇 Additional comments (10)
annot/src/test/java/com/predic8/membrane/annot/SpringConfigXSDErrorsTest.java (5)
14-25: LGTM! Clean imports and static imports.The imports are well-organized and necessary for the tests. The static import of
MC_MAIN_DEMOfromSpringConfigurationXSDGeneratingAnnotationProcessorTestpromotes reusability of test fixtures.
27-45: LGTM! Correct validation of missing @mcmain annotation.The test correctly verifies that when
@MCMainis absent, the annotation processor issues a warning while still allowing compilation to succeed. The assertion properly checks forcompilationSuccess = truewith the expected warning message.
47-61: LGTM! Proper validation of required annotation attribute.The test correctly verifies that
@MCElementwithout the requirednameattribute causes compilation to fail with an appropriate error message about the missing annotation element.
63-84: LGTM! Thorough validation of duplicate element ID detection.The test correctly validates that when two
@MCElementannotations share the same name (and thus the same derived ID), the annotation processor detects the conflict and issues two error messages: one with the detailed error and another marking the duplicate location.
86-107: LGTM! Validates duplicate top-level element name detection.The test properly verifies that when two
@MCElementannotations have the samenameattribute but differentidattributes, the annotation processor detects the duplicate top-level name and provides a helpful error message suggesting the use oftopLevel=falseto resolve the conflict.annot/pom.xml (1)
56-61: ---No changes needed for the Hamcrest version format.
The latest stable Hamcrest for Java is version 3.0, and the version specified in the pom.xml is correct. Consider defining the Hamcrest version in the parent POM's dependency management section for consistency across modules if you centralize dependency versions.
Likely an incorrect or invalid review comment.
annot/src/main/java/com/predic8/membrane/annot/AbstractParser.java (2)
53-53: LGTM! Correct refactor to standard library.The change from
Sets.newHashSet(parserContext.getRegistry().getBeanDefinitionNames())tonew HashSet<>(of(parserContext.getRegistry().getBeanDefinitionNames()))is functionally equivalent and uses standard library idioms.
15-15: All references to AbstractParser have been successfully updated—no action required.Verification confirms the package relocation is complete and correct:
- AbstractParser class is properly defined in its new location at
com.predic8.membrane.annot.AbstractParser- The generator code in Parsers.java line 128 correctly imports from the new package:
import com.predic8.membrane.annot.AbstractParser- Generated parser classes properly extend from AbstractParser in the new location (line 139)
- No references to the old package path for AbstractParser remain in the codebase
The refactoring is complete with no breaking changes or missing updates.
annot/src/main/java/com/predic8/membrane/annot/SpringConfigurationXSDGeneratingAnnotationProcessor.java (2)
218-219: LGTM! Top-level element tracking is correctly implemented.The logic to populate
main.getTopLevels()whentopLevel = trueis straightforward and correctly placed during the element scanning phase. The duplicate validation is appropriately handled later in the processing flow.
271-281: Duplicate top-level validation logic looks correct.The validation correctly detects when two different elements both have
topLevel=trueand the same@MCElementname. The error message appropriately suggests making at least one@MCElement(topLevel=false,...).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
annot/src/main/java/com/predic8/membrane/annot/SpringConfigurationXSDGeneratingAnnotationProcessor.java (1)
271-280: Consider adding a clarifying comment for the duplicate detection logic.The duplicate detection works correctly but relies on map overwriting behavior (line 219) where only the last duplicate remains in
getTopLevels(). This check then catches all earlier duplicates by comparing them against the last one. While functional, this approach is non-obvious to maintainers.Consider adding a comment like:
// Check for duplicate top-level element names. // Note: getTopLevels() contains only the last element with each name // due to map overwriting at line 219. This loop catches all earlier // duplicates by comparing them against the last one in the map.Additionally, the error message could be more specific by including the duplicate name:
-throw new ProcessingException("Duplicate top-level @MCElement name. Make at least one @MCElement(topLevel=false,...) .", f.getKey(), ei2.getElement()); +throw new ProcessingException("Duplicate top-level @MCElement name '" + f.getKey().getAnnotation(MCElement.class).name() + "'. Make at least one @MCElement(topLevel=false,...) .", f.getKey(), ei2.getElement());Also consider a defensive null check at line 273:
MCElement annotation = f.getKey().getAnnotation(MCElement.class); if (annotation != null) { ElementInfo ei2 = main.getTopLevels().get(annotation.name()); // ... rest of logic }annot/src/test/java/com/predic8/membrane/annot/SpringConfigurationXSDGeneratingAnnotationProcessorTest.java (1)
25-44: Consider extracting shared test constant to a utility class.The
MC_MAIN_DEMOconstant is reused inSpringConfigXSDErrorsTest, creating a dependency between test classes. While this works, consider moving shared test constants to a dedicated test utility class (e.g.,TestConstantsorTestFixtures) for better organization and maintainability.For example:
// In a new class: annot/src/test/java/com/predic8/membrane/annot/util/TestConstants.java public class TestConstants { public static final String MC_MAIN_DEMO = """ package com.predic8.membrane.demo; ... """; }Then both test classes can import from this shared location.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
annot/src/main/java/com/predic8/membrane/annot/SpringConfigurationXSDGeneratingAnnotationProcessor.java(2 hunks)annot/src/test/java/com/predic8/membrane/annot/SpringConfigXSDErrorsTest.java(1 hunks)annot/src/test/java/com/predic8/membrane/annot/SpringConfigurationXSDGeneratingAnnotationProcessorTest.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
annot/src/test/java/com/predic8/membrane/annot/SpringConfigurationXSDGeneratingAnnotationProcessorTest.java (1)
annot/src/test/java/com/predic8/membrane/annot/util/CompilerHelper.java (1)
CompilerHelper(30-124)
annot/src/test/java/com/predic8/membrane/annot/SpringConfigXSDErrorsTest.java (2)
annot/src/test/java/com/predic8/membrane/annot/util/CompilerHelper.java (1)
CompilerHelper(30-124)annot/src/test/java/com/predic8/membrane/annot/SpringConfigurationXSDGeneratingAnnotationProcessorTest.java (1)
SpringConfigurationXSDGeneratingAnnotationProcessorTest(23-60)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Automated tests
- GitHub Check: Analyze (java)
🔇 Additional comments (4)
annot/src/main/java/com/predic8/membrane/annot/SpringConfigurationXSDGeneratingAnnotationProcessor.java (2)
218-220: LGTM: Top-level element tracking.The code correctly tracks top-level elements by name. Note that
Map.put()will overwrite previous entries with the same name, which is intentional - the subsequent validation at lines 273-275 detects these duplicates.
281-284: LGTM: Emptiness validation correctly placed.The check ensures at least one
@MCElementis present when@MCMainis declared. The placement after processing all elements is appropriate.annot/src/test/java/com/predic8/membrane/annot/SpringConfigurationXSDGeneratingAnnotationProcessorTest.java (1)
46-57: LGTM: Well-structured basic test.The test correctly validates that the annotation processor can compile a simple
@MCElementconfiguration. The use ofsplitSources()and in-memory compilation provides good test isolation.annot/src/test/java/com/predic8/membrane/annot/SpringConfigXSDErrorsTest.java (1)
1-315: LGTM: Comprehensive error-path test coverage.This test file provides excellent coverage of error scenarios for the annotation processor:
- Missing annotation cases (MCMain, MCElement, MCTextContent)
- Duplicate detection (element IDs and names)
- Constraint violations (
noEnvelopewith various incompatible settings)- TextContent and mixed-content requirements
The test structure with nested classes and consistent assertion patterns makes the test suite maintainable and easy to understand. All expected error messages match the processor implementation.
2 participants
processor
Summary by CodeRabbit
Tests
Bug Fixes / Validation
Chores
Refactor