[GitHub] [maven] elharo commented on a change in pull request #347: [MNG-6802] Fix bug in FileProfileActivator

[GitBox <gi...@apache.org>]

elharo commented on a change in pull request #347:
URL: https://github.com/apache/maven/pull/347#discussion_r628776539



##########
File path: maven-model-builder/src/test/java/org/apache/maven/model/profile/activation/FileProfileActivatorTest.java
##########
@@ -0,0 +1,142 @@
+package org.apache.maven.model.profile.activation;
+
+/*
+ * 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.
+ */
+
+import org.apache.maven.model.Activation;
+import org.apache.maven.model.ActivationFile;
+import org.apache.maven.model.Profile;
+import org.apache.maven.model.path.DefaultPathTranslator;
+import org.apache.maven.model.path.ProfileActivationFilePathInterpolator;
+import org.apache.maven.model.profile.DefaultProfileActivationContext;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Path;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+/**
+ * Tests {@link FileProfileActivator}.
+ *
+ * @author Ravil Galeyev
+ */
+public class FileProfileActivatorTest extends AbstractProfileActivatorTest<FileProfileActivator>
+{
+    private static final String FILE = "file.txt";

Review comment:
       This constant doesn't really add anything. Just inline it. The string literal value is clearer. 

##########
File path: maven-model-builder/src/test/java/org/apache/maven/model/profile/activation/FileProfileActivatorTest.java
##########
@@ -0,0 +1,142 @@
+package org.apache.maven.model.profile.activation;
+
+/*
+ * 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.
+ */
+
+import org.apache.maven.model.Activation;
+import org.apache.maven.model.ActivationFile;
+import org.apache.maven.model.Profile;
+import org.apache.maven.model.path.DefaultPathTranslator;
+import org.apache.maven.model.path.ProfileActivationFilePathInterpolator;
+import org.apache.maven.model.profile.DefaultProfileActivationContext;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Path;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+/**
+ * Tests {@link FileProfileActivator}.
+ *
+ * @author Ravil Galeyev
+ */
+public class FileProfileActivatorTest extends AbstractProfileActivatorTest<FileProfileActivator>
+{
+    private static final String FILE = "file.txt";
+
+    @TempDir
+    Path tempDir;
+
+    private final DefaultProfileActivationContext context = new DefaultProfileActivationContext();
+
+    public FileProfileActivatorTest()
+    {
+        super( FileProfileActivator.class );
+    }
+
+    @BeforeEach
+    public void setUp() throws Exception
+    {
+        super.setUp();
+        activator.setProfileActivationFilePathInterpolator(
+                new ProfileActivationFilePathInterpolator().setPathTranslator( new DefaultPathTranslator() ) );
+
+        context.setProjectDirectory( new File( tempDir.toString() ) );
+
+        File file = new File( tempDir.resolve( FILE ).toString() );
+        if ( !file.exists() )
+        {
+            if ( !file.createNewFile() )
+            {
+                throw new IOException( "Can't create " + file );
+            }
+        }
+    }
+
+
+    @Test
+    public void test_isActive_noFile()
+    {
+        assertActivation( false, newExistsProfile( null ), context );
+        assertActivation( false, newExistsProfile( "someFile.txt" ), context );
+        assertActivation( false, newExistsProfile( "${basedir}/someFile.txt" ), context );
+
+        assertActivation( false, newMissingProfile( null ), context );
+        assertActivation( true, newMissingProfile( "someFile.txt" ), context );
+        assertActivation( true, newMissingProfile( "${basedir}/someFile.txt" ), context );
+    }
+
+    @Test
+    public void test_isActiveExists_fileExists()
+    {
+        assertActivation( true, newExistsProfile( FILE ), context );
+        assertActivation( true, newExistsProfile( "${basedir}" ), context );
+        assertActivation( true, newExistsProfile( "${basedir}/" + FILE ), context );
+
+        assertActivation( false, newMissingProfile( FILE ), context );
+        assertActivation( false, newMissingProfile( "${basedir}" ), context );
+        assertActivation( false, newMissingProfile( "${basedir}/" + FILE ), context );
+    }
+
+    @Test
+    public void test_isActiveExists_leavesFileUnchanged()
+    {
+        Profile profile = newExistsProfile( FILE );
+        assertEquals( profile.getActivation().getFile().getExists(), FILE );

Review comment:
       expected values come first, actual values second

##########
File path: maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuilder.java
##########
@@ -482,6 +500,68 @@ else if ( !parentIds.add( parentData.getId() ) )
         return resultModel;
     }
 
+    private Map<String, Activation> getInterpolatedActivations( Model rawModel,
+                                                                DefaultProfileActivationContext context,
+                                                                DefaultModelProblemCollector problems )
+    {
+        Map<String, Activation> interpolatedActivations = getProfileActivations( rawModel, true );
+        for ( Activation activation : interpolatedActivations.values() )
+        {
+            if ( activation.getFile() != null )
+            {
+                replaceWithInterpolatedValue( activation.getFile(), context, problems );
+            }
+        }
+        return interpolatedActivations;
+    }
+
+    private void replaceWithInterpolatedValue( ActivationFile activationFile, ProfileActivationContext context,
+                                               DefaultModelProblemCollector problems  )
+    {
+        String path;
+        boolean missing;

Review comment:
       This missing variable feels complex, in a cyclomatic sense. It might be simpler to remove it and put more logic inside the first if-else if blocks. What do you think?

##########
File path: maven-model-builder/src/test/java/org/apache/maven/model/profile/activation/FileProfileActivatorTest.java
##########
@@ -0,0 +1,142 @@
+package org.apache.maven.model.profile.activation;
+
+/*
+ * 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.
+ */
+
+import org.apache.maven.model.Activation;
+import org.apache.maven.model.ActivationFile;
+import org.apache.maven.model.Profile;
+import org.apache.maven.model.path.DefaultPathTranslator;
+import org.apache.maven.model.path.ProfileActivationFilePathInterpolator;
+import org.apache.maven.model.profile.DefaultProfileActivationContext;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Path;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+/**
+ * Tests {@link FileProfileActivator}.
+ *
+ * @author Ravil Galeyev
+ */
+public class FileProfileActivatorTest extends AbstractProfileActivatorTest<FileProfileActivator>
+{
+    private static final String FILE = "file.txt";
+
+    @TempDir
+    Path tempDir;
+
+    private final DefaultProfileActivationContext context = new DefaultProfileActivationContext();
+
+    public FileProfileActivatorTest()
+    {
+        super( FileProfileActivator.class );
+    }
+
+    @BeforeEach
+    public void setUp() throws Exception
+    {
+        super.setUp();
+        activator.setProfileActivationFilePathInterpolator(
+                new ProfileActivationFilePathInterpolator().setPathTranslator( new DefaultPathTranslator() ) );
+
+        context.setProjectDirectory( new File( tempDir.toString() ) );
+
+        File file = new File( tempDir.resolve( FILE ).toString() );
+        if ( !file.exists() )
+        {
+            if ( !file.createNewFile() )
+            {
+                throw new IOException( "Can't create " + file );
+            }
+        }
+    }
+
+
+    @Test
+    public void test_isActive_noFile()

Review comment:
       testIsActive_noFile is the normal naming convention. That is, no undescore before the method name and after test

##########
File path: maven-model-builder/src/main/java/org/apache/maven/model/path/ProfileActivationFilePathInterpolator.java
##########
@@ -0,0 +1,103 @@
+package org.apache.maven.model.path;
+
+/*
+ * 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.
+ */
+
+import org.apache.maven.model.ActivationFile;
+import org.apache.maven.model.profile.ProfileActivationContext;
+import org.codehaus.plexus.interpolation.AbstractValueSource;
+import org.codehaus.plexus.interpolation.InterpolationException;
+import org.codehaus.plexus.interpolation.MapBasedValueSource;
+import org.codehaus.plexus.interpolation.RegexBasedInterpolator;
+
+import javax.inject.Inject;
+import javax.inject.Named;
+import javax.inject.Singleton;
+import java.io.File;
+
+/**
+ * Finds an absolute path for {@link ActivationFile#getExists()} or {@link ActivationFile#getMissing()}
+ *
+ * @author Ravil Galeyev
+ */
+@Named
+@Singleton
+public class ProfileActivationFilePathInterpolator
+{
+
+    @Inject
+    private PathTranslator pathTranslator;
+
+    public ProfileActivationFilePathInterpolator setPathTranslator( PathTranslator pathTranslator )
+    {
+        this.pathTranslator = pathTranslator;
+        return this;
+    }
+
+    /**
+     * Interpolates given {@code path}.
+     *
+     * @return absolute path or {@code null} if the input was {@code null}
+     */
+    public String interpolate( String path, ProfileActivationContext context ) throws InterpolationException
+    {
+        if ( path == null )
+        {
+            return null;
+        }
+
+        RegexBasedInterpolator interpolator = new RegexBasedInterpolator();
+
+        final File basedir = context.getProjectDirectory();
+
+        if ( basedir != null )
+        {
+            interpolator.addValueSource( new AbstractValueSource( false )
+            {
+                @Override
+                public Object getValue( String expression )
+                {
+                    /*
+                     * We intentionally only support ${basedir} and not ${project.basedir} as the latter form
+                     * would suggest that other project.* expressions can be used which is beyond the design.

Review comment:
       got it

##########
File path: maven-model-builder/src/test/java/org/apache/maven/model/profile/activation/FileProfileActivatorTest.java
##########
@@ -0,0 +1,142 @@
+package org.apache.maven.model.profile.activation;
+
+/*
+ * 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.
+ */
+
+import org.apache.maven.model.Activation;
+import org.apache.maven.model.ActivationFile;
+import org.apache.maven.model.Profile;
+import org.apache.maven.model.path.DefaultPathTranslator;
+import org.apache.maven.model.path.ProfileActivationFilePathInterpolator;
+import org.apache.maven.model.profile.DefaultProfileActivationContext;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Path;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+/**
+ * Tests {@link FileProfileActivator}.
+ *
+ * @author Ravil Galeyev
+ */
+public class FileProfileActivatorTest extends AbstractProfileActivatorTest<FileProfileActivator>
+{
+    private static final String FILE = "file.txt";
+
+    @TempDir
+    Path tempDir;
+
+    private final DefaultProfileActivationContext context = new DefaultProfileActivationContext();
+
+    public FileProfileActivatorTest()
+    {
+        super( FileProfileActivator.class );
+    }
+
+    @BeforeEach
+    public void setUp() throws Exception
+    {
+        super.setUp();
+        activator.setProfileActivationFilePathInterpolator(
+                new ProfileActivationFilePathInterpolator().setPathTranslator( new DefaultPathTranslator() ) );
+
+        context.setProjectDirectory( new File( tempDir.toString() ) );
+
+        File file = new File( tempDir.resolve( FILE ).toString() );
+        if ( !file.exists() )
+        {
+            if ( !file.createNewFile() )
+            {
+                throw new IOException( "Can't create " + file );
+            }
+        }
+    }
+
+
+    @Test
+    public void test_isActive_noFile()
+    {
+        assertActivation( false, newExistsProfile( null ), context );
+        assertActivation( false, newExistsProfile( "someFile.txt" ), context );
+        assertActivation( false, newExistsProfile( "${basedir}/someFile.txt" ), context );
+
+        assertActivation( false, newMissingProfile( null ), context );
+        assertActivation( true, newMissingProfile( "someFile.txt" ), context );
+        assertActivation( true, newMissingProfile( "${basedir}/someFile.txt" ), context );
+    }
+
+    @Test
+    public void test_isActiveExists_fileExists()
+    {
+        assertActivation( true, newExistsProfile( FILE ), context );
+        assertActivation( true, newExistsProfile( "${basedir}" ), context );
+        assertActivation( true, newExistsProfile( "${basedir}/" + FILE ), context );
+
+        assertActivation( false, newMissingProfile( FILE ), context );
+        assertActivation( false, newMissingProfile( "${basedir}" ), context );
+        assertActivation( false, newMissingProfile( "${basedir}/" + FILE ), context );
+    }
+
+    @Test
+    public void test_isActiveExists_leavesFileUnchanged()
+    {
+        Profile profile = newExistsProfile( FILE );
+        assertEquals( profile.getActivation().getFile().getExists(), FILE );
+
+        assertActivation( true, profile, context );

Review comment:
       testIsActive_noFile is the normal naming convention. That is, no undescore before the method name and after test

##########
File path: maven-model-builder/src/test/java/org/apache/maven/model/profile/activation/FileProfileActivatorTest.java
##########
@@ -0,0 +1,142 @@
+package org.apache.maven.model.profile.activation;
+
+/*
+ * 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.
+ */
+
+import org.apache.maven.model.Activation;
+import org.apache.maven.model.ActivationFile;
+import org.apache.maven.model.Profile;
+import org.apache.maven.model.path.DefaultPathTranslator;
+import org.apache.maven.model.path.ProfileActivationFilePathInterpolator;
+import org.apache.maven.model.profile.DefaultProfileActivationContext;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Path;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+/**
+ * Tests {@link FileProfileActivator}.
+ *
+ * @author Ravil Galeyev
+ */
+public class FileProfileActivatorTest extends AbstractProfileActivatorTest<FileProfileActivator>
+{
+    private static final String FILE = "file.txt";
+
+    @TempDir
+    Path tempDir;
+
+    private final DefaultProfileActivationContext context = new DefaultProfileActivationContext();
+
+    public FileProfileActivatorTest()
+    {
+        super( FileProfileActivator.class );
+    }
+
+    @BeforeEach
+    public void setUp() throws Exception
+    {
+        super.setUp();
+        activator.setProfileActivationFilePathInterpolator(
+                new ProfileActivationFilePathInterpolator().setPathTranslator( new DefaultPathTranslator() ) );
+
+        context.setProjectDirectory( new File( tempDir.toString() ) );
+
+        File file = new File( tempDir.resolve( FILE ).toString() );
+        if ( !file.exists() )
+        {
+            if ( !file.createNewFile() )
+            {
+                throw new IOException( "Can't create " + file );
+            }
+        }
+    }
+
+
+    @Test
+    public void test_isActive_noFile()
+    {
+        assertActivation( false, newExistsProfile( null ), context );
+        assertActivation( false, newExistsProfile( "someFile.txt" ), context );
+        assertActivation( false, newExistsProfile( "${basedir}/someFile.txt" ), context );
+
+        assertActivation( false, newMissingProfile( null ), context );
+        assertActivation( true, newMissingProfile( "someFile.txt" ), context );
+        assertActivation( true, newMissingProfile( "${basedir}/someFile.txt" ), context );
+    }
+
+    @Test
+    public void test_isActiveExists_fileExists()
+    {
+        assertActivation( true, newExistsProfile( FILE ), context );
+        assertActivation( true, newExistsProfile( "${basedir}" ), context );
+        assertActivation( true, newExistsProfile( "${basedir}/" + FILE ), context );
+
+        assertActivation( false, newMissingProfile( FILE ), context );
+        assertActivation( false, newMissingProfile( "${basedir}" ), context );
+        assertActivation( false, newMissingProfile( "${basedir}/" + FILE ), context );
+    }
+
+    @Test
+    public void test_isActiveExists_leavesFileUnchanged()
+    {
+        Profile profile = newExistsProfile( FILE );
+        assertEquals( profile.getActivation().getFile().getExists(), FILE );
+
+        assertActivation( true, profile, context );
+
+        assertEquals( profile.getActivation().getFile().getExists(), FILE );

Review comment:
       expected values come first, actual values second

##########
File path: maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuilder.java
##########
@@ -482,6 +500,68 @@ else if ( !parentIds.add( parentData.getId() ) )
         return resultModel;
     }
 
+    private Map<String, Activation> getInterpolatedActivations( Model rawModel,
+                                                                DefaultProfileActivationContext context,
+                                                                DefaultModelProblemCollector problems )
+    {
+        Map<String, Activation> interpolatedActivations = getProfileActivations( rawModel, true );
+        for ( Activation activation : interpolatedActivations.values() )
+        {
+            if ( activation.getFile() != null )
+            {
+                replaceWithInterpolatedValue( activation.getFile(), context, problems );
+            }
+        }
+        return interpolatedActivations;
+    }
+
+    private void replaceWithInterpolatedValue( ActivationFile activationFile, ProfileActivationContext context,
+                                               DefaultModelProblemCollector problems  )
+    {
+        String path;
+        boolean missing;
+
+        if ( isNotEmpty( activationFile.getExists() ) )
+        {
+            path = activationFile.getExists();
+            missing = false;
+        }
+        else if ( isNotEmpty( activationFile.getMissing() ) )
+        {
+            path = activationFile.getMissing();
+            missing = true;
+        }
+        else
+        {
+            return;
+        }
+
+        try

Review comment:
       I think moving the try block to surround everything, and combining the if-else blocks inside it should work. 

##########
File path: maven-model-builder/src/test/java/org/apache/maven/model/profile/activation/FileProfileActivatorTest.java
##########
@@ -0,0 +1,142 @@
+package org.apache.maven.model.profile.activation;
+
+/*
+ * 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.
+ */
+
+import org.apache.maven.model.Activation;
+import org.apache.maven.model.ActivationFile;
+import org.apache.maven.model.Profile;
+import org.apache.maven.model.path.DefaultPathTranslator;
+import org.apache.maven.model.path.ProfileActivationFilePathInterpolator;
+import org.apache.maven.model.profile.DefaultProfileActivationContext;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Path;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+/**
+ * Tests {@link FileProfileActivator}.
+ *
+ * @author Ravil Galeyev
+ */
+public class FileProfileActivatorTest extends AbstractProfileActivatorTest<FileProfileActivator>
+{
+    private static final String FILE = "file.txt";
+
+    @TempDir
+    Path tempDir;
+
+    private final DefaultProfileActivationContext context = new DefaultProfileActivationContext();
+
+    public FileProfileActivatorTest()
+    {
+        super( FileProfileActivator.class );
+    }
+
+    @BeforeEach
+    public void setUp() throws Exception
+    {
+        super.setUp();
+        activator.setProfileActivationFilePathInterpolator(
+                new ProfileActivationFilePathInterpolator().setPathTranslator( new DefaultPathTranslator() ) );
+
+        context.setProjectDirectory( new File( tempDir.toString() ) );
+
+        File file = new File( tempDir.resolve( FILE ).toString() );
+        if ( !file.exists() )

Review comment:
       You shouldn't need these checks here. 

##########
File path: maven-model-builder/src/test/java/org/apache/maven/model/profile/activation/FileProfileActivatorTest.java
##########
@@ -0,0 +1,142 @@
+package org.apache.maven.model.profile.activation;
+
+/*
+ * 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.
+ */
+
+import org.apache.maven.model.Activation;
+import org.apache.maven.model.ActivationFile;
+import org.apache.maven.model.Profile;
+import org.apache.maven.model.path.DefaultPathTranslator;
+import org.apache.maven.model.path.ProfileActivationFilePathInterpolator;
+import org.apache.maven.model.profile.DefaultProfileActivationContext;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Path;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+/**
+ * Tests {@link FileProfileActivator}.
+ *
+ * @author Ravil Galeyev
+ */
+public class FileProfileActivatorTest extends AbstractProfileActivatorTest<FileProfileActivator>
+{
+    private static final String FILE = "file.txt";
+
+    @TempDir
+    Path tempDir;
+
+    private final DefaultProfileActivationContext context = new DefaultProfileActivationContext();
+
+    public FileProfileActivatorTest()
+    {
+        super( FileProfileActivator.class );
+    }
+
+    @BeforeEach
+    public void setUp() throws Exception
+    {
+        super.setUp();
+        activator.setProfileActivationFilePathInterpolator(
+                new ProfileActivationFilePathInterpolator().setPathTranslator( new DefaultPathTranslator() ) );
+
+        context.setProjectDirectory( new File( tempDir.toString() ) );
+
+        File file = new File( tempDir.resolve( FILE ).toString() );
+        if ( !file.exists() )
+        {
+            if ( !file.createNewFile() )
+            {
+                throw new IOException( "Can't create " + file );
+            }
+        }
+    }
+
+
+    @Test
+    public void test_isActive_noFile()
+    {
+        assertActivation( false, newExistsProfile( null ), context );
+        assertActivation( false, newExistsProfile( "someFile.txt" ), context );
+        assertActivation( false, newExistsProfile( "${basedir}/someFile.txt" ), context );
+
+        assertActivation( false, newMissingProfile( null ), context );
+        assertActivation( true, newMissingProfile( "someFile.txt" ), context );
+        assertActivation( true, newMissingProfile( "${basedir}/someFile.txt" ), context );
+    }
+
+    @Test
+    public void test_isActiveExists_fileExists()
+    {
+        assertActivation( true, newExistsProfile( FILE ), context );
+        assertActivation( true, newExistsProfile( "${basedir}" ), context );
+        assertActivation( true, newExistsProfile( "${basedir}/" + FILE ), context );
+
+        assertActivation( false, newMissingProfile( FILE ), context );
+        assertActivation( false, newMissingProfile( "${basedir}" ), context );
+        assertActivation( false, newMissingProfile( "${basedir}/" + FILE ), context );
+    }
+
+    @Test
+    public void test_isActiveExists_leavesFileUnchanged()
+    {
+        Profile profile = newExistsProfile( FILE );
+        assertEquals( profile.getActivation().getFile().getExists(), FILE );
+
+        assertActivation( true, profile, context );
+
+        assertEquals( profile.getActivation().getFile().getExists(), FILE );
+    }
+
+    private Profile newExistsProfile( String filePath )
+    {
+        return newProfile( filePath, true );
+    }
+
+    private Profile newMissingProfile( String filePath )
+    {
+        return newProfile( filePath, false );
+    }
+
+    private Profile newProfile( String filePath, boolean exists )

Review comment:
       Instead of an exists argument, consider two methods: newProfile and newMissingProfile or something like that. 

##########
File path: maven-model-builder/src/test/java/org/apache/maven/model/profile/activation/FileProfileActivatorTest.java
##########
@@ -0,0 +1,142 @@
+package org.apache.maven.model.profile.activation;
+
+/*
+ * 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.
+ */
+
+import org.apache.maven.model.Activation;
+import org.apache.maven.model.ActivationFile;
+import org.apache.maven.model.Profile;
+import org.apache.maven.model.path.DefaultPathTranslator;
+import org.apache.maven.model.path.ProfileActivationFilePathInterpolator;
+import org.apache.maven.model.profile.DefaultProfileActivationContext;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Path;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+/**
+ * Tests {@link FileProfileActivator}.
+ *
+ * @author Ravil Galeyev
+ */
+public class FileProfileActivatorTest extends AbstractProfileActivatorTest<FileProfileActivator>
+{
+    private static final String FILE = "file.txt";
+
+    @TempDir
+    Path tempDir;
+
+    private final DefaultProfileActivationContext context = new DefaultProfileActivationContext();
+
+    public FileProfileActivatorTest()
+    {
+        super( FileProfileActivator.class );
+    }
+
+    @BeforeEach
+    public void setUp() throws Exception
+    {
+        super.setUp();
+        activator.setProfileActivationFilePathInterpolator(
+                new ProfileActivationFilePathInterpolator().setPathTranslator( new DefaultPathTranslator() ) );
+
+        context.setProjectDirectory( new File( tempDir.toString() ) );
+
+        File file = new File( tempDir.resolve( FILE ).toString() );
+        if ( !file.exists() )
+        {
+            if ( !file.createNewFile() )
+            {
+                throw new IOException( "Can't create " + file );
+            }
+        }
+    }
+
+
+    @Test
+    public void test_isActive_noFile()
+    {
+        assertActivation( false, newExistsProfile( null ), context );
+        assertActivation( false, newExistsProfile( "someFile.txt" ), context );
+        assertActivation( false, newExistsProfile( "${basedir}/someFile.txt" ), context );
+
+        assertActivation( false, newMissingProfile( null ), context );
+        assertActivation( true, newMissingProfile( "someFile.txt" ), context );
+        assertActivation( true, newMissingProfile( "${basedir}/someFile.txt" ), context );
+    }
+
+    @Test
+    public void test_isActiveExists_fileExists()

Review comment:
       testIsActive_noFile is the normal naming convention. That is, no undescore before the method name and after test




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org