elharo commented on code in PR #1220:
URL: https://github.com/apache/maven/pull/1220#discussion_r1312882728
##########
maven-core/pom.xml:
##########
@@ -153,7 +153,7 @@ under the License.
</dependency>
<dependency>
<groupId>org.mockito</groupId>
- <artifactId>mockito-core</artifactId>
+ <artifactId>mockito-inline</artifactId>
Review Comment:
Is this change related?
##########
maven-core/src/test/java/org/apache/maven/classrealm/DefaultClassRealmManagerTest.java:
##########
@@ -0,0 +1,164 @@
+/*
+ * 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.
+ */
+package org.apache.maven.classrealm;
+
+import java.io.File;
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.List;
+
+import org.apache.maven.extension.internal.CoreExports;
+import org.apache.maven.model.Model;
+import org.codehaus.plexus.DefaultPlexusContainer;
+import org.codehaus.plexus.PlexusContainer;
+import org.codehaus.plexus.PlexusContainerException;
+import org.codehaus.plexus.classworlds.realm.ClassRealm;
+import org.eclipse.aether.artifact.Artifact;
+import org.junit.jupiter.api.Test;
+import org.mockito.InOrder;
+import org.mockito.MockedStatic;
+import org.mockito.Mockito;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.endsWith;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.mockito.Mockito.calls;
+import static org.mockito.Mockito.inOrder;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.when;
+
+/**
+ * @author Sebastien Doyon
+ */
+class DefaultClassRealmManagerTest {
+
+ private DefaultClassRealmManager
newDefaultClassRealmManager(PlexusContainer container) {
+ HashSet<String> exportedPackages = new HashSet<String>();
+ exportedPackages.add("group1:artifact1");
+
+ return new DefaultClassRealmManager(
+ container,
+ new ArrayList<ClassRealmManagerDelegate>(),
+ new CoreExports(new ClassRealm(null, "test", null), new
HashSet<String>(), exportedPackages));
+ }
+
+ private List<Artifact> newTestArtifactList() {
+ List<Artifact> artifacts = new ArrayList<Artifact>();
+
+ Artifact artifact = mock(Artifact.class);
+ when(artifact.getFile()).thenReturn(new File(new
File("local/repository"), "some/path"));
+ when(artifact.getGroupId()).thenReturn("group1");
+ when(artifact.getArtifactId()).thenReturn("artifact1");
+ when(artifact.getExtension()).thenReturn("ext");
+ when(artifact.getClassifier()).thenReturn("classifier1");
+ when(artifact.getVersion()).thenReturn("1");
+ artifacts.add(artifact);
+
+ Artifact artifact2 = mock(Artifact.class);
+ when(artifact2.getFile()).thenReturn(null);
+ when(artifact2.getGroupId()).thenReturn("group1");
+ when(artifact2.getArtifactId()).thenReturn("artifact2");
+ when(artifact2.getExtension()).thenReturn("ext");
+ when(artifact2.getClassifier()).thenReturn("classifier1");
+ when(artifact2.getVersion()).thenReturn("1");
+ artifacts.add(artifact2);
+
+ return artifacts;
+ }
+
+ private Model newTestModel() {
+ Model model = new Model();
+ model.setGroupId("modelGroup1");
+ model.setArtifactId("modelArtifact1");
+ model.setVersion("modelVersion1");
+
+ return model;
+ }
+
+ @Test
+ void testDebugEnabled() throws PlexusContainerException {
+ Logger logger = mock(Logger.class);
+ when(logger.isDebugEnabled()).thenReturn(true);
+
+ DefaultClassRealmManager classRealmManager;
+ ClassRealm classRealm;
+
+ InOrder verifier = inOrder(logger);
+
+ PlexusContainer container = new DefaultPlexusContainer();
+
+ try (MockedStatic<LoggerFactory> mockedLoggerFactory =
Mockito.mockStatic(LoggerFactory.class)) {
Review Comment:
In general, I'm skeptical of testing log messages. I tend to think of them
as side effects that can change pretty freely, and not part of a public API
contract that should be tested.
##########
maven-core/src/main/java/org/apache/maven/plugin/internal/MojoLogWrapper.java:
##########
@@ -34,7 +34,9 @@ public MojoLogWrapper(Logger logger) {
}
public void debug(CharSequence content) {
- logger.debug(toString(content));
+ if (isDebugEnabled()) {
Review Comment:
I notice this duplicates a lot of content from DefaultLog above. I wonder if
that's fixable somehow? Not necessarily in this PR.
##########
maven-core/src/main/java/org/apache/maven/internal/impl/DefaultLog.java:
##########
@@ -33,12 +33,16 @@ public DefaultLog(Logger logger) {
}
public void debug(CharSequence content) {
- logger.debug(toString(content));
+ if (isDebugEnabled()) {
Review Comment:
I like this file's changes. They're nice and clean and in one place.
##########
maven-core/src/main/java/org/apache/maven/classrealm/DefaultClassRealmManager.java:
##########
@@ -152,14 +150,20 @@ private ClassRealm createRealm(
List<String> parentImports,
Map<String, ClassLoader> foreignImports,
List<Artifact> artifacts) {
- Set<String> artifactIds = new LinkedHashSet<>();
+ Set<String> artifactIds = Collections.emptySet();
Review Comment:
slipping in an immutable set where a mutable one existed previously is
risky. It's safer to just make it always a LinkedHashSet. I'm, not saying this
case is wrong, but I have experienced a really nasty, hard to debug production
outage by doing pretty much this.
##########
maven-core/src/main/java/org/apache/maven/classrealm/DefaultClassRealmManager.java:
##########
@@ -301,20 +305,22 @@ private void callDelegates(
}
private Set<String> populateRealm(ClassRealm classRealm,
List<ClassRealmConstituent> constituents) {
- Set<String> includedIds = new LinkedHashSet<>();
+ Set<String> includedIds = Collections.emptySet();
Review Comment:
revert
--
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: issues-unsubscr...@maven.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
