#603 Various setting improvements

- Pass PropertyMap to settings class from the outside
- Fix tests not being reentrant due to real file writes
- Improve Node (internal tree for PropertyMap) interface
- Add code coverage for private constructors

src/main/java/fr/xephi/authme/AuthMe.java

@@ -52,6 +52,8 @@ import fr.xephi.authme.settings.properties.PluginSettings;
 import fr.xephi.authme.settings.properties.PurgeSettings;
 import fr.xephi.authme.settings.properties.RestrictionSettings;
 import fr.xephi.authme.settings.properties.SecuritySettings;
+import fr.xephi.authme.settings.properties.SettingsFieldRetriever;
+import fr.xephi.authme.settings.propertymap.PropertyMap;
 import fr.xephi.authme.util.CollectionUtils;
 import fr.xephi.authme.util.FileUtils;
 import fr.xephi.authme.util.GeoLiteAPI;
@@ -464,8 +466,9 @@ public class AuthMe extends JavaPlugin {
 
     private NewSetting createNewSetting() {
         File configFile = new File(getDataFolder(), "config.yml");
+        PropertyMap properties = SettingsFieldRetriever.getAllPropertyFields();
         return FileUtils.copyFileFromResource(configFile, "config.yml")
-            ? new NewSetting(configFile, getDataFolder())
+            ? new NewSetting(configFile, getDataFolder(), properties)
             : null;
     }
 

src/main/java/fr/xephi/authme/settings/NewSetting.java

@@ -6,7 +6,6 @@ import fr.xephi.authme.ConsoleLogger;
 import fr.xephi.authme.settings.domain.Property;
 import fr.xephi.authme.settings.properties.PluginSettings;
 import fr.xephi.authme.settings.properties.RegistrationSettings;
-import fr.xephi.authme.settings.properties.SettingsFieldRetriever;
 import fr.xephi.authme.settings.propertymap.PropertyMap;
 import fr.xephi.authme.util.CollectionUtils;
 import fr.xephi.authme.util.StringUtils;
@@ -33,6 +32,7 @@ public class NewSetting {
 
     private final File pluginFolder;
     private final File configFile;
+    private final PropertyMap propertyMap;
     private FileConfiguration configuration;
     /** The file with the localized messages based on {@link PluginSettings#MESSAGES_LANGUAGE}. */
     private File messagesFile;
@@ -45,10 +45,11 @@ public class NewSetting {
      * @param configFile The configuration file
      * @param pluginFolder The AuthMe plugin folder
      */
-    public NewSetting(File configFile, File pluginFolder) {
+    public NewSetting(File configFile, File pluginFolder, PropertyMap propertyMap) {
         this.configuration = YamlConfiguration.loadConfiguration(configFile);
         this.configFile = configFile;
         this.pluginFolder = pluginFolder;
+        this.propertyMap = propertyMap;
         validateAndLoadOptions();
     }
 
@@ -64,9 +65,10 @@ public class NewSetting {
         this.configuration = configuration;
         this.configFile = configFile;
         this.pluginFolder = new File("");
+        this.propertyMap = propertyMap;
 
-        if (propertyMap != null && SettingsMigrationService.checkAndMigrate(configuration, propertyMap, pluginFolder)) {
+        if (propertyMap != null) {
-            save(propertyMap);
+            validateAndLoadOptions();
         }
     }
 
@@ -92,13 +94,6 @@ public class NewSetting {
         configuration.set(property.getPath(), value);
     }
 
-    /**
-     * Save the config file. Use after migrating one or more settings.
-     */
-    public void save() {
-        save(SettingsFieldRetriever.getAllPropertyFields());
-    }
-
     /**
      * Return the messages file based on the messages language config.
      *
@@ -133,7 +128,10 @@ public class NewSetting {
         validateAndLoadOptions();
     }
 
-    private void save(PropertyMap propertyMap) {
+    /**
+     * Save the config file. Use after migrating one or more settings.
+     */
+    public void save() {
         try (FileWriter writer = new FileWriter(configFile)) {
             Yaml simpleYaml = newYaml(false);
             Yaml singleQuoteYaml = newYaml(true);
@@ -186,11 +184,10 @@ public class NewSetting {
     }
 
     private void validateAndLoadOptions() {
-        PropertyMap propertyMap = SettingsFieldRetriever.getAllPropertyFields();
         if (SettingsMigrationService.checkAndMigrate(configuration, propertyMap, pluginFolder)) {
             ConsoleLogger.info("Merged new config options");
             ConsoleLogger.info("Please check your config.yml file for new settings!");
-            save(propertyMap);
+            save();
         }
 
         messagesFile = buildMessagesFile();

src/main/java/fr/xephi/authme/settings/propertymap/Node.java

@@ -1,5 +1,7 @@
 package fr.xephi.authme.settings.propertymap;
 
+import fr.xephi.authme.ConsoleLogger;
+
 import java.util.ArrayList;
 import java.util.List;
 
@@ -37,14 +39,13 @@ final class Node {
     }
 
     /**
-     * Add a node to the root, creating any intermediary children that don't exist.
+     * Add a child node, creating any intermediary children that don't exist.
      *
-     * @param root The root to add the path to
      * @param fullPath The entire path of the node to add, separate by periods
      */
-    public static void addNode(Node root, String fullPath) {
+    public void addNode(String fullPath) {
         String[] pathParts = fullPath.split("\\.");
-        Node parent = root;
+        Node parent = this;
         for (String part : pathParts) {
             Node child = parent.getChild(part);
             if (child == null) {
@@ -59,17 +60,16 @@ final class Node {
      * Compare two nodes by this class' sorting behavior (insertion order).
      * Note that this method assumes that both supplied paths exist in the tree.
      *
-     * @param root The root of the tree
      * @param fullPath1 The full path to the first node
      * @param fullPath2 The full path to the second node
      * @return The comparison result, in the same format as {@link Comparable#compareTo}
      */
-    public static int compare(Node root, String fullPath1, String fullPath2) {
+    public int compare(String fullPath1, String fullPath2) {
         String[] path1 = fullPath1.split("\\.");
         String[] path2 = fullPath2.split("\\.");
 
         int commonCount = 0;
-        Node commonNode = root;
+        Node commonNode = this;
         while (commonCount < path1.length && commonCount < path2.length
                && path1[commonCount].equals(path2[commonCount]) && commonNode != null) {
             commonNode = commonNode.getChild(path1[commonCount]);
@@ -77,7 +77,7 @@ final class Node {
         }
 
         if (commonNode == null) {
-            System.err.println("Could not find common node for '" + fullPath1 + "' at index " + commonCount);
+            ConsoleLogger.showError("Could not find common node for '" + fullPath1 + "' at index " + commonCount);
             return fullPath1.compareTo(fullPath2); // fallback
         } else if (commonCount >= path1.length || commonCount >= path2.length) {
             return Integer.compare(path1.length, path2.length);

src/main/java/fr/xephi/authme/settings/propertymap/PropertyMapComparator.java

@@ -23,12 +23,12 @@ final class PropertyMapComparator implements Comparator<Property> {
      * @param property The property that is being added
      */
     public void add(Property property) {
-        Node.addNode(parent, property.getPath());
+        parent.addNode(property.getPath());
     }
 
     @Override
     public int compare(Property p1, Property p2) {
-        return Node.compare(parent, p1.getPath(), p2.getPath());
+        return parent.compare(p1.getPath(), p2.getPath());
     }
 
 }

src/test/java/fr/xephi/authme/settings/NewSettingIntegrationTest.java

@@ -1,27 +1,30 @@
 package fr.xephi.authme.settings;
 
 import com.google.common.collect.ImmutableMap;
-import fr.xephi.authme.ReflectionTestUtils;
+import com.google.common.io.Files;
+import fr.xephi.authme.ConsoleLoggerTestInitializer;
+import fr.xephi.authme.TestHelper;
 import fr.xephi.authme.settings.domain.Property;
 import fr.xephi.authme.settings.properties.TestConfiguration;
 import fr.xephi.authme.settings.properties.TestEnum;
 import fr.xephi.authme.settings.propertymap.PropertyMap;
-import fr.xephi.authme.util.WrapperMock;
 import org.bukkit.configuration.file.YamlConfiguration;
+import org.junit.BeforeClass;
+import org.junit.Ignore;
+import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
 
 import java.io.File;
-import java.lang.reflect.Field;
+import java.io.IOException;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
 import java.util.Map;
 
-import static fr.xephi.authme.TestHelper.getJarFile;
 import static fr.xephi.authme.settings.domain.Property.newProperty;
 import static org.hamcrest.Matchers.equalTo;
 import static org.junit.Assert.assertThat;
-import static org.junit.Assume.assumeThat;
 
 /**
  * Integration test for {@link NewSetting}.
@@ -35,16 +38,25 @@ public class NewSettingIntegrationTest {
     /** File name for testing difficult values. */
     private static final String DIFFICULT_FILE = "/config-difficult-values.yml";
 
-    private static PropertyMap propertyMap = generatePropertyMap();
+    private static PropertyMap propertyMap = TestConfiguration.generatePropertyMap();
+
+    @Rule
+    public TemporaryFolder temporaryFolder = new TemporaryFolder();
+
+    @BeforeClass
+    public static void setUpLogger() {
+        ConsoleLoggerTestInitializer.setupLogger();
+    }
 
     @Test
-    public void shouldLoadAndReadAllProperties() {
+    public void shouldLoadAndReadAllProperties() throws IOException {
         // given
-        YamlConfiguration configuration = YamlConfiguration.loadConfiguration(getJarFile(COMPLETE_FILE));
+        YamlConfiguration configuration = YamlConfiguration.loadConfiguration(copyFileFromResources(COMPLETE_FILE));
-        File file = new File("unused");
+        // Pass another, non-existent file to check if the settings had to be rewritten
+        File newFile = temporaryFolder.newFile();
 
         // when / then
-        NewSetting settings = new NewSetting(configuration, file, propertyMap);
+        NewSetting settings = new NewSetting(configuration, newFile, propertyMap);
         Map<Property<?>, Object> expectedValues = ImmutableMap.<Property<?>, Object>builder()
             .put(TestConfiguration.DURATION_IN_SECONDS, 22)
             .put(TestConfiguration.SYSTEM_NAME, "Custom sys name")
@@ -61,15 +73,14 @@ public class NewSettingIntegrationTest {
             assertThat("Property '" + entry.getKey().getPath() + "' has expected value",
                 settings.getProperty(entry.getKey()), equalTo(entry.getValue()));
         }
-        assertThat(file.exists(), equalTo(false));
+        assertThat(newFile.length(), equalTo(0L));
     }
 
     @Test
     public void shouldWriteMissingProperties() {
         // given/when
-        File file = getJarFile(INCOMPLETE_FILE);
+        File file = copyFileFromResources(INCOMPLETE_FILE);
         YamlConfiguration configuration = YamlConfiguration.loadConfiguration(file);
-        assumeThat(configuration.contains(TestConfiguration.BORING_COLORS.getPath()), equalTo(false));
         // Expectation: File is rewritten to since it does not have all configurations
         new NewSetting(configuration, file, propertyMap);
 
@@ -100,23 +111,22 @@ public class NewSettingIntegrationTest {
     @Test
     public void shouldProperlyExportAnyValues() {
         // given
-        File file = getJarFile(DIFFICULT_FILE);
+        File file = copyFileFromResources(DIFFICULT_FILE);
         YamlConfiguration configuration = YamlConfiguration.loadConfiguration(file);
-        assumeThat(configuration.contains(TestConfiguration.DUST_LEVEL.getPath()), equalTo(false));
 
         // Additional string properties
         List<Property<String>> additionalProperties = Arrays.asList(
             newProperty("more.string1", "it's a text with some \\'apostrophes'"),
             newProperty("more.string2", "\tthis one\nhas some\nnew '' lines-test")
         );
-        PropertyMap propertyMap = generatePropertyMap();
+        PropertyMap propertyMap = TestConfiguration.generatePropertyMap();
         for (Property<?> property : additionalProperties) {
             propertyMap.put(property, new String[0]);
         }
 
         // when
         new NewSetting(configuration, file, propertyMap);
-        // reload the file as settings should hav been rewritten
+        // reload the file as settings should have been rewritten
         configuration = YamlConfiguration.loadConfiguration(file);
 
         // then
@@ -147,23 +157,33 @@ public class NewSettingIntegrationTest {
         }
     }
 
-    /**
+    @Test
-     * Generate a property map with all properties in {@link TestConfiguration}.
+    @Ignore
-     *
+    // TODO #603: Un-ignore once migration service is passed to settings
-     * @return The generated property map
+    public void shouldReloadSettings() throws IOException {
-     */
+        // given
-    private static PropertyMap generatePropertyMap() {
+        YamlConfiguration configuration = YamlConfiguration.loadConfiguration(temporaryFolder.newFile());
-        WrapperMock.createInstance();
+        File fullConfigFile = copyFileFromResources(COMPLETE_FILE);
-        PropertyMap propertyMap = new PropertyMap();
+        NewSetting settings = new NewSetting(configuration, fullConfigFile, null);
-        for (Field field : TestConfiguration.class.getDeclaredFields()) {
+
-            Object fieldValue = ReflectionTestUtils.getFieldValue(TestConfiguration.class, null, field.getName());
+        // when
-            if (fieldValue instanceof Property<?>) {
+        assertThat(settings.getProperty(TestConfiguration.RATIO_ORDER),
-                Property<?> property = (Property<?>) fieldValue;
+            equalTo(TestConfiguration.RATIO_ORDER.getDefaultValue()));
-                String[] comments = new String[]{"Comment for '" + property.getPath() + "'"};
+        settings.reload();
-                propertyMap.put(property, comments);
+
+        // then
+        assertThat(settings.getProperty(TestConfiguration.RATIO_ORDER), equalTo(TestEnum.FIRST));
     }
+
+    private File copyFileFromResources(String path) {
+        try {
+            File source = TestHelper.getJarFile(path);
+            File destination = temporaryFolder.newFile();
+            Files.copy(source, destination);
+            return destination;
+        } catch (IOException e) {
+            throw new IllegalStateException("Could not copy test file", e);
         }
-        return propertyMap;
     }
 
 }

src/test/java/fr/xephi/authme/settings/NewSettingTest.java

@@ -1,15 +1,22 @@
 package fr.xephi.authme.settings;
 
+import fr.xephi.authme.ConsoleLoggerTestInitializer;
 import fr.xephi.authme.settings.domain.Property;
 import fr.xephi.authme.settings.properties.TestConfiguration;
 import fr.xephi.authme.settings.properties.TestEnum;
 import org.bukkit.configuration.file.YamlConfiguration;
+import org.junit.BeforeClass;
+import org.junit.Ignore;
+import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
 import org.mockito.internal.stubbing.answers.ReturnsArgumentAt;
 
+import java.io.File;
 import java.io.IOException;
 import java.io.InputStream;
 
+import static fr.xephi.authme.settings.properties.PluginSettings.MESSAGES_LANGUAGE;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.not;
@@ -21,13 +28,22 @@ import static org.mockito.Matchers.anyInt;
 import static org.mockito.Matchers.anyString;
 import static org.mockito.Matchers.eq;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
 /**
- * Test for {@link NewSetting}.
+ * Unit tests for {@link NewSetting}.
  */
 public class NewSettingTest {
 
+    @Rule
+    public TemporaryFolder temporaryFolder = new TemporaryFolder();
+
+    @BeforeClass
+    public static void setUpLogger() {
+        ConsoleLoggerTestInitializer.setupLogger();
+    }
+
     @Test
     public void shouldLoadAllConfigs() {
         // given
@@ -71,6 +87,36 @@ public class NewSettingTest {
         assertThat(stream.read(), not(equalTo(0)));
     }
 
+    @Test
+    public void shouldSetProperty() {
+        // given
+        YamlConfiguration configuration = mock(YamlConfiguration.class);
+        NewSetting settings = new NewSetting(configuration, null, null);
+
+        // when
+        settings.setProperty(TestConfiguration.DUST_LEVEL, -4);
+
+        // then
+        verify(configuration).set(TestConfiguration.DUST_LEVEL.getPath(), -4);
+    }
+
+    @Test
+    @Ignore
+    // TODO #603: Un-ignore once migration service is injected into settings
+    public void shouldReturnMessagesFile() {
+        // given
+        YamlConfiguration configuration = mock(YamlConfiguration.class);
+        given(configuration.contains(anyString())).willReturn(true);
+        given(configuration.getString(eq(MESSAGES_LANGUAGE.getPath()), anyString())).willReturn("fr");
+        NewSetting settings = new NewSetting(configuration, null, TestConfiguration.generatePropertyMap());
+
+        // when
+        File messagesFile = settings.getMessagesFile();
+
+        // then
+        System.out.println(messagesFile.getPath());
+    }
+
     private static <T> void setReturnValue(YamlConfiguration config, Property<T> property, T value) {
         if (value instanceof String) {
             when(config.getString(eq(property.getPath()), anyString())).thenReturn((String) value);

src/test/java/fr/xephi/authme/settings/properties/SettingsClassConsistencyTest.java

@@ -9,6 +9,7 @@ import org.junit.Test;
 import java.io.File;
 import java.lang.reflect.Constructor;
 import java.lang.reflect.Field;
+import java.lang.reflect.InvocationTargetException;
 import java.lang.reflect.Modifier;
 import java.util.ArrayList;
 import java.util.HashSet;
@@ -92,6 +93,17 @@ public class SettingsClassConsistencyTest {
                 constructors, arrayWithSize(1));
             assertThat("Constructor of " + clazz + " is private",
                 Modifier.isPrivate(constructors[0].getModifiers()), equalTo(true));
+
+            // Ugly hack to get coverage on the private constructors
+            // http://stackoverflow.com/questions/14077842/how-to-test-a-private-constructor-in-java-application
+            try {
+                Constructor<?> constructor = clazz.getDeclaredConstructor();
+                constructor.setAccessible(true);
+                constructor.newInstance();
+            } catch (NoSuchMethodException | InstantiationException
+                | IllegalAccessException | InvocationTargetException e) {
+                e.printStackTrace();
+            }
         }
     }
 

src/test/java/fr/xephi/authme/settings/properties/TestConfiguration.java

@@ -1,9 +1,13 @@
 package fr.xephi.authme.settings.properties;
 
+import fr.xephi.authme.ReflectionTestUtils;
 import fr.xephi.authme.settings.domain.Property;
 import fr.xephi.authme.settings.domain.PropertyType;
 import fr.xephi.authme.settings.domain.SettingsClass;
+import fr.xephi.authme.settings.propertymap.PropertyMap;
+import fr.xephi.authme.util.WrapperMock;
 
+import java.lang.reflect.Field;
 import java.util.List;
 
 import static fr.xephi.authme.settings.domain.Property.newProperty;
@@ -47,4 +51,23 @@ public final class TestConfiguration implements SettingsClass {
     private TestConfiguration() {
     }
 
+    /**
+     * Generate a property map with all properties in {@link TestConfiguration}.
+     *
+     * @return The generated property map
+     */
+    public static PropertyMap generatePropertyMap() {
+        WrapperMock.createInstance();
+        PropertyMap propertyMap = new PropertyMap();
+        for (Field field : TestConfiguration.class.getDeclaredFields()) {
+            Object fieldValue = ReflectionTestUtils.getFieldValue(TestConfiguration.class, null, field.getName());
+            if (fieldValue instanceof Property<?>) {
+                Property<?> property = (Property<?>) fieldValue;
+                String[] comments = new String[]{"Comment for '" + property.getPath() + "'"};
+                propertyMap.put(property, comments);
+            }
+        }
+        return propertyMap;
+    }
+
 }