From 3522a5b0c008be2f76c8ae391c3c44b0791480f6 Mon Sep 17 00:00:00 2001 From: ljacqu Date: Mon, 14 Mar 2016 20:32:32 +0100 Subject: [PATCH] #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 | 5 +- .../fr/xephi/authme/settings/NewSetting.java | 25 +++--- .../xephi/authme/settings/domain/Comment.java | 2 +- .../authme/settings/propertymap/Node.java | 16 ++-- .../propertymap/PropertyMapComparator.java | 4 +- .../settings/NewSettingIntegrationTest.java | 86 ++++++++++++------- .../xephi/authme/settings/NewSettingTest.java | 48 ++++++++++- .../SettingsClassConsistencyTest.java | 12 +++ .../properties/TestConfiguration.java | 23 +++++ 9 files changed, 161 insertions(+), 60 deletions(-) diff --git a/src/main/java/fr/xephi/authme/AuthMe.java b/src/main/java/fr/xephi/authme/AuthMe.java index e10c815c5..b4827cef7 100644 --- a/src/main/java/fr/xephi/authme/AuthMe.java +++ b/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; } diff --git a/src/main/java/fr/xephi/authme/settings/NewSetting.java b/src/main/java/fr/xephi/authme/settings/NewSetting.java index 00a597249..809655834 100644 --- a/src/main/java/fr/xephi/authme/settings/NewSetting.java +++ b/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)) { - save(propertyMap); + if (propertyMap != null) { + 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(); diff --git a/src/main/java/fr/xephi/authme/settings/domain/Comment.java b/src/main/java/fr/xephi/authme/settings/domain/Comment.java index 07f20e257..0e69ec97a 100644 --- a/src/main/java/fr/xephi/authme/settings/domain/Comment.java +++ b/src/main/java/fr/xephi/authme/settings/domain/Comment.java @@ -12,6 +12,6 @@ import java.lang.annotation.Target; @Target(ElementType.FIELD) public @interface Comment { - String[] value(); + String[] value(); } diff --git a/src/main/java/fr/xephi/authme/settings/propertymap/Node.java b/src/main/java/fr/xephi/authme/settings/propertymap/Node.java index 70d8ce239..5586a578f 100644 --- a/src/main/java/fr/xephi/authme/settings/propertymap/Node.java +++ b/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); diff --git a/src/main/java/fr/xephi/authme/settings/propertymap/PropertyMapComparator.java b/src/main/java/fr/xephi/authme/settings/propertymap/PropertyMapComparator.java index 77f5f60ee..a7ec1cb3b 100644 --- a/src/main/java/fr/xephi/authme/settings/propertymap/PropertyMapComparator.java +++ b/src/main/java/fr/xephi/authme/settings/propertymap/PropertyMapComparator.java @@ -23,12 +23,12 @@ final class PropertyMapComparator implements Comparator { * @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()); } } diff --git a/src/test/java/fr/xephi/authme/settings/NewSettingIntegrationTest.java b/src/test/java/fr/xephi/authme/settings/NewSettingIntegrationTest.java index 2cb0197de..b56f125c7 100644 --- a/src/test/java/fr/xephi/authme/settings/NewSettingIntegrationTest.java +++ b/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)); - File file = new File("unused"); + YamlConfiguration configuration = YamlConfiguration.loadConfiguration(copyFileFromResources(COMPLETE_FILE)); + // 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, Object> expectedValues = ImmutableMap., 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> 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 { } } - /** - * Generate a property map with all properties in {@link TestConfiguration}. - * - * @return The generated property map - */ - private 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); - } + @Test + @Ignore + // TODO #603: Un-ignore once migration service is passed to settings + public void shouldReloadSettings() throws IOException { + // given + YamlConfiguration configuration = YamlConfiguration.loadConfiguration(temporaryFolder.newFile()); + File fullConfigFile = copyFileFromResources(COMPLETE_FILE); + NewSetting settings = new NewSetting(configuration, fullConfigFile, null); + + // when + assertThat(settings.getProperty(TestConfiguration.RATIO_ORDER), + equalTo(TestConfiguration.RATIO_ORDER.getDefaultValue())); + settings.reload(); + + // 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; } } diff --git a/src/test/java/fr/xephi/authme/settings/NewSettingTest.java b/src/test/java/fr/xephi/authme/settings/NewSettingTest.java index 710698e5a..047289808 100644 --- a/src/test/java/fr/xephi/authme/settings/NewSettingTest.java +++ b/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 void setReturnValue(YamlConfiguration config, Property property, T value) { if (value instanceof String) { when(config.getString(eq(property.getPath()), anyString())).thenReturn((String) value); diff --git a/src/test/java/fr/xephi/authme/settings/properties/SettingsClassConsistencyTest.java b/src/test/java/fr/xephi/authme/settings/properties/SettingsClassConsistencyTest.java index 075103345..c98dd7d0e 100644 --- a/src/test/java/fr/xephi/authme/settings/properties/SettingsClassConsistencyTest.java +++ b/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(); + } } } diff --git a/src/test/java/fr/xephi/authme/settings/properties/TestConfiguration.java b/src/test/java/fr/xephi/authme/settings/properties/TestConfiguration.java index 72960f6c4..426a69ef0 100644 --- a/src/test/java/fr/xephi/authme/settings/properties/TestConfiguration.java +++ b/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; + } + }