gnodet commented on code in PR #11829:
URL: https://github.com/apache/maven/pull/11829#discussion_r2983544994
##########
impl/maven-testing/src/main/java/org/apache/maven/api/plugin/testing/MojoExtension.java:
##########
@@ -437,8 +439,8 @@ public void beforeEach(ExtensionContext context) throws
Exception {
model = new MavenMerger().merge(tmodel, defaultModel, false, null);
}
tmodel = new DefaultModelPathTranslator(new DefaultPathTranslator())
- .alignToBaseDirectory(tmodel, Paths.get(getBasedir()), null);
- context.getStore(ExtensionContext.Namespace.GLOBAL).put(Model.class,
tmodel);
+ .alignToBaseDirectory(model, Paths.get(getBasedir()), null);
Review Comment:
**Undocumented behavior change: `tmodel` to `model`.**
The original passes `tmodel` (the raw parsed POM) to `alignToBaseDirectory`,
while this PR passes `model` (the merged model). This is arguably an
improvement — it fixes a potential NPE when `tmodel` is null, and aligning the
merged model makes more sense. But it's a behavioral change mixed into a
refactoring PR without being called out.
Consider either:
- Documenting this in the PR description as an intentional fix, or
- Splitting it into a separate commit
_Claude Code on behalf of Guillaume Nodet_
##########
impl/maven-testing/src/main/java/org/apache/maven/api/di/testing/MavenDIExtension.java:
##########
@@ -76,18 +78,21 @@ public void beforeEach(ExtensionContext context) throws
Exception {
*
* @param context The extension context to store
*/
- protected void setContext(ExtensionContext context) {
- MavenDIExtension.context = context;
+ protected static void setContext(ExtensionContext context) {
+ EXTENSION_CONTEXT_THREAD_LOCAL.set(context);
}
+ protected static Optional<ExtensionContext> getContext() {
Review Comment:
Nit: missing blank line before the Javadoc block (style consistency with the
rest of the file).
_Claude Code on behalf of Guillaume Nodet_
##########
impl/maven-testing/src/main/java/org/apache/maven/api/plugin/testing/MojoExtension.java:
##########
@@ -331,23 +333,23 @@ private static String
getGoalFromMojoImplementationClass(Class<?> cl) throws IOE
@Override
@SuppressWarnings("checkstyle:MethodLength")
public void beforeEach(ExtensionContext context) throws Exception {
- if (pluginBasedir == null) {
- pluginBasedir = MavenDIExtension.getBasedir();
- }
- basedir =
AnnotationSupport.findAnnotation(context.getElement().orElseThrow(),
Basedir.class)
+ setContext(context);
+
+ String pluginBasedir = MavenDIExtension.getBasedir();
+
+ String basedir =
AnnotationSupport.findAnnotation(context.getElement().orElseThrow(),
Basedir.class)
.map(Basedir::value)
- .orElse(pluginBasedir);
- if (basedir != null) {
- if (basedir.isEmpty()) {
- basedir = pluginBasedir + "/target/tests/"
- + context.getRequiredTestClass().getSimpleName() + "/"
- + context.getRequiredTestMethod().getName();
- } else {
- basedir = basedir.replace("${basedir}", pluginBasedir);
- }
+ .orElse(null);
Review Comment:
**Bug: behavioral change when `@Basedir` is absent.**
The original code uses `.orElse(pluginBasedir)`, so when `@Basedir` is not
present, `basedir = pluginBasedir` (a real, non-empty path). Then the `if
(basedir.isEmpty())` check is false, so `basedir` stays as `pluginBasedir`.
With this change (`.orElse(null)`), when `@Basedir` is absent, `basedir ==
null` enters the `if` branch and `basedir = pluginBasedir +
"/target/tests/ClassName/methodName"` — a completely different directory.
This contradicts the `@Basedir` Javadoc: _"If not specified, the plugin's
base directory will be used as the default."_ and will break any plugin test
that doesn't use `@Basedir` but expects `getBasedir()` to return the plugin
root.
Suggested fix — restore original semantics:
```java
.orElse(pluginBasedir);
if (basedir.isEmpty()) {
basedir = pluginBasedir + "/target/tests/"
+ context.getRequiredTestClass().getSimpleName() + "/"
+ context.getRequiredTestMethod().getName();
} else {
basedir = basedir.replace("${basedir}", pluginBasedir);
}
```
_Claude Code on behalf of Guillaume Nodet_
##########
impl/maven-testing/src/main/java/org/apache/maven/api/di/testing/MavenDIExtension.java:
##########
@@ -76,18 +78,21 @@ public void beforeEach(ExtensionContext context) throws
Exception {
*
Review Comment:
Note: this changes the method signature from `protected` instance to
`protected static`. This is a public API change — any downstream class
overriding `setContext` would break at compile time. In this codebase only
`MojoExtension` extends `MavenDIExtension` (and doesn't override it), but
`maven-testing` is a public artifact used by plugin developers.
_Claude Code on behalf of Guillaume Nodet_
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
