slawekjaranowski commented on code in PR #1024:
URL: https://github.com/apache/maven-enforcer/pull/1024#discussion_r4009545186
##########
enforcer-rules/src/test/java/org/apache/maven/enforcer/rules/TestRequireSameVersions.java:
##########
@@ -262,6 +303,51 @@ private static String extractGaString(Artifact dependency)
{
return String.format("%s:%s", dependency.getGroupId(),
dependency.getArtifactId());
}
+ @Test
+ void testReportPluginUsesBuildPluginVersionBeforeManagedVersion() throws
IOException {
Review Comment:
This test sits after the private helpers `constructArtifact` /
`extractGaString`, which breaks the file layout (tests first, helpers at the
bottom). Please move it up next to the other tests.
##########
enforcer-rules/src/main/java/org/apache/maven/enforcer/rules/RequireSameVersions.java:
##########
@@ -136,6 +140,66 @@ private Map<String, List<String>> collectVersionMembers(
return versionMembers;
}
+ private Map<String, List<String>> collectReportVersionMembers(
+ Set<Artifact> artifacts, Collection<String> patterns, String
source) {
+ Map<String, List<String>> versionMembers = new LinkedHashMap<>();
+
+ List<Pattern> regExs = new ArrayList<>();
+ for (String pattern : patterns) {
+ String regex = pattern.replace(".", "\\.")
+ .replace("*", ".*")
+ .replace(":", "\\:")
+ .replace('?', '.');
+
+ regExs.add(Pattern.compile(regex + "(\\:.+)?"));
+ }
+
+ for (Artifact artifact : artifacts) {
+ for (Pattern regEx : regExs) {
+ if
(regEx.matcher(artifact.getDependencyConflictId()).matches()) {
+ String version = getReportPluginVersion(artifact);
+ versionMembers
+ .computeIfAbsent(version, unused -> new
ArrayList<>())
+ .add(artifact.getDependencyConflictId() + source);
+ }
+ }
+ }
+ return versionMembers;
+ }
+
+ private String getReportPluginVersion(Artifact artifact) {
Review Comment:
Worth considering a simpler shape here. Both `getReportArtifacts()` and
`getReportPlugins()` are `@Deprecated` in `MavenProject`, and this adds a
second use of that API. Since the reporting artifacts carry no usable version
anyway, the entries could be built straight from the model
(`project.getModel().getReporting()`), where `ReportPlugin` already has
groupId/artifactId/version - that removes the artifact to model lookup
entirely. The only thing to reconstruct would be
`groupId:artifactId:maven-plugin` for the pattern matching and the message. Not
a blocker, but it would make this special case considerably smaller.
##########
enforcer-rules/src/main/java/org/apache/maven/enforcer/rules/RequireSameVersions.java:
##########
@@ -136,6 +140,66 @@ private Map<String, List<String>> collectVersionMembers(
return versionMembers;
}
+ private Map<String, List<String>> collectReportVersionMembers(
Review Comment:
This is a copy of `collectVersionMembers` that differs in exactly one line
(the version lookup at 160), and the copy also dropped the `// pattern is
groupId[:artifactId[:type[:classifier]]]` comment. Please keep a single method
and parameterise the version lookup instead, e.g.
```java
private Map<String, List<String>> collectVersionMembers(
Set<Artifact> artifacts, Collection<String> patterns, String source,
Function<Artifact, String> versionResolver) {
```
with the two existing call sites passing the current expression and the
reporting one passing `this::getReportPluginVersion`.
##########
enforcer-rules/src/test/java/org/apache/maven/enforcer/rules/TestRequireSameVersions.java:
##########
@@ -254,6 +258,43 @@ void
testProjectWithDifferentDependencyVersionsInBuildAndReportAndPlugins() thro
assertThatCode(rule::execute).isInstanceOf(EnforcerRuleException.class);
}
+ @Test
+ void testReportPluginUsesManagedVersionWhenArtifactVersionIsRelease()
throws IOException {
Review Comment:
Both new tests only assert `doesNotThrowAnyException()`. Could you add a
negative one - a genuine mismatch coming through `pluginManagement` (e.g.
managed `maven-surefire-report-plugin` `3.6.0` against build
`maven-surefire-plugin` `3.5.3`) that still has to fail? As it stands, an
implementation that simply always returned the build plugin's version would
pass both new tests, so nothing guards against the fix masking real mismatches.
##########
maven-enforcer-plugin/src/it/projects/require-same-versions-916/pom.xml:
##########
@@ -0,0 +1,90 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+-->
+<project xmlns="http://maven.apache.org/POM/4.0.0";
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance";
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0
+ http://maven.apache.org/xsd/maven-4.0.0.xsd";>
+
+ <modelVersion>4.0.0</modelVersion>
+
+ <groupId>org.apache.maven.enforcer.its</groupId>
+ <artifactId>requireSameVersions-916</artifactId>
Review Comment:
Two nits on the IT. The directory name doesn't follow the convention of the
neighbouring ITs (`require-same-versions_success`, `_failure`,
`_multi-module-build`) - something like
`require-same-versions_managed-report-plugin` would describe the case rather
than the issue number. Also the license header uses `* ` prefixes inside the
XML comment, unlike the other IT poms.
--
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]
