[MNG-8432] Integration test for MNG-8432 using mixins#12461
Conversation
gnodet
left a comment
There was a problem hiding this comment.
Review — Integration Test for MNG-8432
This PR adds an integration test that verifies mixin-based property and dependency management inheritance, targeting the MNG-8432 scenario. The test follows the established patterns in MavenITmng5102MixinsTest and MavenITmng11133MixinsPrecedenceTest.
Observations
🔵 Low — Partial overlap with existing mixin tests
MavenITmng5102MixinsTest already covers mixin property inheritance. However, this test specifically targets the MNG-8432 scenario (BOM-style mixin with both properties and <dependencyManagement> via GAV resolution), which is a valid regression test for the reported issue.
🔵 Low — String matching on effective POM
The test uses Files.readString() + String.contains() on the effective POM XML. The Maven IT convention typically uses maven-it-plugin-expression with verifier.loadProperties(), which is more robust against whitespace/formatting changes. That said, MavenITmng11133MixinsPrecedenceTest uses the same Files.readString + contains approach, so this is an established alternative pattern.
Summary
Clean integration test that follows existing conventions. The test correctly installs the mixin BOM, then verifies that the consumer project's effective POM includes both the inherited property and the managed dependency. No blocking issues found. ✅
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Guillaume Nodet
New MNG-8432 integration test PR. Verifier caught 8/10 false positives from reviewer (outdated API assumptions). 2 LOW observations posted. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace help:effective-pom + Files.readString/String.contains with the conventional maven-it-plugin-expression approach using verifier.loadProperties(), which is more robust against whitespace/formatting changes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Proposed solution for MNG-8432The use case described in MNG-8432 — inheriting both Instead of extending BOM <project xmlns="http://maven.apache.org/POM/4.2.0" root="true" ...>
<modelVersion>4.2.0</modelVersion>
...
<mixins>
<mixin>
<groupId>com.example</groupId>
<artifactId>my-bom</artifactId>
<version>1.0</version>
</mixin>
</mixins>
</project>This PR adds an integration test verifying this behavior — that the consumer project successfully inherits both properties and managed dependencies from the mixin BOM. |
gnodet
left a comment
There was a problem hiding this comment.
AI Re-Review — PR #12461 (new commit)
Verdict: ✅ APPROVE
Summary: The new commit (bf5ac5c — "Use maven-it-plugin-expression instead of effective POM string matching") directly addresses the string-matching observation from the previous review. It replaces the help:effective-pom + Files.readString() + String.contains() approach with the idiomatic maven-it-plugin-expression + verifier.loadProperties() + assertEquals() pattern used by 193+ other integration tests in the suite.
What Changed
- Before: Ran
help:effective-pom, read the output file, and usedString.contains()to verify mixin properties and dependencies. - After: Uses
maven-it-plugin-expressionplugin atvalidatephase to evaluateproject/propertiesandproject/dependencyManagementintotarget/model.properties, then asserts withassertEquals().
Assessment
- ✅ Follows the established
core-it-suiteconvention (confirmed across 193 test files) - ✅ Pattern matches existing tests like
MavenITmng5102MixinsTestandMavenITmng5600DependencyManagementImportExclusionsTest - ✅ JUnit
assertEqualsconsistent with all existing Maven ITs - ✅ Responsive to prior review feedback — commit title explicitly signals what changed
- ⏳ CI checks still pending (just pushed today) — expected to pass given the well-structured test
No findings. Clean, targeted improvement.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Guillaume Nodet
Re-review of apache#12461 — new commit addressed prior string matching feedback. APPROVED. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This PR adds an integration test to demonstrate and document the correct usage of the
<mixins>feature (introduced in Maven 4.2.0) as the native and architecturally preferred solution to MNG-8432.As discussed in the original PR #12417 (which proposed extending
<dependency>BOM imports to include properties), inheriting both dependency management and properties from a shared BOM-like module is a highly requested use case (e.g. for Spring Boot, Quarkus). However, extendingdependencyManagementimports to pull properties introduces significant risks around property pollution, silent semantic drift, and ordering fragility.Instead, as @gnodet correctly pointed out, Mixins are the supported mechanism for this use case. Following his suggestion, this integration test (
MavenITmng8432MixinsPropertiesTest) explicitly verifies that when a project uses a<mixin>, it successfully inherits both:dependencyManagement(verified by the consumer being able to resolve the managed dependency)properties(verified by checking theeffective-pomfor the inherited property)This serves as both a regression test for the mixin functionality and as executable documentation for users encountering the MNG-8432 use case.