Fix unit tests for settings by providing custom setting migration services

- Allow to provide custom plugin folder to test constructor
- #603 test all NewSetting methods

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

@@ -63,15 +63,16 @@ public class NewSetting {
      *
      * @param configuration The FileConfiguration object to use
      * @param configFile The file to write to
+     * @param pluginFolder The plugin folder
      * @param propertyMap The property map whose properties should be verified for presence, or null to skip this
      * @param migrationService Migration service, or null to skip migration checks
      */
     @VisibleForTesting
-    NewSetting(FileConfiguration configuration, File configFile, PropertyMap propertyMap,
+    NewSetting(FileConfiguration configuration, File configFile, File pluginFolder, PropertyMap propertyMap,
                SettingsMigrationService migrationService) {
         this.configuration = configuration;
         this.configFile = configFile;
-        this.pluginFolder = new File("");
+        this.pluginFolder = pluginFolder;
         this.propertyMap = propertyMap;
         this.migrationService = migrationService;
 
@@ -210,18 +211,20 @@ public class NewSetting {
 
     private File buildMessagesFile() {
         String languageCode = getProperty(PluginSettings.MESSAGES_LANGUAGE);
-        File messagesFile = buildMessagesFileFromCode(languageCode);
+
-        if (messagesFile.exists()) {
+        String filePath = buildMessagesFilePathFromCode(languageCode);
+        File messagesFile = new File(pluginFolder, filePath);
+        if (copyFileFromResource(messagesFile, filePath)) {
             return messagesFile;
         }
 
-        return copyFileFromResource(messagesFile, buildMessagesFilePathFromCode(languageCode))
+        // File doesn't exist or couldn't be copied - try again with default, "en"
-            ? messagesFile
+        String defaultFilePath = buildMessagesFilePathFromCode("en");
-            : buildMessagesFileFromCode("en");
+        File defaultFile = new File(pluginFolder, defaultFilePath);
-    }
+        copyFileFromResource(defaultFile, defaultFilePath);
 
-    private File buildMessagesFileFromCode(String language) {
+        // No matter the result, need to return a file
-        return new File(pluginFolder, buildMessagesFilePathFromCode(language));
+        return defaultFile;
     }
 
     private static String buildMessagesFilePathFromCode(String language) {
@@ -248,7 +251,7 @@ public class NewSetting {
         final Charset charset = Charset.forName("UTF-8");
         if (copyFileFromResource(emailFile, "email.html")) {
             try {
-                return StringUtils.join("", Files.readLines(emailFile, charset));
+                return StringUtils.join("\n", Files.readLines(emailFile, charset));
             } catch (IOException e) {
                 ConsoleLogger.logException("Failed to read file '" + emailFile.getPath() + "':", e);
             }

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

@@ -31,7 +31,7 @@ public class ConfigFileConsistencyTest {
     private static final String CONFIG_FILE = "/config.yml";
 
     @Test
-    public void shouldHaveAllConfigs() throws IOException {
+    public void shouldHaveAllConfigs() throws IOException { // TODO ljacqu: How is this different from SettingsMigrationServiceTest?
         // given
         File configFile = TestHelper.getJarFile(CONFIG_FILE);
         FileConfiguration configuration = YamlConfiguration.loadConfiguration(configFile);

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

@@ -8,8 +8,8 @@ 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 org.bukkit.configuration.file.FileConfiguration;
 import org.bukkit.configuration.file.YamlConfiguration;
+import org.junit.Before;
 import org.junit.BeforeClass;
 import org.junit.Rule;
 import org.junit.Test;
@@ -22,12 +22,10 @@ import java.util.Collections;
 import java.util.List;
 import java.util.Map;
 
+import static fr.xephi.authme.settings.TestSettingsMigrationServices.checkAllPropertiesPresent;
 import static fr.xephi.authme.settings.domain.Property.newProperty;
 import static org.hamcrest.Matchers.equalTo;
 import static org.junit.Assert.assertThat;
-import static org.mockito.BDDMockito.given;
-import static org.mockito.Matchers.any;
-import static org.mockito.Mockito.mock;
 
 /**
  * Integration test for {@link NewSetting}.
@@ -46,11 +44,18 @@ public class NewSettingIntegrationTest {
     @Rule
     public TemporaryFolder temporaryFolder = new TemporaryFolder();
 
+    private File testPluginFolder;
+
     @BeforeClass
     public static void setUpLogger() {
         ConsoleLoggerTestInitializer.setupLogger();
     }
 
+    @Before
+    public void setUpTestPluginFolder() throws IOException {
+        testPluginFolder = temporaryFolder.newFolder();
+    }
+
     @Test
     public void shouldLoadAndReadAllProperties() throws IOException {
         // given
@@ -59,7 +64,8 @@ public class NewSettingIntegrationTest {
         File newFile = temporaryFolder.newFile();
 
         // when / then
-        NewSetting settings = new NewSetting(configuration, newFile, propertyMap, new PlainSettingsMigrationService());
+        NewSetting settings = new NewSetting(configuration, newFile, testPluginFolder, propertyMap,
+            checkAllPropertiesPresent());
         Map<Property<?>, Object> expectedValues = ImmutableMap.<Property<?>, Object>builder()
             .put(TestConfiguration.DURATION_IN_SECONDS, 22)
             .put(TestConfiguration.SYSTEM_NAME, "Custom sys name")
@@ -85,13 +91,14 @@ public class NewSettingIntegrationTest {
         File file = copyFileFromResources(INCOMPLETE_FILE);
         YamlConfiguration configuration = YamlConfiguration.loadConfiguration(file);
         // Expectation: File is rewritten to since it does not have all configurations
-        new NewSetting(configuration, file, propertyMap, new PlainSettingsMigrationService());
+        new NewSetting(configuration, file, testPluginFolder, propertyMap, checkAllPropertiesPresent());
 
         // Load the settings again -> checks that what we wrote can be loaded again
         configuration = YamlConfiguration.loadConfiguration(file);
 
         // then
-        NewSetting settings = new NewSetting(configuration, file, propertyMap, new PlainSettingsMigrationService());
+        NewSetting settings = new NewSetting(configuration, file, testPluginFolder, propertyMap,
+            checkAllPropertiesPresent());
         Map<Property<?>, Object> expectedValues = ImmutableMap.<Property<?>, Object>builder()
             .put(TestConfiguration.DURATION_IN_SECONDS, 22)
             .put(TestConfiguration.SYSTEM_NAME, "[TestDefaultValue]")
@@ -122,20 +129,20 @@ public class NewSettingIntegrationTest {
             newProperty("more.string1", "it's a text with some \\'apostrophes'"),
             newProperty("more.string2", "\tthis one\nhas some\nnew '' lines-test")
         );
-        PropertyMap propertyMap = TestConfiguration.generatePropertyMap();
         for (Property<?> property : additionalProperties) {
             propertyMap.put(property, new String[0]);
         }
 
         // when
-        new NewSetting(configuration, file, propertyMap, new PlainSettingsMigrationService());
+        new NewSetting(configuration, file, testPluginFolder, propertyMap, checkAllPropertiesPresent());
         // reload the file as settings should have been rewritten
         configuration = YamlConfiguration.loadConfiguration(file);
 
         // then
         // assert that we won't rewrite the settings again! One rewrite should produce a valid, complete configuration
         File unusedFile = new File("config-difficult-values.unused.yml");
-        NewSetting settings = new NewSetting(configuration, unusedFile, propertyMap, new PlainSettingsMigrationService());
+        NewSetting settings = new NewSetting(configuration, unusedFile, testPluginFolder, propertyMap,
+            checkAllPropertiesPresent());
         assertThat(unusedFile.exists(), equalTo(false));
         assertThat(configuration.contains(TestConfiguration.DUST_LEVEL.getPath()), equalTo(true));
 
@@ -165,11 +172,8 @@ public class NewSettingIntegrationTest {
         // given
         YamlConfiguration configuration = YamlConfiguration.loadConfiguration(temporaryFolder.newFile());
         File fullConfigFile = copyFileFromResources(COMPLETE_FILE);
-        SettingsMigrationService migrationService = mock(SettingsMigrationService.class);
+        NewSetting settings = new NewSetting(configuration, fullConfigFile, testPluginFolder, propertyMap,
-        given(migrationService.checkAndMigrate(any(FileConfiguration.class), any(PropertyMap.class), any(File.class)))
+            TestSettingsMigrationServices.alwaysFulfilled());
-            .willReturn(false);
-        NewSetting settings = new NewSetting(configuration, fullConfigFile, TestConfiguration.generatePropertyMap(),
-            new PlainSettingsMigrationService());
 
         // when
         assertThat(settings.getProperty(TestConfiguration.RATIO_ORDER),

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

@@ -2,9 +2,11 @@ package fr.xephi.authme.settings;
 
 import fr.xephi.authme.ConsoleLoggerTestInitializer;
 import fr.xephi.authme.settings.domain.Property;
+import fr.xephi.authme.settings.properties.RegistrationSettings;
 import fr.xephi.authme.settings.properties.TestConfiguration;
 import fr.xephi.authme.settings.properties.TestEnum;
 import org.bukkit.configuration.file.YamlConfiguration;
+import org.junit.Before;
 import org.junit.BeforeClass;
 import org.junit.Rule;
 import org.junit.Test;
@@ -14,10 +16,15 @@ import org.mockito.internal.stubbing.answers.ReturnsArgumentAt;
 import java.io.File;
 import java.io.IOException;
 import java.io.InputStream;
+import java.nio.file.Files;
+import java.util.List;
 
 import static fr.xephi.authme.settings.properties.PluginSettings.MESSAGES_LANGUAGE;
+import static fr.xephi.authme.util.StringUtils.makePath;
 import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.endsWith;
 import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.hasSize;
 import static org.hamcrest.Matchers.not;
 import static org.hamcrest.Matchers.nullValue;
 import static org.mockito.BDDMockito.given;
@@ -37,12 +44,18 @@ public class NewSettingTest {
 
     @Rule
     public TemporaryFolder temporaryFolder = new TemporaryFolder();
+    private File testPluginFolder;
 
     @BeforeClass
     public static void setUpLogger() {
         ConsoleLoggerTestInitializer.setupLogger();
     }
 
+    @Before
+    public void setUpTestPluginFolder() throws IOException {
+        testPluginFolder = temporaryFolder.newFolder();
+    }
+
     @Test
     public void shouldLoadAllConfigs() {
         // given
@@ -58,7 +71,7 @@ public class NewSettingTest {
         setReturnValue(configuration, TestConfiguration.SYSTEM_NAME, "myTestSys");
 
         // when / then
-        NewSetting settings = new NewSetting(configuration, null, null, null);
+        NewSetting settings = new NewSetting(configuration, null, null, null, null);
 
         assertThat(settings.getProperty(TestConfiguration.VERSION_NUMBER), equalTo(20));
         assertThat(settings.getProperty(TestConfiguration.SKIP_BORING_FEATURES), equalTo(true));
@@ -74,7 +87,7 @@ public class NewSettingTest {
     public void shouldReturnDefaultFile() throws IOException {
         // given
         YamlConfiguration configuration = mock(YamlConfiguration.class);
-        NewSetting settings = new NewSetting(configuration, null, null, null);
+        NewSetting settings = new NewSetting(configuration, null, null, null, null);
 
         // when
         String defaultFile = settings.getDefaultMessagesFile();
@@ -90,7 +103,7 @@ public class NewSettingTest {
     public void shouldSetProperty() {
         // given
         YamlConfiguration configuration = mock(YamlConfiguration.class);
-        NewSetting settings = new NewSetting(configuration, null, null, null);
+        NewSetting settings = new NewSetting(configuration, null, null, null, null);
 
         // when
         settings.setProperty(TestConfiguration.DUST_LEVEL, -4);
@@ -102,17 +115,81 @@ public class NewSettingTest {
     @Test
     public void shouldReturnMessagesFile() {
         // given
+        // Use some code that is for sure not present in our JAR
+        String languageCode = "notinjar";
+        File file = new File(testPluginFolder, makePath("messages", "messages_" + languageCode + ".yml"));
+        createFile(file);
+
         YamlConfiguration configuration = mock(YamlConfiguration.class);
         given(configuration.contains(anyString())).willReturn(true);
-        given(configuration.getString(eq(MESSAGES_LANGUAGE.getPath()), anyString())).willReturn("fr");
+        setReturnValue(configuration, MESSAGES_LANGUAGE, languageCode);
-        NewSetting settings = new NewSetting(configuration, null, TestConfiguration.generatePropertyMap(),
+        NewSetting settings = new NewSetting(configuration, null, testPluginFolder,
-            new PlainSettingsMigrationService());
+            TestConfiguration.generatePropertyMap(), TestSettingsMigrationServices.alwaysFulfilled());
 
         // when
         File messagesFile = settings.getMessagesFile();
 
         // then
-        System.out.println(messagesFile.getPath());
+        assertThat(messagesFile.getPath(), endsWith("messages_" + languageCode + ".yml"));
+        assertThat(messagesFile.exists(), equalTo(true));
+    }
+
+    @Test
+    public void shouldCopyDefaultForUnknownLanguageCode() {
+        // given
+        YamlConfiguration configuration = mock(YamlConfiguration.class);
+        given(configuration.contains(anyString())).willReturn(true);
+        setReturnValue(configuration, MESSAGES_LANGUAGE, "doesntexist");
+        NewSetting settings = new NewSetting(configuration, null, testPluginFolder,
+            TestConfiguration.generatePropertyMap(), TestSettingsMigrationServices.alwaysFulfilled());
+
+        // when
+        File messagesFile = settings.getMessagesFile();
+
+        // then
+        assertThat(messagesFile.getPath(), endsWith("messages_en.yml"));
+        assertThat(messagesFile.exists(), equalTo(true));
+    }
+
+    @Test
+    public void shouldLoadWelcomeMessage() throws IOException {
+        // given
+        String welcomeMessage = "This is my welcome message for testing\nBye!";
+        File welcomeFile = new File(testPluginFolder, "welcome.txt");
+        createFile(welcomeFile);
+        Files.write(welcomeFile.toPath(), welcomeMessage.getBytes());
+
+        YamlConfiguration configuration = mock(YamlConfiguration.class);
+        setReturnValue(configuration, RegistrationSettings.USE_WELCOME_MESSAGE, true);
+        NewSetting settings = new NewSetting(configuration, null, testPluginFolder,
+            TestConfiguration.generatePropertyMap(), TestSettingsMigrationServices.alwaysFulfilled());
+
+        // when
+        List<String> result = settings.getWelcomeMessage();
+
+        // then
+        assertThat(result, hasSize(2));
+        assertThat(result.get(0), equalTo(welcomeMessage.split("\\n")[0]));
+        assertThat(result.get(1), equalTo(welcomeMessage.split("\\n")[1]));
+    }
+
+    @Test
+    public void shouldLoadEmailMessage() throws IOException {
+        // given
+        String emailMessage = "Sample email message\nThat's all!";
+        File emailFile = new File(testPluginFolder, "email.html");
+        createFile(emailFile);
+        Files.write(emailFile.toPath(), emailMessage.getBytes());
+
+        YamlConfiguration configuration = mock(YamlConfiguration.class);
+        NewSetting settings = new NewSetting(configuration, null, testPluginFolder,
+            TestConfiguration.generatePropertyMap(), TestSettingsMigrationServices.alwaysFulfilled());
+
+        // when
+        String result = settings.getEmailMessage();
+
+        // then
+        assertThat(result, equalTo(emailMessage));
     }
 
     private static <T> void setReturnValue(YamlConfiguration config, Property<T> property, T value) {
@@ -135,4 +212,13 @@ public class NewSettingTest {
             setting.getProperty(property).equals(property.getDefaultValue()), equalTo(true));
     }
 
+    private static void createFile(File file) {
+        try {
+            file.getParentFile().mkdirs();
+            file.createNewFile();
+        } catch (IOException e) {
+            throw new IllegalStateException(e);
+        }
+    }
+
 }

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

@@ -1,20 +0,0 @@
-package fr.xephi.authme.settings;
-
-import fr.xephi.authme.settings.propertymap.PropertyMap;
-import org.bukkit.configuration.file.FileConfiguration;
-
-import java.io.File;
-
-/**
- * Simple settings migration service simply returning {@code true} if all properties are present,
- * {@code false} otherwise.
- */
-public class PlainSettingsMigrationService extends SettingsMigrationService {
-
-    // See parent javadoc: true = some migration had to be done, false = config file is up-to-date
-    @Override
-    public boolean checkAndMigrate(FileConfiguration configuration, PropertyMap propertyMap, File pluginFolder) {
-        return !super.containsAllSettings(configuration, propertyMap);
-    }
-
-}

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

@@ -0,0 +1,49 @@
+package fr.xephi.authme.settings;
+
+import fr.xephi.authme.settings.propertymap.PropertyMap;
+import org.bukkit.configuration.file.FileConfiguration;
+
+import java.io.File;
+
+/**
+ * Provides {@link SettingsMigrationService} implementations for testing.
+ */
+public final class TestSettingsMigrationServices {
+
+    private TestSettingsMigrationServices() {
+    }
+
+    /**
+     * Returns a settings migration service which always answers that all data is up-to-date.
+     *
+     * @return test settings migration service
+     */
+    public static SettingsMigrationService alwaysFulfilled() {
+        return new SettingsMigrationService() {
+            @Override
+            public boolean checkAndMigrate(FileConfiguration configuration, PropertyMap propertyMap, File pluginFolder) {
+                return false;
+            }
+            @Override
+            public boolean containsAllSettings(FileConfiguration configuration, PropertyMap propertyMap) {
+                return true;
+            }
+        };
+    }
+
+    /**
+     * Returns a simple settings migration service which is fulfilled if all properties are present.
+     *
+     * @return test settings migration service
+     */
+    public static SettingsMigrationService checkAllPropertiesPresent() {
+        return new SettingsMigrationService() {
+            // See parent javadoc: true = some migration had to be done, false = config file is up-to-date
+            @Override
+            public boolean checkAndMigrate(FileConfiguration configuration, PropertyMap propertyMap, File pluginFolder) {
+                return !super.containsAllSettings(configuration, propertyMap);
+            }
+        };
+    }
+
+}