Skip to content

Refactor MNG-11133 test to use maven-it-plugin-expression#12502

Open
gnodet wants to merge 1 commit into
apache:masterfrom
gnodet:refactor-mavenitmng11133mixinsprecedencetest-to-us
Open

Refactor MNG-11133 test to use maven-it-plugin-expression#12502
gnodet wants to merge 1 commit into
apache:masterfrom
gnodet:refactor-mavenitmng11133mixinsprecedencetest-to-us

Conversation

@gnodet

@gnodet gnodet commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Replace help:effective-pom + Files.readString() + String.contains() with the conventional maven-it-plugin-expression approach using verifier.loadProperties() in MavenITmng11133MixinsPrecedenceTest
  • This is the same pattern applied in PR [MNG-8432] Integration test for MNG-8432 using mixins #12461 for MavenITmng8432MixinsPropertiesTest — more robust against whitespace/formatting changes in effective POM XML
  • Added the expression plugin to the test resource pom.xml to evaluate project/properties at the validate phase

Test plan

  • MavenITmng11133MixinsPrecedenceTest passes locally (1 test, 0 failures)
  • CI build passes

🤖 Generated with Claude Code

…ng in MNG-11133 test

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>
@gnodet
gnodet marked this pull request as ready for review July 17, 2026 10:00

@gnodet gnodet left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI Review — PR #12502

Verdict: ✅ LGTM (own PR, posting as COMMENT)

Clean refactoring from help:effective-pom string matching to the conventional maven-it-plugin-expression + loadProperties() + assertEquals() approach. Same pattern used by 30+ tests in core-it-suite and applied in PR #12461 for MNG-8432.

What I Appreciate

  • Stronger assertionassertEquals("21", ...) checks the exact resolved property value rather than substring-matching raw XML, making the test both more precise and more resilient to formatting changes.
  • Self-documenting failure message"Property from mixin should override the parent's value" immediately tells a developer what went wrong.
  • Clean import hygiene — removed Files and assertTrue (no longer needed), added Properties and assertEquals, no dead imports.

Findings

None — textbook refactoring. Same behavior, better implementation. ✨


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

gnodet added a commit to gnodet/maven that referenced this pull request Jul 17, 2026
Review posted for apache#12502 (MNG-11133 test refactoring). Own PR, LGTM.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant