From 41890b9440593802e7a8c8612bdf7ffb185d83f3 Mon Sep 17 00:00:00 2001 From: Paul Campbell Date: Thu, 1 Nov 2018 17:49:54 +0000 Subject: [PATCH 01/18] Fix for SUREFIRE-1588 Latest version of Java 1.8.0_191 enforces that Manifest classpath entries be relative. https://issues.apache.org/jira/browse/SUREFIRE-1588 --- parent/pom.xml | 1 + 1 file changed, 1 insertion(+) diff --git a/parent/pom.xml b/parent/pom.xml index e1b3737d36..d6feccfc23 100644 --- a/parent/pom.xml +++ b/parent/pom.xml @@ -1385,6 +1385,7 @@ maven-surefire-plugin ${surefire.version} + false listener From 9f89562caf16b58154c31366d89f54b8bebced21 Mon Sep 17 00:00:00 2001 From: Paul Campbell Date: Mon, 5 Nov 2018 10:50:46 +0000 Subject: [PATCH 02/18] [rt-felix] embedded framework test survives teardown When deleting files during teardown, if a temporary file can't be deleted then the test should not fail for that reason. Log that the file could not be delete and carry on. --- .../brooklyn/rt/felix/EmbeddedFelixFrameworkTest.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/utils/rt-felix/src/test/java/org/apache/brooklyn/rt/felix/EmbeddedFelixFrameworkTest.java b/utils/rt-felix/src/test/java/org/apache/brooklyn/rt/felix/EmbeddedFelixFrameworkTest.java index 11dfe2777c..cde02b1a51 100644 --- a/utils/rt-felix/src/test/java/org/apache/brooklyn/rt/felix/EmbeddedFelixFrameworkTest.java +++ b/utils/rt-felix/src/test/java/org/apache/brooklyn/rt/felix/EmbeddedFelixFrameworkTest.java @@ -66,7 +66,11 @@ public static void tearDownOsgiFramework(Framework framework, File storageTempDi EmbeddedFelixFramework.stopFramework(framework); framework = null; if (storageTempDir != null) { - FileUtils.deleteDirectory(storageTempDir); + try { + FileUtils.deleteDirectory(storageTempDir); + } catch (IOException e) { + log.warn(e.getMessage()); + } storageTempDir = null; } } From fbb6a6f2e678dcaac66a7ad15de701f856823965 Mon Sep 17 00:00:00 2001 From: Paul Campbell Date: Mon, 5 Nov 2018 14:05:18 +0000 Subject: [PATCH 03/18] [core] embedded framework test survives teardown When deleting files during teardown, if a temporary file can't be deleted then the test should not fail for that reason. Log that the file could not be delete and carry on. --- .../apache/brooklyn/util/core/osgi/OsgiTestBase.java | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/core/src/test/java/org/apache/brooklyn/util/core/osgi/OsgiTestBase.java b/core/src/test/java/org/apache/brooklyn/util/core/osgi/OsgiTestBase.java index 4bc1e583c3..31b8065661 100644 --- a/core/src/test/java/org/apache/brooklyn/util/core/osgi/OsgiTestBase.java +++ b/core/src/test/java/org/apache/brooklyn/util/core/osgi/OsgiTestBase.java @@ -28,6 +28,8 @@ import org.osgi.framework.Bundle; import org.osgi.framework.BundleException; import org.osgi.framework.launch.Framework; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import org.testng.annotations.AfterMethod; import org.testng.annotations.BeforeMethod; @@ -37,6 +39,8 @@ */ public class OsgiTestBase { + private static final Logger log = LoggerFactory.getLogger(OsgiTestBase.class); + public static final String BROOKLYN_OSGI_TEST_A_0_1_0_PATH = OsgiTestResources.BROOKLYN_OSGI_TEST_A_0_1_0_PATH; public static final String BROOKLYN_OSGI_TEST_A_0_1_0_URL = "classpath:"+BROOKLYN_OSGI_TEST_A_0_1_0_PATH; @@ -75,7 +79,11 @@ public static void tearDownOsgiFramework(Framework framework, File storageTempDi Osgis.ungetFramework(framework); framework = null; if (storageTempDir != null) { - FileUtils.deleteDirectory(storageTempDir); + try { + FileUtils.deleteDirectory(storageTempDir); + } catch (IOException e) { + log.warn(e.getMessage()); + } storageTempDir = null; } } From c2af743b6c7daa392b1301946e9b5632b4a61fc1 Mon Sep 17 00:00:00 2001 From: Paul Campbell Date: Mon, 5 Nov 2018 22:03:15 +0000 Subject: [PATCH 04/18] [utils] Extract FileUtil.deleteDirectory() from duplicated code --- .../brooklyn/util/core/osgi/OsgiTestBase.java | 21 ++++++------------- .../org/apache/brooklyn/util/io/FileUtil.java | 17 +++++++++++++++ .../rt/felix/EmbeddedFelixFrameworkTest.java | 18 ++++------------ 3 files changed, 27 insertions(+), 29 deletions(-) diff --git a/core/src/test/java/org/apache/brooklyn/util/core/osgi/OsgiTestBase.java b/core/src/test/java/org/apache/brooklyn/util/core/osgi/OsgiTestBase.java index 31b8065661..4bcadbde0b 100644 --- a/core/src/test/java/org/apache/brooklyn/util/core/osgi/OsgiTestBase.java +++ b/core/src/test/java/org/apache/brooklyn/util/core/osgi/OsgiTestBase.java @@ -16,15 +16,14 @@ package org.apache.brooklyn.util.core.osgi; import java.io.File; -import java.io.IOException; import org.apache.brooklyn.api.mgmt.ManagementContext; import org.apache.brooklyn.core.mgmt.internal.ManagementContextInternal; import org.apache.brooklyn.test.support.TestResourceUnavailableException; import org.apache.brooklyn.util.exceptions.Exceptions; +import org.apache.brooklyn.util.io.FileUtil; import org.apache.brooklyn.util.os.Os; import org.apache.brooklyn.util.osgi.OsgiTestResources; -import org.apache.commons.io.FileUtils; import org.osgi.framework.Bundle; import org.osgi.framework.BundleException; import org.osgi.framework.launch.Framework; @@ -44,7 +43,7 @@ public class OsgiTestBase { public static final String BROOKLYN_OSGI_TEST_A_0_1_0_PATH = OsgiTestResources.BROOKLYN_OSGI_TEST_A_0_1_0_PATH; public static final String BROOKLYN_OSGI_TEST_A_0_1_0_URL = "classpath:"+BROOKLYN_OSGI_TEST_A_0_1_0_PATH; - protected Bundle install(String url) throws BundleException { + protected Bundle install(String url) { try { return Osgis.install(framework, url); } catch (Exception e) { @@ -52,7 +51,7 @@ protected Bundle install(String url) throws BundleException { } } - protected Bundle installFromClasspath(String resourceName) throws BundleException { + protected Bundle installFromClasspath(String resourceName) { TestResourceUnavailableException.throwIfResourceUnavailable(getClass(), resourceName); try { return Osgis.install(framework, String.format("classpath:%s", resourceName)); @@ -71,21 +70,13 @@ public void setUp() throws Exception { } @AfterMethod(alwaysRun = true) - public void tearDown() throws BundleException, IOException, InterruptedException { + public void tearDown() { tearDownOsgiFramework(framework, storageTempDir); } - public static void tearDownOsgiFramework(Framework framework, File storageTempDir) throws BundleException, InterruptedException, IOException { + public static void tearDownOsgiFramework(Framework framework, File storageTempDir) { Osgis.ungetFramework(framework); - framework = null; - if (storageTempDir != null) { - try { - FileUtils.deleteDirectory(storageTempDir); - } catch (IOException e) { - log.warn(e.getMessage()); - } - storageTempDir = null; - } + FileUtil.deleteDirectory(storageTempDir); } public static void preinstallLibrariesLowLevelToPreventCatalogBomParsing(ManagementContext mgmt, String ...libraries) { diff --git a/utils/common/src/main/java/org/apache/brooklyn/util/io/FileUtil.java b/utils/common/src/main/java/org/apache/brooklyn/util/io/FileUtil.java index a4908f2498..176ad821a5 100644 --- a/utils/common/src/main/java/org/apache/brooklyn/util/io/FileUtil.java +++ b/utils/common/src/main/java/org/apache/brooklyn/util/io/FileUtil.java @@ -184,4 +184,21 @@ private static boolean createNewFile(File file) throws IOException { } return file.createNewFile(); } + + /** + * Recursively delete a directory. + * + *

Any IOException that might be raised is logged and not thrown.

+ * + * @param file The directory to be deleted + */ + public static void deleteDirectory(final File file) { + if (file != null) { + try { + FileUtils.deleteDirectory(file); + } catch (IOException e) { + LOG.warn(e.getMessage()); + } + } + } } diff --git a/utils/rt-felix/src/test/java/org/apache/brooklyn/rt/felix/EmbeddedFelixFrameworkTest.java b/utils/rt-felix/src/test/java/org/apache/brooklyn/rt/felix/EmbeddedFelixFrameworkTest.java index cde02b1a51..0c837cd8b4 100644 --- a/utils/rt-felix/src/test/java/org/apache/brooklyn/rt/felix/EmbeddedFelixFrameworkTest.java +++ b/utils/rt-felix/src/test/java/org/apache/brooklyn/rt/felix/EmbeddedFelixFrameworkTest.java @@ -16,7 +16,6 @@ package org.apache.brooklyn.rt.felix; import java.io.File; -import java.io.IOException; import java.io.InputStream; import java.net.URL; import java.util.Enumeration; @@ -25,11 +24,10 @@ import org.apache.brooklyn.test.support.TestResourceUnavailableException; import org.apache.brooklyn.util.collections.MutableSet; +import org.apache.brooklyn.util.io.FileUtil; import org.apache.brooklyn.util.os.Os; import org.apache.brooklyn.util.osgi.OsgiTestResources; import org.apache.brooklyn.util.stream.Streams; -import org.apache.commons.io.FileUtils; -import org.osgi.framework.BundleException; import org.osgi.framework.launch.Framework; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -58,21 +56,13 @@ public void setUp() throws Exception { } @AfterMethod(alwaysRun = true) - public void tearDown() throws BundleException, IOException, InterruptedException { + public void tearDown() { tearDownOsgiFramework(framework, storageTempDir); } - public static void tearDownOsgiFramework(Framework framework, File storageTempDir) throws BundleException, InterruptedException, IOException { + private static void tearDownOsgiFramework(Framework framework, File storageTempDir) { EmbeddedFelixFramework.stopFramework(framework); - framework = null; - if (storageTempDir != null) { - try { - FileUtils.deleteDirectory(storageTempDir); - } catch (IOException e) { - log.warn(e.getMessage()); - } - storageTempDir = null; - } + FileUtil.deleteDirectory(storageTempDir); } @Test From 18f65e1ebffa6bfc55bc2f706098747296ec429c Mon Sep 17 00:00:00 2001 From: Paul Campbell Date: Mon, 5 Nov 2018 22:06:18 +0000 Subject: [PATCH 05/18] [core] Fix syntax for maven-compiler plugin --- core/pom.xml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/pom.xml b/core/pom.xml index e7e7236201..28de44e9a6 100644 --- a/core/pom.xml +++ b/core/pom.xml @@ -261,9 +261,9 @@ groovy-eclipse-compiler - + **/*.groovy - + true false ${java.version} From c5a851d626cf5069fe06c9dceac174f238892dbb Mon Sep 17 00:00:00 2001 From: Paul Campbell Date: Tue, 6 Nov 2018 10:15:26 +0000 Subject: [PATCH 06/18] [test-support] Add @DisableOnWindows annotation for TestNG tests --- parent/pom.xml | 1 + .../brooklyn/test/DisableOnWindows.java | 40 +++++++++++++ .../test/DisableOnWindowsListener.java | 57 +++++++++++++++++++ .../services/org.testng.ITestNGListener | 1 + .../brooklyn/test/DisableOnWindowsTest.java | 41 +++++++++++++ 5 files changed, 140 insertions(+) create mode 100644 test-support/src/main/java/org/apache/brooklyn/test/DisableOnWindows.java create mode 100644 test-support/src/main/java/org/apache/brooklyn/test/DisableOnWindowsListener.java create mode 100644 test-support/src/main/resources/META-INF/services/org.testng.ITestNGListener create mode 100644 test-support/src/test/java/org/apache/brooklyn/test/DisableOnWindowsTest.java diff --git a/parent/pom.xml b/parent/pom.xml index d6feccfc23..971c76ebaa 100644 --- a/parent/pom.xml +++ b/parent/pom.xml @@ -1110,6 +1110,7 @@ **/src/main/license/** **/src/test/license/** **/MANIFEST.MF + **/META-INF/services/** **/test-output/** **/*.pem.pub **/*.pem diff --git a/test-support/src/main/java/org/apache/brooklyn/test/DisableOnWindows.java b/test-support/src/main/java/org/apache/brooklyn/test/DisableOnWindows.java new file mode 100644 index 0000000000..9fa2998cdd --- /dev/null +++ b/test-support/src/main/java/org/apache/brooklyn/test/DisableOnWindows.java @@ -0,0 +1,40 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.brooklyn.test; + +import java.lang.annotation.*; + +/** + * Used to indicate that a test should ne be executed on Windows. + * + *

You must add the following to the class where this annotation is being used: + * {@code @Listeners(DisableOnWindows)}

+ */ +@Retention(RetentionPolicy.RUNTIME) +@Target(ElementType.METHOD) +@Documented +public @interface DisableOnWindows { + + /**3 + * Explain the reason for the test being disabled. + * + *

e.g. "Needs an ssh server listening on port 22 on localhost."

+ */ + String reason(); +} diff --git a/test-support/src/main/java/org/apache/brooklyn/test/DisableOnWindowsListener.java b/test-support/src/main/java/org/apache/brooklyn/test/DisableOnWindowsListener.java new file mode 100644 index 0000000000..743edde9bd --- /dev/null +++ b/test-support/src/main/java/org/apache/brooklyn/test/DisableOnWindowsListener.java @@ -0,0 +1,57 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.brooklyn.test; + +import org.apache.brooklyn.util.os.Os; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.testng.IAnnotationTransformer; +import org.testng.annotations.ITestAnnotation; + +import java.lang.reflect.Constructor; +import java.lang.reflect.Method; + +/** + * Scans all tests annotated with {@link DisableOnWindows} and, on Windows, sets {@code enabled=false} if the current + * environment is Windows.. + */ +public class DisableOnWindowsListener implements IAnnotationTransformer { + + private static final Logger LOG = LoggerFactory.getLogger(DisableOnWindowsListener.class); + + @Override + public void transform( + final ITestAnnotation annotation, + final Class testClass, + final Constructor testConstructor, + final Method testMethod + ) { + if (testMethod != null ) { + final DisableOnWindows disableOnWindows = testMethod.getAnnotation(DisableOnWindows.class); + if (disableOnWindows != null && Os.isMicrosoftWindows()) { + annotation.setEnabled(false); + LOG.info(String.format("Disabled: %s.%s - %s", + testMethod.getDeclaringClass().getName(), + testMethod.getName(), + disableOnWindows.reason())); + } + } + } + +} diff --git a/test-support/src/main/resources/META-INF/services/org.testng.ITestNGListener b/test-support/src/main/resources/META-INF/services/org.testng.ITestNGListener new file mode 100644 index 0000000000..584861a390 --- /dev/null +++ b/test-support/src/main/resources/META-INF/services/org.testng.ITestNGListener @@ -0,0 +1 @@ +org.apache.brooklyn.test.DisableOnWindowsListener \ No newline at end of file diff --git a/test-support/src/test/java/org/apache/brooklyn/test/DisableOnWindowsTest.java b/test-support/src/test/java/org/apache/brooklyn/test/DisableOnWindowsTest.java new file mode 100644 index 0000000000..9929f666ef --- /dev/null +++ b/test-support/src/test/java/org/apache/brooklyn/test/DisableOnWindowsTest.java @@ -0,0 +1,41 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.brooklyn.test; + +import org.apache.brooklyn.util.os.Os; +import org.testng.annotations.Test; + +import static org.apache.brooklyn.test.Asserts.assertTrue; +import static org.apache.brooklyn.test.Asserts.fail; + +public class DisableOnWindowsTest { + + @Test + public void alwaysRun() { + assertTrue(true); + } + + @Test + @DisableOnWindows(reason = "unit test") + public void isDisabledOnWindows() { + if (Os.isMicrosoftWindows()) { + fail("Test should have been disabled on windows"); + } + } +} From 2ba7ce671973fdf49ad099f3c744b2f38807860c Mon Sep 17 00:00:00 2001 From: Paul Campbell Date: Tue, 6 Nov 2018 10:23:50 +0000 Subject: [PATCH 07/18] [core] Disable BashCommandsIntegrationTest test on Windows --- .../core/ssh/BashCommandsIntegrationTest.java | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/core/src/test/java/org/apache/brooklyn/util/core/ssh/BashCommandsIntegrationTest.java b/core/src/test/java/org/apache/brooklyn/util/core/ssh/BashCommandsIntegrationTest.java index 3f99ee3ac2..311bce7d38 100644 --- a/core/src/test/java/org/apache/brooklyn/util/core/ssh/BashCommandsIntegrationTest.java +++ b/core/src/test/java/org/apache/brooklyn/util/core/ssh/BashCommandsIntegrationTest.java @@ -35,6 +35,7 @@ import org.apache.brooklyn.core.test.BrooklynMgmtUnitTestSupport; import org.apache.brooklyn.core.test.entity.LocalManagementContextForTests; +import org.apache.brooklyn.test.DisableOnWindows; import org.apache.brooklyn.util.core.ResourceUtils; import org.apache.brooklyn.util.core.task.BasicExecutionContext; import org.apache.brooklyn.util.core.task.ssh.SshTasks; @@ -133,6 +134,7 @@ public void tearDown() throws Exception { } @Test(groups="Integration") + @DisableOnWindows(reason = "Needs a bash shell available on localhost") public void testRemoveRequireTtyFromSudoersFile() throws Exception { String cmds = BashCommands.dontRequireTtyForSudo(); @@ -154,6 +156,7 @@ public void testRemoveRequireTtyFromSudoersFile() throws Exception { } @Test(groups="Integration") + @DisableOnWindows(reason = "Needs a whoami command available on localhost") public void testSudo() throws Exception { ByteArrayOutputStream outStream = new ByteArrayOutputStream(); ByteArrayOutputStream errStream = new ByteArrayOutputStream(); @@ -177,6 +180,7 @@ public void testDownloadUrl() throws Exception { } @Test(groups="Integration") + @DisableOnWindows(reason = "Needs a bash shell available on localhost") public void testDownloadFirstSuccessfulFile() throws Exception { List cmds = BashCommands.commandsToDownloadUrlsAs( ImmutableList.of(sourceNonExistantFileUrl, sourceFileUrl1, sourceFileUrl2), @@ -188,6 +192,7 @@ public void testDownloadFirstSuccessfulFile() throws Exception { } @Test(groups="Integration") + @DisableOnWindows(reason = "Needs an ssh server listening on port 22 on localhost") public void testDownloadToStdout() throws Exception { ProcessTaskWrapper t = SshTasks.newSshExecTaskFactory(loc, "cd "+destFile.getParentFile().getAbsolutePath(), @@ -199,6 +204,7 @@ public void testDownloadToStdout() throws Exception { } @Test(groups="Integration") + @DisableOnWindows(reason = "Needs an ssh server listening on port 22 on localhost") public void testAlternativesWhereFirstSucceeds() throws Exception { ProcessTaskWrapper t = SshTasks.newSshExecTaskFactory(loc) .add(BashCommands.alternatives(Arrays.asList("echo first", "exit 88"))) @@ -213,6 +219,7 @@ public void testAlternativesWhereFirstSucceeds() throws Exception { } @Test(groups="Integration") + @DisableOnWindows(reason = "Needs an ssh server listening on port 22 on localhost") public void testAlternatives() throws Exception { ProcessTaskWrapper t = SshTasks.newSshExecTaskFactory(loc) .add(BashCommands.alternatives(Arrays.asList("asdfj_no_such_command_1", "exit 88"))) @@ -224,6 +231,7 @@ public void testAlternatives() throws Exception { } @Test(groups="Integration") + @DisableOnWindows(reason = "Needs an ssh server listening on port 22 on localhost") public void testRequireTestHandlesFailure() throws Exception { ProcessTaskWrapper t = SshTasks.newSshExecTaskFactory(loc) .add(BashCommands.requireTest("-f "+sourceNonExistantFile.getPath(), @@ -236,6 +244,7 @@ public void testRequireTestHandlesFailure() throws Exception { } @Test(groups="Integration") + @DisableOnWindows(reason = "Needs an ssh server listening on port 22 on localhost") public void testRequireTestHandlesSuccess() throws Exception { ProcessTaskWrapper t = SshTasks.newSshExecTaskFactory(loc) .add(BashCommands.requireTest("-f "+sourceFile1.getPath(), @@ -247,6 +256,7 @@ public void testRequireTestHandlesSuccess() throws Exception { } @Test(groups="Integration") + @DisableOnWindows(reason = "Needs an ssh server listening on port 22 on localhost") public void testRequireFileHandlesFailure() throws Exception { ProcessTaskWrapper t = SshTasks.newSshExecTaskFactory(loc) .add(BashCommands.requireFile(sourceNonExistantFile.getPath())).newTask(); @@ -260,6 +270,7 @@ public void testRequireFileHandlesFailure() throws Exception { } @Test(groups="Integration") + @DisableOnWindows(reason = "Needs an ssh server listening on port 22 on localhost") public void testRequireFileHandlesSuccess() throws Exception { ProcessTaskWrapper t = SshTasks.newSshExecTaskFactory(loc) .add(BashCommands.requireFile(sourceFile1.getPath())).newTask(); @@ -270,6 +281,7 @@ public void testRequireFileHandlesSuccess() throws Exception { } @Test(groups="Integration") + @DisableOnWindows(reason = "Needs an ssh server listening on port 22 on localhost") public void testRequireFailureExitsImmediately() throws Exception { ProcessTaskWrapper t = SshTasks.newSshExecTaskFactory(loc) .add(BashCommands.requireTest("-f "+sourceNonExistantFile.getPath(), @@ -284,6 +296,7 @@ public void testRequireFailureExitsImmediately() throws Exception { } @Test(groups="Integration") + @DisableOnWindows(reason = "Needs a bash shell available on localhost") public void testPipeMultiline() throws Exception { String output = execRequiringZeroAndReturningStdout(loc, BashCommands.pipeTextTo("hello world\n"+"and goodbye\n", "wc")).get(); @@ -292,6 +305,7 @@ public void testPipeMultiline() throws Exception { } @Test(groups="Integration") + @DisableOnWindows(reason = "Needs a bash shell available on localhost") public void testWaitForFileContentsWhenAbortingOnFail() throws Exception { String fileContent = "mycontents"; String cmd = BashCommands.waitForFileContents(destFile.getAbsolutePath(), fileContent, Duration.ONE_SECOND, true); @@ -305,6 +319,7 @@ public void testWaitForFileContentsWhenAbortingOnFail() throws Exception { } @Test(groups="Integration") + @DisableOnWindows(reason = "Needs a bash shell available on localhost") public void testWaitForFileContentsWhenNotAbortingOnFail() throws Exception { String fileContent = "mycontents"; String cmd = BashCommands.waitForFileContents(destFile.getAbsolutePath(), fileContent, Duration.ONE_SECOND, false); @@ -318,6 +333,7 @@ public void testWaitForFileContentsWhenNotAbortingOnFail() throws Exception { } @Test(groups="Integration") + @DisableOnWindows(reason = "Needs a bash shell available on localhost") public void testWaitForFileContentsWhenContentsAppearAfterStart() throws Exception { String fileContent = "mycontents"; @@ -335,6 +351,7 @@ public void testWaitForFileContentsWhenContentsAppearAfterStart() throws Excepti } @Test(groups="Integration") + @DisableOnWindows(reason = "Needs a bash shell available on localhost") public void testWaitForFileExistsWhenAbortingOnFail() throws Exception { String cmd = BashCommands.waitForFileExists(destFile.getAbsolutePath(), Duration.ONE_SECOND, true); @@ -347,6 +364,7 @@ public void testWaitForFileExistsWhenAbortingOnFail() throws Exception { } @Test(groups="Integration") + @DisableOnWindows(reason = "Needs a bash shell available on localhost") public void testWaitForFileExistsWhenNotAbortingOnFail() throws Exception { String cmd = BashCommands.waitForFileExists(destFile.getAbsolutePath(), Duration.ONE_SECOND, false); @@ -359,6 +377,7 @@ public void testWaitForFileExistsWhenNotAbortingOnFail() throws Exception { } @Test(groups="Integration", dependsOnMethods="testSudo") + @DisableOnWindows(reason = "Needs a bash shell available on localhost") public void testWaitForPortFreeWhenAbortingOnTimeout() throws Exception { ServerSocket serverSocket = openServerSocket(); try { @@ -378,6 +397,7 @@ public void testWaitForPortFreeWhenAbortingOnTimeout() throws Exception { } @Test(groups="Integration", dependsOnMethods="testSudo") + @DisableOnWindows(reason = "Needs a bash shell available on localhost") public void testWaitForPortFreeWhenNotAbortingOnTimeout() throws Exception { ServerSocket serverSocket = openServerSocket(); try { @@ -397,6 +417,7 @@ public void testWaitForPortFreeWhenNotAbortingOnTimeout() throws Exception { } @Test(groups="Integration", dependsOnMethods="testSudo") + @DisableOnWindows(reason = "Needs a bash shell available on localhost") public void testWaitForPortFreeWhenFreedAfterStart() throws Exception { ServerSocket serverSocket = openServerSocket(); try { From 6dddc5d102bcecfeb15f5076fe8e2675c7db76c6 Mon Sep 17 00:00:00 2001 From: Paul Campbell Date: Tue, 6 Nov 2018 15:31:58 +0000 Subject: [PATCH 08/18] [core] Remove unused Logger in OsgiTestBase --- .../java/org/apache/brooklyn/util/core/osgi/OsgiTestBase.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/core/src/test/java/org/apache/brooklyn/util/core/osgi/OsgiTestBase.java b/core/src/test/java/org/apache/brooklyn/util/core/osgi/OsgiTestBase.java index 4bcadbde0b..0569ffb15b 100644 --- a/core/src/test/java/org/apache/brooklyn/util/core/osgi/OsgiTestBase.java +++ b/core/src/test/java/org/apache/brooklyn/util/core/osgi/OsgiTestBase.java @@ -38,8 +38,6 @@ */ public class OsgiTestBase { - private static final Logger log = LoggerFactory.getLogger(OsgiTestBase.class); - public static final String BROOKLYN_OSGI_TEST_A_0_1_0_PATH = OsgiTestResources.BROOKLYN_OSGI_TEST_A_0_1_0_PATH; public static final String BROOKLYN_OSGI_TEST_A_0_1_0_URL = "classpath:"+BROOKLYN_OSGI_TEST_A_0_1_0_PATH; From ed08ff1899f6c25206f3d4dfeedf63acc002b4f8 Mon Sep 17 00:00:00 2001 From: Alex Heneveld Date: Thu, 15 Nov 2018 17:58:27 +0000 Subject: [PATCH 09/18] when getting details, get the raw value then resolve it immediately prevents delays while it tries to resolve things which aren't immediately available --- .../apache/brooklyn/rest/resources/ApplicationResource.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/resources/ApplicationResource.java b/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/resources/ApplicationResource.java index 1b7fe35b25..abe783c271 100644 --- a/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/resources/ApplicationResource.java +++ b/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/resources/ApplicationResource.java @@ -53,6 +53,7 @@ import org.apache.brooklyn.core.config.ConfigPredicates; import org.apache.brooklyn.core.config.ConstraintViolationException; import org.apache.brooklyn.core.entity.Attributes; +import org.apache.brooklyn.core.entity.EntityInternal; import org.apache.brooklyn.core.entity.EntityPredicates; import org.apache.brooklyn.core.entity.lifecycle.Lifecycle; import org.apache.brooklyn.core.entity.trait.Startable; @@ -335,9 +336,10 @@ private EntitySummary addConfigByGlobs(EntitySummary result, Entity entity, List extraConfigGlobs.stream().forEach(g -> { extraConfigPreds.add( ConfigPredicates.nameMatchesGlob(g) ); }); entity.config().findKeysDeclared(Predicates.or(extraConfigPreds)).forEach(key -> { if (Entitlements.isEntitled(mgmt().getEntitlementManager(), Entitlements.SEE_CONFIG, new EntityAndItem(entity, key.getName()))) { - Object v = entity.config().get(key); + Maybe vRaw = ((EntityInternal)entity).config().getRaw(key); + Object v = vRaw.isPresent() ? vRaw.get() : entity.config().get(key); if (v!=null) { - v = resolving(v).preferJson(true).asJerseyOutermostReturnValue(false).raw(true).context(entity).immediately(true).timeout(Duration.ZERO).resolve(); + v = resolving(v, mgmt()).preferJson(true).asJerseyOutermostReturnValue(false).raw(true).context(entity).immediately(true).timeout(Duration.ZERO).resolve(); configs.put(key.getName(), v); } } From 5dbf70de77518c7ad0a3ae3e8520ee48c8833b32 Mon Sep 17 00:00:00 2001 From: Alex Heneveld Date: Thu, 15 Nov 2018 17:59:13 +0000 Subject: [PATCH 10/18] introduce new distinct "application" itemType on catalog BOM files, which don't set template tag --- .../brooklyn/api/catalog/CatalogItem.java | 3 +- .../brooklyn/api/objs/BrooklynObjectType.java | 3 +- .../core/catalog/CatalogPredicates.java | 4 +- .../internal/BasicBrooklynCatalog.java | 6 ++- .../internal/CatalogApplicationItemDto.java | 42 +++++++++++++++++++ .../catalog/internal/CatalogItemBuilder.java | 7 ++++ .../stock/aggregator/AggregationJob.java | 2 +- .../core/plan/XmlPlanToSpecTransformer.java | 2 +- .../failover/PropagatePrimaryEnricher.java | 3 +- .../rest/transform/CatalogTransformer.java | 2 + .../org/apache/brooklyn/cli/ItemLister.java | 2 +- 11 files changed, 68 insertions(+), 8 deletions(-) create mode 100644 core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogApplicationItemDto.java diff --git a/api/src/main/java/org/apache/brooklyn/api/catalog/CatalogItem.java b/api/src/main/java/org/apache/brooklyn/api/catalog/CatalogItem.java index 90eff4bed8..104fdbfdd5 100644 --- a/api/src/main/java/org/apache/brooklyn/api/catalog/CatalogItem.java +++ b/api/src/main/java/org/apache/brooklyn/api/catalog/CatalogItem.java @@ -45,6 +45,7 @@ public interface CatalogItem extends BrooklynObject, Rebindable { public static enum CatalogItemType { TEMPLATE, + APPLICATION, ENTITY, POLICY, ENRICHER, @@ -63,7 +64,7 @@ public static CatalogItemType ofTargetClass(Class type if (Policy.class.isAssignableFrom(type)) return POLICY; if (Enricher.class.isAssignableFrom(type)) return ENRICHER; if (Location.class.isAssignableFrom(type)) return LOCATION; - if (Application.class.isAssignableFrom(type)) return TEMPLATE; + if (Application.class.isAssignableFrom(type)) return APPLICATION; if (Entity.class.isAssignableFrom(type)) return ENTITY; return null; } diff --git a/api/src/main/java/org/apache/brooklyn/api/objs/BrooklynObjectType.java b/api/src/main/java/org/apache/brooklyn/api/objs/BrooklynObjectType.java index f4a175648c..4bde7d6708 100644 --- a/api/src/main/java/org/apache/brooklyn/api/objs/BrooklynObjectType.java +++ b/api/src/main/java/org/apache/brooklyn/api/objs/BrooklynObjectType.java @@ -99,9 +99,10 @@ public static BrooklynObjectType of(CatalogItemType t) { switch (t) { case ENRICHER: return BrooklynObjectType.ENRICHER; case ENTITY: return BrooklynObjectType.ENTITY; + case APPLICATION: return BrooklynObjectType.ENTITY; + case TEMPLATE: return BrooklynObjectType.ENTITY; case LOCATION: return BrooklynObjectType.LOCATION; case POLICY: return BrooklynObjectType.POLICY; - case TEMPLATE: return BrooklynObjectType.ENTITY; default: return BrooklynObjectType.UNKNOWN; } } diff --git a/core/src/main/java/org/apache/brooklyn/core/catalog/CatalogPredicates.java b/core/src/main/java/org/apache/brooklyn/core/catalog/CatalogPredicates.java index 4756007e50..eb44df4ed6 100644 --- a/core/src/main/java/org/apache/brooklyn/core/catalog/CatalogPredicates.java +++ b/core/src/main/java/org/apache/brooklyn/core/catalog/CatalogPredicates.java @@ -141,8 +141,10 @@ public String toString() { public static final Predicate>> IS_TEMPLATE = CatalogPredicates.>isCatalogItemType(CatalogItemType.TEMPLATE); + public static final Predicate>> IS_APPLICATION = + CatalogPredicates.>isCatalogItemType(CatalogItemType.APPLICATION); public static final Predicate>> IS_ENTITY = - CatalogPredicates.>isCatalogItemType(CatalogItemType.ENTITY); + CatalogPredicates.>isCatalogItemType(CatalogItemType.ENTITY); public static final Predicate>> IS_POLICY = CatalogPredicates.>isCatalogItemType(CatalogItemType.POLICY); public static final Predicate>> IS_ENRICHER = diff --git a/core/src/main/java/org/apache/brooklyn/core/catalog/internal/BasicBrooklynCatalog.java b/core/src/main/java/org/apache/brooklyn/core/catalog/internal/BasicBrooklynCatalog.java index 5b98cb0b81..504baeb779 100644 --- a/core/src/main/java/org/apache/brooklyn/core/catalog/internal/BasicBrooklynCatalog.java +++ b/core/src/main/java/org/apache/brooklyn/core/catalog/internal/BasicBrooklynCatalog.java @@ -954,6 +954,9 @@ private void collectCatalogItemsFromItemMetadataBlock(String sourceYaml, Managed if (itemType==CatalogItemType.TEMPLATE) { tags.add(BrooklynTags.CATALOG_TEMPLATE); + itemType = CatalogItemType.APPLICATION; + } + if (itemType==CatalogItemType.APPLICATION) { itemType = CatalogItemType.ENTITY; superTypes.add(Application.class); } @@ -1371,7 +1374,7 @@ private boolean attemptType(String key, CatalogItemType candidateCiType) { } catch (Exception e) { Exceptions.propagateIfFatal(e); // record the error if we have reason to expect this guess to succeed - if (item.containsKey("services") && (candidateCiType==CatalogItemType.ENTITY || candidateCiType==CatalogItemType.TEMPLATE)) { + if (item.containsKey("services") && (candidateCiType==CatalogItemType.ENTITY || candidateCiType==CatalogItemType.APPLICATION || candidateCiType==CatalogItemType.TEMPLATE)) { // explicit services supplied, so plan should have been parseable for an entity or a a service errors.add(e); } else if (catalogItemType!=null && key!=null) { @@ -2079,6 +2082,7 @@ public CatalogItem apply(@Nullable CatalogItemDo item) { dto.setSymbolicName(dto.getJavaType()); switch (dto.getCatalogItemType()) { case TEMPLATE: + case APPLICATION: case ENTITY: dto.setPlanYaml("services: [{ type: "+dto.getJavaType()+" }]"); break; diff --git a/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogApplicationItemDto.java b/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogApplicationItemDto.java new file mode 100644 index 0000000000..eabcd6d19d --- /dev/null +++ b/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogApplicationItemDto.java @@ -0,0 +1,42 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.brooklyn.core.catalog.internal; + +import org.apache.brooklyn.api.entity.Application; +import org.apache.brooklyn.api.entity.EntitySpec; + +public class CatalogApplicationItemDto extends CatalogItemDtoAbstract> { + + @Override + public CatalogItemType getCatalogItemType() { + return CatalogItemType.APPLICATION; + } + + @Override + public Class getCatalogItemJavaType() { + return Application.class; + } + + @SuppressWarnings({ "rawtypes", "unchecked" }) + @Override + public Class> getSpecType() { + return (Class)EntitySpec.class; + } + +} diff --git a/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogItemBuilder.java b/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogItemBuilder.java index f08ef17da2..28f9090233 100644 --- a/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogItemBuilder.java +++ b/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogItemBuilder.java @@ -33,6 +33,7 @@ public static CatalogItemBuilder newItem(CatalogItemType itemType, String sym Preconditions.checkNotNull(itemType, "itemType required"); switch (itemType) { case ENTITY: return newEntity(symbolicName, version); + case APPLICATION: return newApplication(symbolicName, version); case TEMPLATE: return newTemplate(symbolicName, version); case POLICY: return newPolicy(symbolicName, version); case ENRICHER: return newEnricher(symbolicName, version); @@ -47,6 +48,12 @@ public static CatalogItemBuilder newEntity(String symbolic .version(version); } + public static CatalogItemBuilder newApplication(String symbolicName, String version) { + return new CatalogItemBuilder(new CatalogApplicationItemDto()) + .symbolicName(symbolicName) + .version(version); + } + public static CatalogItemBuilder newTemplate(String symbolicName, String version) { return new CatalogItemBuilder(new CatalogTemplateItemDto()) .symbolicName(symbolicName) diff --git a/core/src/main/java/org/apache/brooklyn/enricher/stock/aggregator/AggregationJob.java b/core/src/main/java/org/apache/brooklyn/enricher/stock/aggregator/AggregationJob.java index ed8c00677e..a7027ffba6 100644 --- a/core/src/main/java/org/apache/brooklyn/enricher/stock/aggregator/AggregationJob.java +++ b/core/src/main/java/org/apache/brooklyn/enricher/stock/aggregator/AggregationJob.java @@ -52,7 +52,7 @@ public final class AggregationJob implements Runnable { public static BasicAttributeSensorAndConfigKey>> DASHBOARD_POLICY_HIGHLIGHTS = new BasicAttributeSensorAndConfigKey(List.class, "dashboard.policyHighlights", - "Highlights from policies. List of Masps, where each map should contain text and category"); + "Highlights from policies. List of Maps, where each map should contain text and category"); private final Entity entity; diff --git a/core/src/test/java/org/apache/brooklyn/core/plan/XmlPlanToSpecTransformer.java b/core/src/test/java/org/apache/brooklyn/core/plan/XmlPlanToSpecTransformer.java index 404d4f8205..065cbd488b 100644 --- a/core/src/test/java/org/apache/brooklyn/core/plan/XmlPlanToSpecTransformer.java +++ b/core/src/test/java/org/apache/brooklyn/core/plan/XmlPlanToSpecTransformer.java @@ -84,7 +84,7 @@ public > SpecT c if (item.getCatalogItemType()==CatalogItemType.ENTITY) { return (SpecT)toEntitySpec(parseXml(item.getPlanYaml()), 1); } - if (item.getCatalogItemType()==CatalogItemType.TEMPLATE) { + if (item.getCatalogItemType()==CatalogItemType.TEMPLATE || item.getCatalogItemType()==CatalogItemType.APPLICATION) { return (SpecT)toEntitySpec(parseXml(item.getPlanYaml()), 0); } throw new UnsupportedTypePlanException("Type "+item.getCatalogItemType()+" not supported"); diff --git a/policy/src/main/java/org/apache/brooklyn/policy/failover/PropagatePrimaryEnricher.java b/policy/src/main/java/org/apache/brooklyn/policy/failover/PropagatePrimaryEnricher.java index ca2a99a1b4..6ecdca2409 100644 --- a/policy/src/main/java/org/apache/brooklyn/policy/failover/PropagatePrimaryEnricher.java +++ b/policy/src/main/java/org/apache/brooklyn/policy/failover/PropagatePrimaryEnricher.java @@ -90,6 +90,7 @@ public synchronized void onEvent(SensorEvent event) { Collection> sensorsToRemove = config().get(PROPAGATING); if (sensorsToRemove!=null) { for (Sensor s: sensorsToRemove) { + // TODO - allow strings above also if (s instanceof AttributeSensor) { ((EntityInternal)entity).sensors().remove((AttributeSensor)s); } @@ -119,7 +120,7 @@ protected void addPropagatorEnricher(Entity primary) { Collection> sensorsToPropagate = getConfig(PROPAGATING); if (sensorsToPropagate==null || sensorsToPropagate.isEmpty()) { - log.warn(""); + log.warn("Nothing found to propagate from primary "+primary+" at "+entity); return; } spec.configure(Propagator.PROPAGATING, sensorsToPropagate); diff --git a/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/transform/CatalogTransformer.java b/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/transform/CatalogTransformer.java index 3684a3eb4e..d559a44e0c 100644 --- a/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/transform/CatalogTransformer.java +++ b/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/transform/CatalogTransformer.java @@ -294,6 +294,7 @@ public static CatalogItemSummary catalogItemSummary(BrooklynRestResourceUtils b, try { switch (item.getCatalogItemType()) { case TEMPLATE: + case APPLICATION: case ENTITY: return catalogEntitySummary(b, item, ub); case POLICY: @@ -369,6 +370,7 @@ private static URI getSelfLink(CatalogItem item, UriBuilder ub) { String itemId = item.getId(); switch (item.getCatalogItemType()) { case TEMPLATE: + case APPLICATION: return serviceUriBuilder(ub, CatalogApi.class, "getApplication").build(itemId, item.getVersion()); case ENTITY: return serviceUriBuilder(ub, CatalogApi.class, "getEntity").build(itemId, item.getVersion()); diff --git a/server-cli/src/main/java/org/apache/brooklyn/cli/ItemLister.java b/server-cli/src/main/java/org/apache/brooklyn/cli/ItemLister.java index 6e31e5327c..4c22a6c60e 100644 --- a/server-cli/src/main/java/org/apache/brooklyn/cli/ItemLister.java +++ b/server-cli/src/main/java/org/apache/brooklyn/cli/ItemLister.java @@ -239,7 +239,7 @@ protected Map populateDescriptors() throws MalformedURLException Map itemDescriptor = ItemDescriptors.toItemDescriptor(catalog, item, headingsOnly); itemCount++; - if (item.getCatalogItemType() == CatalogItem.CatalogItemType.ENTITY || item.getCatalogItemType() == CatalogItem.CatalogItemType.TEMPLATE) { + if (item.getCatalogItemType() == CatalogItem.CatalogItemType.ENTITY || item.getCatalogItemType() == CatalogItem.CatalogItemType.TEMPLATE || item.getCatalogItemType() == CatalogItem.CatalogItemType.APPLICATION) { entities.add(itemDescriptor); } else if (item.getCatalogItemType() == CatalogItem.CatalogItemType.POLICY) { policies.add(itemDescriptor); From 44b6e540986a4ee26b47c075b7ebfe66585208fc Mon Sep 17 00:00:00 2001 From: Alex Heneveld Date: Fri, 16 Nov 2018 14:34:39 +0000 Subject: [PATCH 11/18] when editting it is useful to know the source URL this includes that in the payload if different --- .../apache/brooklyn/rest/transform/TypeTransformer.java | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/transform/TypeTransformer.java b/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/transform/TypeTransformer.java index e8bfb5098b..2796acdf3c 100644 --- a/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/transform/TypeTransformer.java +++ b/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/transform/TypeTransformer.java @@ -23,6 +23,7 @@ import java.net.URI; import java.util.Collections; import java.util.Map; +import java.util.Objects; import java.util.Set; import java.util.concurrent.atomic.AtomicInteger; @@ -87,7 +88,11 @@ private static T embellish(T result, RegisteredType item result.setExtraField("template", true); } if (item.getIconUrl()!=null) { - result.setIconUrl(tidyIconLink(b, item, item.getIconUrl(), ub)); + String tidiedUrl = tidyIconLink(b, item, item.getIconUrl(), ub); + result.setIconUrl(tidiedUrl); + if (!Objects.equals(item.getIconUrl(), tidiedUrl)) { + result.setExtraField("iconUrlSource", item.getIconUrl()); + } } if (detail) { From 2174aa48a5c3b942cb757760459efb30987c9cc1 Mon Sep 17 00:00:00 2001 From: Alex Heneveld Date: Sat, 24 Nov 2018 23:11:54 +0000 Subject: [PATCH 12/18] add test to show deep coercion problem this adds a block to config yaml tests that fail both here and in master, in a similar way to some of the other failures introduced here the next PR fixes it --- .../java/org/apache/brooklyn/camp/brooklyn/ConfigYamlTest.java | 2 ++ .../java/org/apache/brooklyn/core/test/entity/TestEntity.java | 1 + 2 files changed, 3 insertions(+) diff --git a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/ConfigYamlTest.java b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/ConfigYamlTest.java index 53a6be84ce..571076d0ab 100644 --- a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/ConfigYamlTest.java +++ b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/ConfigYamlTest.java @@ -293,6 +293,8 @@ public void testDeferredSupplierToConfig() throws Exception { " - $brooklyn:config(\"myOtherConf\")", " test.confSetPlain:", " - $brooklyn:config(\"myOtherConf\")", + " test.confSetStringPlain:", + " - $brooklyn:config(\"myOtherConf\")", " myOtherConf: myOther"); final Entity app = createStartWaitAndLogApplication(yaml); diff --git a/core/src/test/java/org/apache/brooklyn/core/test/entity/TestEntity.java b/core/src/test/java/org/apache/brooklyn/core/test/entity/TestEntity.java index bfada48325..743d5f499f 100644 --- a/core/src/test/java/org/apache/brooklyn/core/test/entity/TestEntity.java +++ b/core/src/test/java/org/apache/brooklyn/core/test/entity/TestEntity.java @@ -67,6 +67,7 @@ public interface TestEntity extends Entity, Startable, EntityLocal, EntityIntern public static final BasicConfigKey CONF_MAP_PLAIN = new BasicConfigKey(Map.class, "test.confMapPlain", "Configuration key that's a plain map", ImmutableMap.of()); public static final BasicConfigKey CONF_LIST_PLAIN = new BasicConfigKey(List.class, "test.confListPlain", "Configuration key that's a plain list", ImmutableList.of()); public static final BasicConfigKey CONF_SET_PLAIN = new BasicConfigKey(Set.class, "test.confSetPlain", "Configuration key that's a plain set", ImmutableSet.of()); + public static final BasicConfigKey> CONF_SET_STRING_PLAIN = new BasicConfigKey>(new TypeToken>() {}, "test.confSetStringPlain", "Configuration key that's a plain set of strings", ImmutableSet.of()); public static final MapConfigKey CONF_MAP_THING = new MapConfigKey(String.class, "test.confMapThing", "Configuration key that's a map thing"); public static final MapConfigKey CONF_MAP_OBJ_THING = new MapConfigKey(Object.class, "test.confMapObjThing", "Configuration key that's a map thing with objects"); public static final ListConfigKey CONF_LIST_THING = new ListConfigKey(String.class, "test.confListThing", "Configuration key that's a list thing"); From fc62d399d75cb6b8c0b679385ebd1130ab87f71c Mon Sep 17 00:00:00 2001 From: Alex Heneveld Date: Sat, 24 Nov 2018 22:56:27 +0000 Subject: [PATCH 13/18] defer to parent key to extract value when overriding, and coerce better when we override we change e.g. the default value or the description, but we don't intend to change how it extracts. without this if we overwrite a StructuredConfigKey we lose its smarts for extracting. also when we coerce on write we didn't treat overwritten StructuredConfigKeys the same as StructuredConfigKeys. also we had overlooked the case where a List containing a Future is being passed to a ConfigKey. this repairs all those cases. --- .../brooklyn/core/config/BasicConfigKey.java | 21 +++++++ .../internal/AbstractConfigMapImpl.java | 55 ++++++++++++++++--- .../location/internal/LocationConfigMap.java | 4 +- 3 files changed, 71 insertions(+), 9 deletions(-) diff --git a/core/src/main/java/org/apache/brooklyn/core/config/BasicConfigKey.java b/core/src/main/java/org/apache/brooklyn/core/config/BasicConfigKey.java index cbcb94c304..5cf9df7066 100644 --- a/core/src/main/java/org/apache/brooklyn/core/config/BasicConfigKey.java +++ b/core/src/main/java/org/apache/brooklyn/core/config/BasicConfigKey.java @@ -39,6 +39,7 @@ import org.apache.brooklyn.util.core.task.ValueResolver; import org.apache.brooklyn.util.exceptions.Exceptions; import org.apache.brooklyn.util.guava.TypeTokens; +import org.apache.brooklyn.util.javalang.JavaClassNames; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -476,5 +477,25 @@ public BasicConfigKeyOverwriting(ConfigKey key, String newDescription, T defa public ConfigKey getParentKey() { return parentKey; } + + @Override + public T extractValue(Map vals, ExecutionContext exec) { + // if we are unsure and want to log both, then + T v1, v2 = null; + if (parentKey instanceof BasicConfigKey) { + v2 = ((BasicConfigKey)parentKey).extractValue(vals, exec); + return v2; + } + v1 = super.extractValue(vals, exec); + // uncomment the "return v2" above, and these lines below, if we want to reenable reporting + // of cases where these values are different. based on spot-checks and thinking it through, + // i (alex) think v2 is always correct if applicable, but leaving this in for a while after 2018-11 + // in case it warrants further investigation. +// if (v2!=null && !Objects.equal(v1, v2)) { +// log.warn("Different values extracted for "+this+": "+v1+" ("+JavaClassNames.cleanSimpleClassName(v1)+") / "+v2+" ("+JavaClassNames.cleanSimpleClassName(v2)+")"); +// return v2; +// } + return v1; + } } } diff --git a/core/src/main/java/org/apache/brooklyn/core/config/internal/AbstractConfigMapImpl.java b/core/src/main/java/org/apache/brooklyn/core/config/internal/AbstractConfigMapImpl.java index 9e68099179..c5b12228dc 100644 --- a/core/src/main/java/org/apache/brooklyn/core/config/internal/AbstractConfigMapImpl.java +++ b/core/src/main/java/org/apache/brooklyn/core/config/internal/AbstractConfigMapImpl.java @@ -44,6 +44,7 @@ import org.apache.brooklyn.core.config.ConfigKeys.InheritanceContext; import org.apache.brooklyn.core.config.Sanitizer; import org.apache.brooklyn.core.config.StructuredConfigKey; +import org.apache.brooklyn.core.config.BasicConfigKey.BasicConfigKeyOverwriting; import org.apache.brooklyn.core.objs.BrooklynObjectInternal; import org.apache.brooklyn.util.collections.MutableList; import org.apache.brooklyn.util.collections.MutableMap; @@ -177,7 +178,7 @@ public Object setConfig(final ConfigKey key, Object v) { ConfigKey ownKey = getKeyAtContainer(getContainer(), key); if (ownKey==null) ownKey = key; - Object val = coerceConfigVal(ownKey, v); + Object val = coerceConfigValOnWrite(ownKey, v); Object oldVal; if (ownKey instanceof StructuredConfigKey) { oldVal = ((StructuredConfigKey)ownKey).applyValueToMap(val, ownConfig); @@ -248,17 +249,48 @@ public Maybe getConfigRaw(ConfigKey key, boolean includeInherited) { return getParentInternal().config().getInternalConfigMap().getConfigRaw(key, includeInherited); } - protected Object coerceConfigVal(ConfigKey key, Object v) { + // In general we coerce on write to detect errors fast, but there are a lot of exceptions + // where we aren't strict on write, such as deferred suppliers and deep config. + protected Object coerceConfigValOnWrite(ConfigKey key, Object v) { if ((v instanceof Future) || (v instanceof DeferredSupplier) || (v instanceof TaskFactory)) { // no coercion for these (coerce on exit) return v; - } else if (key instanceof StructuredConfigKey) { - // no coercion for these structures (they decide what to do) + } else if (isStructuredKey(key)) { + // no coercion for these structures (they decide what coercion to enforce when elements are set) return v; - } else if ((v instanceof Map || v instanceof Iterable) && key.getType().isInstance(v)) { - // don't do coercion on put for these, if the key type is compatible, - // because that will force resolution deeply + } else if (v instanceof Map && key.getType().isInstance(v)) { + // don't do coercion on put for maps, as we may have deferred suppliers internally + // (we could implement deep coercion which excludes futures but not realllly needed) return v; + } else if (v instanceof Iterable && Iterable.class.isAssignableFrom(key.getType())) { + // as for maps, but do handle set/list differences + // (prior to 2018-11 we applied the same check as for Map for Iterable, which meant + // setting a List in a ConfigKey did not come in to this block, but rather it + // fell through to the coercion block below; if the list had a Future in it, and + // were being asked to coerce to a Set, it would of course fail to coerce, + // and no attempt would be made to coerce to a Set; on read it will attempt to coerce + // the values to String but it would not coerce the List to a Set; it never happened arose + // as a problem IRL because we were good about using SetConfigKey, but the addition of tests of + // TestEntity.CONF_SET_STRING_PLAIN demonstrated the problem in existing code that was revealed + // by setting defaults and using BasicConfigKeyOverwriting; the new logic below corrects this problem + try { + // try to coerce on input, to detect errors sooner + return TypeCoercions.coerce(v, key.getTypeToken()); + } catch (Exception e) { + // could not coerce, something in the iterable is not coercible; + // for now however assume it is a future and proceed without flagging an error + if (key.getType().isInstance(v)) { + return v; + } + if (key.getType().isAssignableFrom(MutableList.class)) { + return MutableList.copyOf((Iterable)v).asUnmodifiable(); + } + if (key.getType().isAssignableFrom(MutableSet.class)) { + return MutableSet.copyOf((Iterable)v).asUnmodifiable(); + } + // it's a weird sort of collection-to-collection mapping, so just fail + throw new IllegalArgumentException("Cannot coerce or set iterable types, "+v+" to "+key, e); + } } else { try { // try to coerce on input, to detect errors sooner @@ -275,6 +307,15 @@ protected Object coerceConfigVal(ConfigKey key, Object v) { } } + private boolean isStructuredKey(ConfigKey key) { + // check if it is structured _or effectively so_ + if (key instanceof StructuredConfigKey) return true; + if (key instanceof BasicConfigKeyOverwriting) { + return isStructuredKey(((BasicConfigKeyOverwriting)key).getParentKey()); + } + return false; + } + @Override public Map asMapWithStringKeys() { return mapViewWithStringKeys; diff --git a/core/src/main/java/org/apache/brooklyn/core/location/internal/LocationConfigMap.java b/core/src/main/java/org/apache/brooklyn/core/location/internal/LocationConfigMap.java index 51e2e616c4..9e49aac0cc 100644 --- a/core/src/main/java/org/apache/brooklyn/core/location/internal/LocationConfigMap.java +++ b/core/src/main/java/org/apache/brooklyn/core/location/internal/LocationConfigMap.java @@ -89,7 +89,7 @@ public LocationConfigMap submap(Predicate> filter) { } @Override - protected Object coerceConfigVal(ConfigKey key, Object v) { + protected Object coerceConfigValOnWrite(ConfigKey key, Object v) { if ((Class.class.isAssignableFrom(key.getType()) || Function.class.isAssignableFrom(key.getType())) && v instanceof String) { // strings can be written where classes/functions are permitted; // this because an occasional pattern only for locations because validation wasn't enforced there @@ -98,7 +98,7 @@ protected Object coerceConfigVal(ConfigKey key, Object v) { return v; } - return super.coerceConfigVal(key, v); + return super.coerceConfigValOnWrite(key, v); } @Override From 4d79eafb864ae24bb5595cb1fa4c0d60b4b6d172 Mon Sep 17 00:00:00 2001 From: Alex Heneveld Date: Sat, 24 Nov 2018 23:38:07 +0000 Subject: [PATCH 14/18] update other tests where semantics are changed in acceptable ways --- .../brooklyn/ConfigParametersYamlTest.java | 42 ++++++++----------- .../camp/brooklyn/DslAndRebindYamlTest.java | 22 +++++++++- 2 files changed, 38 insertions(+), 26 deletions(-) diff --git a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/ConfigParametersYamlTest.java b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/ConfigParametersYamlTest.java index 87be1a45dc..3c8297f214 100644 --- a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/ConfigParametersYamlTest.java +++ b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/ConfigParametersYamlTest.java @@ -28,6 +28,7 @@ import java.util.Date; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Set; import java.util.stream.Collectors; @@ -175,7 +176,12 @@ public void testConfigParameterOverridingJavaListedInType() throws Exception { // it was using the `MapConfigKey`. Unfortunately the `MapConfigKey` uses a different structure // for storing its data (flattening out the map). So the subsequent lookup failed. // - // There are three parts to fixing this: + // 2018-11 This is now working as the BasicConfigKey acts better; + // however more complicated tests could be written for inconsistencies between YAML "Map" + // parameters (ie BasicConfigKey) and java MapConfigKey. + // Remember these notes if we encounter those! + // + // These are the anticipated steps to fixing this better: // 1. [DONE] In `entity.config().set(key, val)`, replace `key` with the entity's own key // (i.e. the same logic that will subsequently be used in the `get`). // 2. [DONE] In `BrooklynComponentTemplateResolver.findAllFlagsAndConfigKeyValues`, respect @@ -186,19 +192,7 @@ public void testConfigParameterOverridingJavaListedInType() throws Exception { // 4. [TODO] Major overhaul of the ConfigKey name versus `SetFromFlag` alias. It is currently // confusing in when reading the config values what the precedence is because there are // different names that are only understood by some things. - @Test(groups="Broken") public void testConfigParameterOverridingJavaMapConfigKey() throws Exception { - runConfigParameterOverridingJavaMapConfigKey(true); - } - - @Test - public void testConfigParameterOverridingJavaMapConfigKeyWithoutRebindValueCheck() throws Exception { - // A cut-down test of what is actually working just now (so we can detect any - // further regressions!) - runConfigParameterOverridingJavaMapConfigKey(false); - } - - protected void runConfigParameterOverridingJavaMapConfigKey(boolean assertReboundVal) throws Exception { addCatalogItems( "brooklyn.catalog:", " itemType: entity", @@ -227,15 +221,7 @@ protected void runConfigParameterOverridingJavaMapConfigKey(boolean assertReboun // Rebind, and then check again that the config key is listed Entity newApp = rebind(); TestEntity newEntity = (TestEntity) Iterables.getOnlyElement(newApp.getChildren()); - if (assertReboundVal) { - assertKeyEquals(newEntity, TestEntity.CONF_MAP_THING.getName(), "myDescription", java.util.Map.class, null, ImmutableMap.of("mykey", "myval")); - } else { - // TODO delete duplication from `assertKeyEquals`, when the above works! - ConfigKey key = newEntity.getEntityType().getConfigKey(TestEntity.CONF_MAP_THING.getName()); - assertEquals(key.getDescription(), "myDescription"); - assertEquals(key.getType(), java.util.Map.class); - assertEquals(key.getDefaultValue(), null); - } + assertKeyEquals(newEntity, TestEntity.CONF_MAP_THING.getName(), "myDescription", java.util.Map.class, null, ImmutableMap.of("mykey", "myval")); } @Test @@ -1357,16 +1343,22 @@ public static interface TestEntityWithPinnedConfig extends Entity { public static class TestEntityWithPinnedConfigImpl extends TestEntityImpl implements TestEntityWithPinnedConfig { } - protected void assertKeyEquals(Entity entity, String keyName, String expectedDescription, Class expectedType, T expectedDefaultVal, T expectedEntityVal) { + protected void assertKeyEquals(Entity entity, String keyName, String expectedDescription, Class expectedType, T expectedDefaultValIfNoEntityVal, T expectedEntityVal) { ConfigKey key = entity.getEntityType().getConfigKey(keyName); assertNotNull(key, "No key '"+keyName+"'; keys="+entity.getEntityType().getConfigKeys()); assertEquals(key.getName(), keyName); assertEquals(key.getDescription(), expectedDescription); assertEquals(key.getType(), expectedType); - assertEquals(key.getDefaultValue(), expectedDefaultVal); - assertEquals(entity.config().get(key), expectedEntityVal); + + if (Objects.equals(key.getDefaultValue(), expectedEntityVal!=null ? expectedEntityVal : expectedDefaultValIfNoEntityVal)) { + // fine - if the entity has a value, we want the default value to have changed to be that actual value; + // this is because types defined in yaml, when subtyped, should see the value the parent set. + // if there is no value then the original default is expected. + } else { + Assert.fail("Default value for "+key+" not as expected: "+key.getDefaultValue()); + } } @ImplementedBy(TestEntityWithUninheritedConfigImpl.class) diff --git a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/DslAndRebindYamlTest.java b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/DslAndRebindYamlTest.java index 67cb48fe88..53862b7b06 100644 --- a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/DslAndRebindYamlTest.java +++ b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/DslAndRebindYamlTest.java @@ -227,8 +227,28 @@ protected void doDslAttributeWhenReadyPersistedAsDeferredSupplier(boolean resolv // foo // // + // may be longer if there is a constraint, inheritance, esp if it is a BasicConfigKeyOverwriting storing the parent's definition also, eg +// +// test.confName +// +// java.lang.String +// Configuration key, my name +// +// false +// ALWAYS_TRUE +// +// test.confName +// +// java.lang.String +// Configuration key, my name +// defaultval +// false +// ALWAYS_TRUE +// +// + - Assert.assertTrue(testConfNamePersistedState.length() < 400, "persisted state too long: " + testConfNamePersistedState); + Assert.assertTrue(testConfNamePersistedState.length() < 1600, "persisted state too long ("+testConfNamePersistedState.length()+"): " + testConfNamePersistedState); Assert.assertFalse(testConfNamePersistedState.contains("bar"), "value 'bar' leaked in persisted state"); } From 4f129171ab8c421288bf4e0840e728b449d4c08b Mon Sep 17 00:00:00 2001 From: Alex Heneveld Date: Sun, 25 Nov 2018 10:35:18 +0000 Subject: [PATCH 15/18] don't include common defaults in serialized repn of config keys this now excludes: ALWAYS_TRUE and (messy expression for empty immutable list) but it _will_ read them in just find so persisted state reading is unchanged, writing is less verbose --- .../camp/brooklyn/DslAndRebindYamlTest.java | 8 ++-- .../brooklyn/core/config/BasicConfigKey.java | 37 ++++++++++----- .../rebind/RebindConfigInheritanceTest.java | 31 ++++++++----- ...onfig-inheritance-basic-2016-11-kmpez5fznt | 2 +- ...g-inheritance-basic-2016-11-kmpez5fznt-out | 45 +++++++++++++++++++ 5 files changed, 94 insertions(+), 29 deletions(-) create mode 100644 core/src/test/resources/org/apache/brooklyn/core/mgmt/rebind/config-inheritance-basic-2016-11-kmpez5fznt-out diff --git a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/DslAndRebindYamlTest.java b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/DslAndRebindYamlTest.java index 53862b7b06..f39c3da6fe 100644 --- a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/DslAndRebindYamlTest.java +++ b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/DslAndRebindYamlTest.java @@ -227,28 +227,26 @@ protected void doDslAttributeWhenReadyPersistedAsDeferredSupplier(boolean resolv // foo // // + // may be longer if there is a constraint, inheritance, esp if it is a BasicConfigKeyOverwriting storing the parent's definition also, eg + // (with some cleanups now since 2018-11 the BasicConfigKey will prefer nulls for lesser-used fields with common default values) // // test.confName -// // java.lang.String // Configuration key, my name // // false -// ALWAYS_TRUE // // test.confName -// // java.lang.String // Configuration key, my name // defaultval // false -// ALWAYS_TRUE // // - Assert.assertTrue(testConfNamePersistedState.length() < 1600, "persisted state too long ("+testConfNamePersistedState.length()+"): " + testConfNamePersistedState); + Assert.assertTrue(testConfNamePersistedState.length() < 1200, "persisted state too long ("+testConfNamePersistedState.length()+"): " + testConfNamePersistedState); Assert.assertFalse(testConfNamePersistedState.contains("bar"), "value 'bar' leaked in persisted state"); } diff --git a/core/src/main/java/org/apache/brooklyn/core/config/BasicConfigKey.java b/core/src/main/java/org/apache/brooklyn/core/config/BasicConfigKey.java index 5cf9df7066..c80bc21ecb 100644 --- a/core/src/main/java/org/apache/brooklyn/core/config/BasicConfigKey.java +++ b/core/src/main/java/org/apache/brooklyn/core/config/BasicConfigKey.java @@ -39,7 +39,6 @@ import org.apache.brooklyn.util.core.task.ValueResolver; import org.apache.brooklyn.util.exceptions.Exceptions; import org.apache.brooklyn.util.guava.TypeTokens; -import org.apache.brooklyn.util.javalang.JavaClassNames; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -271,7 +270,10 @@ private BasicConfigKey(Class typeC, TypeToken typeT, String name, String d public BasicConfigKey(Builder builder) { this.name = checkNotNull(builder.name, "name"); - this.deprecatedNames = checkNotNull(builder.deprecatedNames, "deprecatedNames"); + + this.deprecatedNames = builder.deprecatedNames; + getDeprecatedNames(); // invoke method to tidy up for persistence + TypeTokens.checkCompatibleOneNonNull(builder.type, builder.typeToken); this.type = builder.type; this.typeToken = builder.typeToken; @@ -280,20 +282,28 @@ public BasicConfigKey(Builder builder) { this.reconfigurable = builder.reconfigurable; this.runtimeInheritance = builder.runtimeInheritance; this.typeInheritance = builder.typeInheritance; + // Note: it's intentionally possible to have default values that are not valid // per the configured constraint. If validity were checked here any class that // contained a weirdly-defined config key would fail to initialise. - this.constraint = checkNotNull(builder.constraint, "constraint"); + this.constraint = builder.constraint; + getConstraint(); // invoke method to tidy up for persistence } /** @see ConfigKey#getName() */ @Override public String getName() { return name; } /** @see ConfigKey#getDeprecatedNames() */ - @Override public Collection getDeprecatedNames() { - // check for null, for backwards compatibility of serialized state - if (deprecatedNames == null) deprecatedNames = ImmutableList.of(); - return deprecatedNames; + @Override public synchronized Collection getDeprecatedNames() { + // check for empty list, for backwards compatibility of serialized state; prefer "null" so we don't get noise in serialised state + if (deprecatedNames!=null) { + if (deprecatedNames.isEmpty()) { + deprecatedNames = null; + } else { + return deprecatedNames; + } + } + return ImmutableList.of(); } /** @see ConfigKey#getTypeName() */ @@ -375,13 +385,16 @@ public ConfigInheritance getParentInheritance() { /** @see ConfigKey#getConstraint() */ @Override @Nonnull - public Predicate getConstraint() { - // Could be null after rebinding + public synchronized Predicate getConstraint() { if (constraint != null) { - return constraint; - } else { - return Predicates.alwaysTrue(); + if (Predicates.alwaysTrue().equals(constraint)) { + // prefer null so persisted state is cleaner + constraint = null; + } else { + return constraint; + } } + return Predicates.alwaysTrue(); } /** @see ConfigKey#isValueValid(T) */ diff --git a/core/src/test/java/org/apache/brooklyn/core/mgmt/rebind/RebindConfigInheritanceTest.java b/core/src/test/java/org/apache/brooklyn/core/mgmt/rebind/RebindConfigInheritanceTest.java index 97faeec5ef..ff7c4fcb0f 100644 --- a/core/src/test/java/org/apache/brooklyn/core/mgmt/rebind/RebindConfigInheritanceTest.java +++ b/core/src/test/java/org/apache/brooklyn/core/mgmt/rebind/RebindConfigInheritanceTest.java @@ -33,6 +33,7 @@ import org.apache.brooklyn.core.entity.EntityAsserts; import org.apache.brooklyn.core.test.entity.TestEntity; import org.apache.brooklyn.test.Asserts; +import org.apache.brooklyn.util.exceptions.Exceptions; import org.apache.brooklyn.util.os.Os; import org.apache.brooklyn.util.stream.Streams; import org.apache.brooklyn.util.text.Strings; @@ -52,7 +53,7 @@ public class RebindConfigInheritanceTest extends RebindTestFixtureWithApp { private static final Logger log = LoggerFactory.getLogger(RebindConfigInheritanceTest.class); ConfigKey key1 = ConfigKeys.builder(String.class, "key1").runtimeInheritance(BasicConfigInheritance.NEVER_INHERITED).build(); - String origMemento, newMemento; + String inMementoRaw, expectedOutMemento, newMemento; Application rebindedApp; @Override @@ -63,10 +64,20 @@ public void setUp() throws Exception { protected void doReadConfigInheritance(String label, String entityId) throws Exception { String mementoFilename = "config-inheritance-"+label+"-"+entityId; - origMemento = Streams.readFullyString(getClass().getResourceAsStream(mementoFilename)); + inMementoRaw = Streams.readFullyString(getClass().getResourceAsStream(mementoFilename)); File persistedEntityFile = new File(mementoDir, Os.mergePaths("entities", entityId)); - Files.write(origMemento.getBytes(), persistedEntityFile); + Files.write(inMementoRaw.getBytes(), persistedEntityFile); + try { + expectedOutMemento = Streams.readFullyString(getClass().getResourceAsStream(mementoFilename+"-out")); + } catch (Exception e) { + Exceptions.propagateIfFatal(e); + expectedOutMemento = inMementoRaw; + } + if (expectedOutMemento.startsWith("")+3).trim(); + } + expectedOutMemento = Strings.replaceAllNonRegex(expectedOutMemento, "$VERSION", BrooklynVersion.get()); // we'll make assertions on what we've loaded rebind(); @@ -84,8 +95,8 @@ public void testPreBasicConfigInheritance_2016_07() throws Exception { ConfigKey k = Iterables.getOnlyElement( rebindedApp.config().findKeysDeclared(ConfigPredicates.nameEqualTo("my.config.inheritanceMerged")) ); - Asserts.assertStringContains(origMemento, ""); - Asserts.assertStringDoesNotContain(origMemento, BasicConfigInheritance.DEEP_MERGE.getClass().getName()); + Asserts.assertStringContains(inMementoRaw, ""); + Asserts.assertStringDoesNotContain(inMementoRaw, BasicConfigInheritance.DEEP_MERGE.getClass().getName()); // should now convert it to BasicConfigInheritance.DEEP_MERGE Asserts.assertStringDoesNotContain(newMemento, "ConfigInheritance$Merged"); @@ -102,9 +113,9 @@ public void testBasicConfigInheritance_2016_10() throws Exception { checkNewAppNonInheritingKey1(rebindedApp); - Asserts.assertStringContains(origMemento, "isReinherited"); - Asserts.assertStringDoesNotContain(origMemento, "NEVER_INHERITED"); - Asserts.assertStringDoesNotContain(origMemento, "NeverInherited"); + Asserts.assertStringContains(inMementoRaw, "isReinherited"); + Asserts.assertStringDoesNotContain(inMementoRaw, "NEVER_INHERITED"); + Asserts.assertStringDoesNotContain(inMementoRaw, "NeverInherited"); // should write it out as NeverInherited Asserts.assertStringDoesNotContain(newMemento, "isReinherited"); @@ -116,9 +127,7 @@ public void testReadConfigInheritance_2016_11() throws Exception { doReadConfigInheritance("basic-2016-11", "kmpez5fznt"); checkNewAppNonInheritingKey1(rebindedApp); - String origMementoTidied = origMemento.substring(origMemento.indexOf("")); - origMementoTidied = Strings.replaceAllNonRegex(origMementoTidied, "VERSION", BrooklynVersion.get()); - Asserts.assertEquals(origMementoTidied, newMemento.replaceAll("\n.*searchPath.*\n", "\n")); + Asserts.assertEquals(newMemento.replaceAll("\n.*searchPath.*\n", "\n"), expectedOutMemento); } @Test diff --git a/core/src/test/resources/org/apache/brooklyn/core/mgmt/rebind/config-inheritance-basic-2016-11-kmpez5fznt b/core/src/test/resources/org/apache/brooklyn/core/mgmt/rebind/config-inheritance-basic-2016-11-kmpez5fznt index 2dc8996482..cf635eae8c 100644 --- a/core/src/test/resources/org/apache/brooklyn/core/mgmt/rebind/config-inheritance-basic-2016-11-kmpez5fznt +++ b/core/src/test/resources/org/apache/brooklyn/core/mgmt/rebind/config-inheritance-basic-2016-11-kmpez5fznt @@ -18,7 +18,7 @@ under the License. --> - VERSION + 0.10.0-SNAPSHOT org.apache.brooklyn.core.test.entity.TestApplicationNoEnrichersImpl kmpez5fznt TestApplicationNoEnrichersImpl:kmpe diff --git a/core/src/test/resources/org/apache/brooklyn/core/mgmt/rebind/config-inheritance-basic-2016-11-kmpez5fznt-out b/core/src/test/resources/org/apache/brooklyn/core/mgmt/rebind/config-inheritance-basic-2016-11-kmpez5fznt-out new file mode 100644 index 0000000000..a1b177d1b4 --- /dev/null +++ b/core/src/test/resources/org/apache/brooklyn/core/mgmt/rebind/config-inheritance-basic-2016-11-kmpez5fznt-out @@ -0,0 +1,45 @@ + + + + $VERSION + org.apache.brooklyn.core.test.entity.TestApplicationNoEnrichersImpl + kmpez5fznt + TestApplicationNoEnrichersImpl:kmpe + + 1 + + + kmpez5fznt + kmpez5fznt + + + + + + + + key1 + java.lang.String + false + + + + + \ No newline at end of file From 731b9adfccd3dbc7c7bdacec0f89e85d88cfa22e Mon Sep 17 00:00:00 2001 From: Alex Heneveld Date: Mon, 26 Nov 2018 01:37:08 +0000 Subject: [PATCH 16/18] fix set config and isSet check for BasicConfigKeyOverwritten --- .../brooklyn/core/config/BasicConfigKey.java | 12 ++++++++++-- .../config/internal/AbstractConfigMapImpl.java | 15 ++++++++------- .../brooklyn/core/entity/AbstractEntity.java | 3 ++- 3 files changed, 20 insertions(+), 10 deletions(-) diff --git a/core/src/main/java/org/apache/brooklyn/core/config/BasicConfigKey.java b/core/src/main/java/org/apache/brooklyn/core/config/BasicConfigKey.java index c80bc21ecb..ea47dff2d2 100644 --- a/core/src/main/java/org/apache/brooklyn/core/config/BasicConfigKey.java +++ b/core/src/main/java/org/apache/brooklyn/core/config/BasicConfigKey.java @@ -490,13 +490,21 @@ public BasicConfigKeyOverwriting(ConfigKey key, String newDescription, T defa public ConfigKey getParentKey() { return parentKey; } + + @Override + public boolean isSet(Map vals) { + if (parentKey instanceof ConfigKeySelfExtracting) { + return ((ConfigKeySelfExtracting)parentKey).isSet(vals); + } + return super.isSet(vals); + } @Override public T extractValue(Map vals, ExecutionContext exec) { // if we are unsure and want to log both, then T v1, v2 = null; - if (parentKey instanceof BasicConfigKey) { - v2 = ((BasicConfigKey)parentKey).extractValue(vals, exec); + if (parentKey instanceof ConfigKeySelfExtracting) { + v2 = ((ConfigKeySelfExtracting)parentKey).extractValue(vals, exec); return v2; } v1 = super.extractValue(vals, exec); diff --git a/core/src/main/java/org/apache/brooklyn/core/config/internal/AbstractConfigMapImpl.java b/core/src/main/java/org/apache/brooklyn/core/config/internal/AbstractConfigMapImpl.java index c5b12228dc..ab4076cb56 100644 --- a/core/src/main/java/org/apache/brooklyn/core/config/internal/AbstractConfigMapImpl.java +++ b/core/src/main/java/org/apache/brooklyn/core/config/internal/AbstractConfigMapImpl.java @@ -180,8 +180,9 @@ public Object setConfig(final ConfigKey key, Object v) { Object val = coerceConfigValOnWrite(ownKey, v); Object oldVal; - if (ownKey instanceof StructuredConfigKey) { - oldVal = ((StructuredConfigKey)ownKey).applyValueToMap(val, ownConfig); + Maybe structured = getStructuredKeyMaybe(ownKey); + if (structured.isPresent()) { + oldVal = structured.get().applyValueToMap(val, ownConfig); } else { oldVal = ownConfig.put(ownKey, val); } @@ -255,7 +256,7 @@ protected Object coerceConfigValOnWrite(ConfigKey key, Object v) { if ((v instanceof Future) || (v instanceof DeferredSupplier) || (v instanceof TaskFactory)) { // no coercion for these (coerce on exit) return v; - } else if (isStructuredKey(key)) { + } else if (getStructuredKeyMaybe(key).isPresent()) { // no coercion for these structures (they decide what coercion to enforce when elements are set) return v; } else if (v instanceof Map && key.getType().isInstance(v)) { @@ -307,13 +308,13 @@ protected Object coerceConfigValOnWrite(ConfigKey key, Object v) { } } - private boolean isStructuredKey(ConfigKey key) { + private Maybe getStructuredKeyMaybe(ConfigKey key) { // check if it is structured _or effectively so_ - if (key instanceof StructuredConfigKey) return true; + if (key instanceof StructuredConfigKey) return Maybe.of((StructuredConfigKey)key); if (key instanceof BasicConfigKeyOverwriting) { - return isStructuredKey(((BasicConfigKeyOverwriting)key).getParentKey()); + return getStructuredKeyMaybe(((BasicConfigKeyOverwriting)key).getParentKey()); } - return false; + return Maybe.absent(); } @Override diff --git a/core/src/main/java/org/apache/brooklyn/core/entity/AbstractEntity.java b/core/src/main/java/org/apache/brooklyn/core/entity/AbstractEntity.java index fa7cecd251..493b838238 100644 --- a/core/src/main/java/org/apache/brooklyn/core/entity/AbstractEntity.java +++ b/core/src/main/java/org/apache/brooklyn/core/entity/AbstractEntity.java @@ -356,7 +356,8 @@ private static Map checkConstructorFlags(Map flags, Entity parent) { } /** - * @deprecated since 0.7.0; only used for legacy brooklyn types where constructor is called directly + * @deprecated since 0.7.0; only used for legacy brooklyn types where constructor is called directly, + * (and used for internal private configuration from entity specs which use flags to set keys) */ @Override @Deprecated From 9374ecd20eeebf81567135b3b90a2740ab79f789 Mon Sep 17 00:00:00 2001 From: Alex Heneveld Date: Mon, 26 Nov 2018 02:02:25 +0000 Subject: [PATCH 17/18] add failing test (marked broken) for edge case where flags not included in default --- .../brooklyn/ConfigInheritanceYamlTest.java | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/ConfigInheritanceYamlTest.java b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/ConfigInheritanceYamlTest.java index 89f282b262..e89ddc1149 100644 --- a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/ConfigInheritanceYamlTest.java +++ b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/ConfigInheritanceYamlTest.java @@ -35,6 +35,7 @@ import org.apache.brooklyn.api.sensor.AttributeSensor; import org.apache.brooklyn.config.ConfigKey; import org.apache.brooklyn.core.config.ConfigKeys; +import org.apache.brooklyn.core.config.ConfigPredicates; import org.apache.brooklyn.core.config.MapConfigKey; import org.apache.brooklyn.core.entity.Entities; import org.apache.brooklyn.core.entity.EntityAsserts; @@ -43,6 +44,7 @@ import org.apache.brooklyn.core.test.entity.TestEntityImpl; import org.apache.brooklyn.entity.software.base.EmptySoftwareProcess; import org.apache.brooklyn.entity.stock.BasicApplication; +import org.apache.brooklyn.test.Asserts; import org.apache.brooklyn.util.collections.MutableMap; import org.apache.brooklyn.util.core.internal.ssh.RecordingSshTool; import org.apache.brooklyn.util.os.Os; @@ -741,7 +743,7 @@ public void testExtendsSuperTypeConfigSimple() throws Exception { } @Test - public void testExtendsSuperTypeConfigMixingLongOverridingShortNames() throws Exception { + public Entity testExtendsSuperTypeConfigMixingLongOverridingShortNames() throws Exception { ImmutableMap expectedEnv = ImmutableMap.of("ENV1", "myEnv1", "ENV2", "myEnv2"); // super-type has env; sub-type shell.env @@ -756,8 +758,21 @@ public void testExtendsSuperTypeConfigMixingLongOverridingShortNames() throws Ex Entity app = createStartWaitAndLogApplication(yaml); Entity entity = Iterables.getOnlyElement(app.getChildren()); EntityAsserts.assertConfigEquals(entity, EmptySoftwareProcess.SHELL_ENVIRONMENT, expectedEnv); + return entity; } - + + /** EntitySpecs track flags and config separately, with the merge occurring on Entity creation, + * so from the spec alone it is not straightforward to include flags in the determination of the default value. + * Thus default values do _not_ include flags and this test has only the shell.env KV pair in the default + * (so size 1, not size 2 which we want) + */ + @Test(groups="Broken") + public void testDefaultValueWhenExtendingSuperTypeConfigMixingLongOverridingShortNames() throws Exception { + Entity entity = testExtendsSuperTypeConfigMixingLongOverridingShortNames(); + ConfigKey key = Iterables.getOnlyElement(entity.config().findKeysDeclared(ConfigPredicates.nameEqualTo(EmptySoftwareProcess.SHELL_ENVIRONMENT.getName()))); + Asserts.assertSize((Map)key.getDefaultValue(), 2); + } + @Test public void testExtendsSuperTypeConfigMixingShortOverridingLongName() throws Exception { ImmutableMap expectedEnv = ImmutableMap.of("ENV1", "myEnv1", "ENV2", "myEnv2"); From 69f33c02353a26f7f0607c5c94913718f2d994da Mon Sep 17 00:00:00 2001 From: Alex Heneveld Date: Mon, 26 Nov 2018 11:22:48 +0000 Subject: [PATCH 18/18] expand + correct test of runtime inheritance for key visibility the failure was a simple default value has changed as expected by this PR, but there are some other cases that it's interesting to check --- .../brooklyn/ConfigInheritanceYamlTest.java | 94 +++++++++++++------ 1 file changed, 65 insertions(+), 29 deletions(-) diff --git a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/ConfigInheritanceYamlTest.java b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/ConfigInheritanceYamlTest.java index e89ddc1149..e456493a31 100644 --- a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/ConfigInheritanceYamlTest.java +++ b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/ConfigInheritanceYamlTest.java @@ -22,6 +22,7 @@ import java.io.File; import java.util.Collection; +import java.util.Iterator; import java.util.List; import java.util.Map; import java.util.concurrent.Callable; @@ -558,6 +559,9 @@ public void testEntityRuntimeInheritanceOptions() throws Exception { " type: java.util.Map", " inheritance.runtime: not_reinherited"); + ImmutableMap defaultKV = ImmutableMap.of("myDefaultKey", "myDefaultVal"); + ImmutableMap myKV = ImmutableMap.of("mykey", "myval"); + // Test retrieval of defaults { String yaml = Joiner.on("\n").join( @@ -569,10 +573,10 @@ public void testEntityRuntimeInheritanceOptions() throws Exception { Entity app = createStartWaitAndLogApplication(yaml); TestEntity entity = (TestEntity) Iterables.getOnlyElement(app.getChildren()); - assertDeclaredKeyAndAnonymousKeyValuesEqual(entity, "map.type-merged", ImmutableMap.of("myDefaultKey", "myDefaultVal")); - assertDeclaredKeyAndAnonymousKeyValuesEqual(entity, "map.type-overwrite", ImmutableMap.of("myDefaultKey", "myDefaultVal")); - assertDeclaredKeyAndAnonymousKeyValuesEqual(entity, "map.type-never", ImmutableMap.of("myDefaultKey", "myDefaultVal")); - assertDeclaredKeyAndAnonymousKeyValuesEqual(entity, "map.type-not-reinherited", ImmutableMap.of("myDefaultKey", "myDefaultVal")); + assertDeclaredKeyAndAnonymousKeyValuesEqual(entity, "map.type-merged", defaultKV); + assertDeclaredKeyAndAnonymousKeyValuesEqual(entity, "map.type-overwrite", defaultKV); + assertDeclaredKeyAndAnonymousKeyValuesEqual(entity, "map.type-never", defaultKV); + assertDeclaredKeyAndAnonymousKeyValuesEqual(entity, "map.type-not-reinherited", defaultKV); } // Test override defaults in parent entity @@ -595,10 +599,10 @@ public void testEntityRuntimeInheritanceOptions() throws Exception { Entity app = createStartWaitAndLogApplication(yaml); TestEntity entity = (TestEntity) Iterables.getOnlyElement(app.getChildren()); - assertDeclaredKeyAndAnonymousKeyValuesEqual(entity, "map.type-merged", ImmutableMap.of("mykey", "myval")); - assertDeclaredKeyAndAnonymousKeyValuesEqual(entity, "map.type-overwrite", ImmutableMap.of("mykey", "myval")); - assertDeclaredKeyAndAnonymousKeyValuesEqual(entity, "map.type-never", ImmutableMap.of("myDefaultKey", "myDefaultVal")); - assertDeclaredKeyAndAnonymousKeyValuesEqual(entity, "map.type-not-reinherited", ImmutableMap.of("mykey", "myval")); + assertDeclaredKeyAndAnonymousKeyValuesEqual(entity, "map.type-merged", myKV); + assertDeclaredKeyAndAnonymousKeyValuesEqual(entity, "map.type-overwrite", myKV); + assertDeclaredKeyAndAnonymousKeyValuesEqual(entity, "map.type-never", defaultKV); + assertDeclaredKeyAndAnonymousKeyValuesEqual(entity, "map.type-not-reinherited", myKV); } // Test merging/overriding of explicit config @@ -651,28 +655,60 @@ public void testEntityRuntimeInheritanceOptions() throws Exception { " map.type-not-reinherited:", " mykey: myval", " brooklyn.children:", + " - type: " + TestEntity.class.getName(), + "- type: entity-with-keys", + " brooklyn.children:", " - type: " + TestEntity.class.getName()); Entity app = createStartWaitAndLogApplication(yaml); - TestEntity entity = (TestEntity) Iterables.getOnlyElement(app.getChildren()); - TestEntity child = (TestEntity) Iterables.getOnlyElement(entity.getChildren()); + Iterator children = app.getChildren().iterator(); + TestEntity entityMyValues = (TestEntity) children.next(); + TestEntity childMyValues = (TestEntity) Iterables.getOnlyElement(entityMyValues.getChildren()); + TestEntity entityNoValues = (TestEntity) children.next(); + TestEntity childNoValues = (TestEntity) Iterables.getOnlyElement(entityNoValues.getChildren()); - // Not using `assertConfigEquals(child, ...)`, bacause child.getEntityType().getConfigKey() - // will not find these keys. - // - // TODO Note the different behaviour for "never" and "not-reinherited" keys for if an - // untyped key is used, versus the strongly typed key. We can live with that - I wouldn't - // expect an implementation of TestEntity to try to use such keys that it doesn't know - // about! - assertEquals(child.config().get(entity.getEntityType().getConfigKey("map.type-merged")), ImmutableMap.of("mykey", "myval")); - assertEquals(child.config().get(entity.getEntityType().getConfigKey("map.type-overwrite")), ImmutableMap.of("mykey", "myval")); - assertEquals(child.config().get(entity.getEntityType().getConfigKey("map.type-never")), ImmutableMap.of("myDefaultKey", "myDefaultVal")); - assertEquals(child.config().get(entity.getEntityType().getConfigKey("map.type-not-reinherited")), ImmutableMap.of("myDefaultKey", "myDefaultVal")); + // looking first at child with MY values (from runtime mgmt parent) + + // using keys from cNV we see the inherited values or if not inherited we don't see the keys + assertEquals(childMyValues.config().get(ConfigKeys.newConfigKey(Map.class, "map.type-merged")), myKV); + assertEquals(childMyValues.config().get(ConfigKeys.newConfigKey(Map.class, "map.type-overwrite")), myKV); + assertEquals(childMyValues.config().get(ConfigKeys.newConfigKey(Map.class, "map.type-never")), null); + assertEquals(childMyValues.config().get(ConfigKeys.newConfigKey(Map.class, "map.type-not-reinherited")), null); + + // using keys from eNV as originally defined, we get the runtime inherited values or original default values + // the use of non-inherited keys is weird but allows us to assert the right things happen depending which + // keys we use (this of course is also why we don't use `assertConfigEquals(child, ...)` + assertEquals(childMyValues.config().get(entityNoValues.getEntityType().getConfigKey("map.type-merged")), myKV); + assertEquals(childMyValues.config().get(entityNoValues.getEntityType().getConfigKey("map.type-overwrite")), myKV); + assertEquals(childMyValues.config().get(entityNoValues.getEntityType().getConfigKey("map.type-never")), defaultKV); + assertEquals(childMyValues.config().get(entityNoValues.getEntityType().getConfigKey("map.type-not-reinherited")), defaultKV); + + // if we use eMV keys, the "defaults" we see are the values it has set, since we change defaults (2018-11) + assertEquals(childMyValues.config().get(entityMyValues.getEntityType().getConfigKey("map.type-merged")), myKV); + assertEquals(childMyValues.config().get(entityMyValues.getEntityType().getConfigKey("map.type-overwrite")), myKV); + assertEquals(childMyValues.config().get(entityMyValues.getEntityType().getConfigKey("map.type-never")), myKV); + assertEquals(childMyValues.config().get(entityMyValues.getEntityType().getConfigKey("map.type-not-reinherited")), myKV); + + // looking at child with NO values + + // using keys from cNV we see the default values or if not inherited we don't see the keys + assertEquals(childNoValues.config().get(ConfigKeys.newConfigKey(Map.class, "map.type-merged")), defaultKV); + assertEquals(childNoValues.config().get(ConfigKeys.newConfigKey(Map.class, "map.type-overwrite")), defaultKV); + assertEquals(childNoValues.config().get(ConfigKeys.newConfigKey(Map.class, "map.type-never")), null); + assertEquals(childNoValues.config().get(ConfigKeys.newConfigKey(Map.class, "map.type-not-reinherited")), null); + + // using keys from eNV as originally defined, we get the runtime inherited values or original default values + assertEquals(childNoValues.config().get(entityNoValues.getEntityType().getConfigKey("map.type-merged")), defaultKV); + assertEquals(childNoValues.config().get(entityNoValues.getEntityType().getConfigKey("map.type-overwrite")), defaultKV); + assertEquals(childNoValues.config().get(entityNoValues.getEntityType().getConfigKey("map.type-never")), defaultKV); + assertEquals(childNoValues.config().get(entityNoValues.getEntityType().getConfigKey("map.type-not-reinherited")), defaultKV); - assertEquals(child.config().get(ConfigKeys.newConfigKey(Map.class, "map.type-merged")), ImmutableMap.of("mykey", "myval")); - assertEquals(child.config().get(ConfigKeys.newConfigKey(Map.class, "map.type-overwrite")), ImmutableMap.of("mykey", "myval")); - assertEquals(child.config().get(ConfigKeys.newConfigKey(Map.class, "map.type-never")), null); - assertEquals(child.config().get(ConfigKeys.newConfigKey(Map.class, "map.type-not-reinherited")), null); + // if we use eMV keys, the "defaults" we see are the values it has set, since we change defaults (2018-11) + // (this is a weird one, we wouldn't do this in real life!) + assertEquals(childNoValues.config().get(entityMyValues.getEntityType().getConfigKey("map.type-merged")), myKV); + assertEquals(childNoValues.config().get(entityMyValues.getEntityType().getConfigKey("map.type-overwrite")), myKV); + assertEquals(childNoValues.config().get(entityMyValues.getEntityType().getConfigKey("map.type-never")), myKV); + assertEquals(childNoValues.config().get(entityMyValues.getEntityType().getConfigKey("map.type-not-reinherited")), myKV); } // Test inheritance by child with duplicated keys that have no defaults - does it get runtime-management parent's defaults? @@ -689,8 +725,8 @@ public void testEntityRuntimeInheritanceOptions() throws Exception { // Note this merges the default values from the parent and child (i.e. the child's default // value does not override the parent's. - assertDeclaredKeyAndAnonymousKeyValuesEqual(child, "map.type-merged", ImmutableMap.of("myDefaultKey", "myDefaultVal")); - assertDeclaredKeyAndAnonymousKeyValuesEqual(child, "map.type-overwrite", ImmutableMap.of("myDefaultKey", "myDefaultVal")); + assertDeclaredKeyAndAnonymousKeyValuesEqual(child, "map.type-merged", defaultKV); + assertDeclaredKeyAndAnonymousKeyValuesEqual(child, "map.type-overwrite", defaultKV); assertDeclaredKeyAndAnonymousKeyValuesEqual(child, "map.type-never", null); assertDeclaredKeyAndAnonymousKeyValuesEqual(child, "map.type-not-reinherited", null); } @@ -717,8 +753,8 @@ public void testEntityRuntimeInheritanceOptions() throws Exception { TestEntity child = (TestEntity) Iterables.getOnlyElement(entity.getChildren()); - assertDeclaredKeyAndAnonymousKeyValuesEqual(child, "map.type-merged", ImmutableMap.of("mykey", "myval")); - assertDeclaredKeyAndAnonymousKeyValuesEqual(child, "map.type-overwrite", ImmutableMap.of("mykey", "myval")); + assertDeclaredKeyAndAnonymousKeyValuesEqual(child, "map.type-merged", myKV); + assertDeclaredKeyAndAnonymousKeyValuesEqual(child, "map.type-overwrite", myKV); assertDeclaredKeyAndAnonymousKeyValuesEqual(child, "map.type-never", null); assertDeclaredKeyAndAnonymousKeyValuesEqual(child, "map.type-not-reinherited", null); }