You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by duncangrant <gi...@git.apache.org> on 2018/06/05 15:36:17 UTC
[GitHub] brooklyn-server pull request #969: Add registered types test
GitHub user duncangrant opened a pull request:
https://github.com/apache/brooklyn-server/pull/969
Add registered types test
This PR follows on from #968
It adds 2 commits. The first adds tests for the functionality fixed in #968
The second changes the logic in #968 in a way that I think is equivalent but I haven't managed to add the requisite test coverage to prove this.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/duncangrant/brooklyn-server add-registered-types-test
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/brooklyn-server/pull/969.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #969
----
commit 066cbbf2f70cb9b7e4b79a6eeb29bf87422dd2f6
Author: Alex Heneveld <al...@...>
Date: 2018-06-01T23:29:02Z
does a yaml parse and compares the hash of the result when checking plan equivalence
means that the same plan entered twice but with different comments, spacing, quotations etc,
will not result in an error. it only treats a plan as non-equivalent if their yaml is different
after a deserialize-then-serialize cycle. previously a catalog push for an already-present
non-snapshot plan would fail unless the plan was identical up to a trim. now it only fails if
the objects are different.
commit c5e4e2993e8f2448286cbf08295cd7eebb006daf
Author: Duncan Grant <du...@...>
Date: 2018-06-05T15:24:23Z
Add tests to RegisteredTypes.arePlansEquivilant
Hopefully these show that we now ignore things like comments and strings
with surrounding quotes
commit e6dee4e591cb2d14acb24a9270a01d2c4bbf5e4b
Author: Duncan Grant <du...@...>
Date: 2018-06-05T15:33:05Z
Refactor to remove duplicate checks
----
---
[GitHub] brooklyn-server pull request #969: Add registered types test
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/969#discussion_r194323280
--- Diff: core/src/test/java/org/apache/brooklyn/core/typereg/RegisteredTypesTest.java ---
@@ -0,0 +1,123 @@
+package org.apache.brooklyn.core.typereg;
+
+import org.apache.brooklyn.api.typereg.BrooklynTypeRegistry;
+import org.apache.brooklyn.api.typereg.RegisteredType;
+import org.testng.annotations.Test;
+
+import javax.annotation.Nullable;
+
+import static org.testng.Assert.*;
+
+public class RegisteredTypesTest {
+
+ public static final String DRBD_YAML = "brooklyn.catalog:\n" +
--- End diff --
happy to merge as is but i like @geomacy 's suggestion readability could be improved. maybe an inner static class `EquivalentPlanGenerator` that has fields `usesQuotes1` and `includeComments` so code is like:
plan +=
" drbdIconUrl: "+quoteIfTrue(usesQuotes1, "https://s3.eu-central-1.amazonaws.com/misc-csft/drbd.jpg");
then each test might be rewritten
assertTrue(RegisteredTypes.arePlansEquivalent(
new EquivalentPlanGenerator().usesQuotes1(true).build(),
new EquivalentPlanGenerator().includesComments1(true).build() ));
(but as i said not at all necessary -- nice job adding the tests!)
---
[GitHub] brooklyn-server pull request #969: Add registered types test
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/969#discussion_r194324142
--- Diff: core/src/test/java/org/apache/brooklyn/core/typereg/RegisteredTypesTest.java ---
@@ -0,0 +1,123 @@
+package org.apache.brooklyn.core.typereg;
+
+import org.apache.brooklyn.api.typereg.BrooklynTypeRegistry;
+import org.apache.brooklyn.api.typereg.RegisteredType;
+import org.testng.annotations.Test;
+
+import javax.annotation.Nullable;
+
+import static org.testng.Assert.*;
+
+public class RegisteredTypesTest {
+
+ public static final String DRBD_YAML = "brooklyn.catalog:\n" +
+ " version: \"1.0.0-SNAPSHOT\" # BROOKLYN_DRBD_VERSION\n" +
+ " publish:\n" +
+ " description: |\n" +
+ " DRBD is a distributed replicated storage system for the Linux platform. It is implemented as a kernel driver, \n" +
+ " several userspace management applications, and some shell scripts. DRBD is traditionally used in high availability (HA) computer clusters to provide a\n" +
+ " RAID 1 like configuration over a network.\n" +
+ " license_code: Apache-2.0\n" +
+ " defaults:\n" +
+ " drbdIconUrl: \"https://s3.eu-central-1.amazonaws.com/misc-csft/drbd.jpg\"\n" +
+ " classpathIconUrl: &drbdIconUrl 'classpath://io.brooklyn.drbd.brooklyn-drbd:drbd.png'\n" +
+ " items:\n" +
+ " - \"https://github.com/brooklyncentral/common-catalog-utils/releases/download/v0.1.0/common.bom\"\n" +
+ " - id: drbd-node\n";
+
+ public static final String DRBD_TESTS_YAML = "brooklyn.catalog:\n" +
+ " version: \"1.0.0-SNAPSHOT\" # BROOKLYN_DRBD_VERSION\n" +
+ " items:\n" +
+ " - https://github.com/brooklyncentral/common-catalog-utils/releases/download/v0.1.0/common.tests.bom\n" +
+ " - id: drbd-test\n" +
+ " name: DRBD Test\n" +
+ " itemType: template\n" +
+ " iconUrl: https://s3.eu-central-1.amazonaws.com/misc-csft/drbd.jpg\n" +
+ " license: Apache-2.0\n" +
+ " item:\n" +
+ " booklyn.config:\n" +
+ " defaultDisplayName: DRBD Test\n" +
+ " services:\n" +
+ " - type: drbd-webapp\n";
+
+ public static final String DRBR_YAML_WITH_COMMENTS = DRBD_YAML + "\n" +
+ "# this is a comment";
+ private static final String DRBR_YAML_WITHOUT_QUOTES = "brooklyn.catalog:\n" +
+ " version: 1.0.0-SNAPSHOT # BROOKLYN_DRBD_VERSION\n" +
+ " publish:\n" +
+ " description: |\n" +
+ " DRBD is a distributed replicated storage system for the Linux platform. It is implemented as a kernel driver, \n" +
+ " several userspace management applications, and some shell scripts. DRBD is traditionally used in high availability (HA) computer clusters to provide a\n" +
+ " RAID 1 like configuration over a network.\n" +
+ " license_code: Apache-2.0\n" +
+ " defaults:\n" +
+ " drbdIconUrl: https://s3.eu-central-1.amazonaws.com/misc-csft/drbd.jpg\n" +
+ " classpathIconUrl: &drbdIconUrl 'classpath://io.brooklyn.drbd.brooklyn-drbd:drbd.png'\n" +
+ " items:\n" +
+ " - https://github.com/brooklyncentral/common-catalog-utils/releases/download/v0.1.0/common.bom\n" +
+ " - id: drbd-node\n";
+
+ @Test
+ public void testIdenticalPlansAreEquivalent() {
+ BasicRegisteredType a = makeTypeWithYaml(DRBD_YAML);
+
+ BasicRegisteredType b = makeTypeWithYaml(DRBD_YAML);
+
+ boolean plansEquivalent = RegisteredTypes.arePlansEquivalent(
+ a, b);
+ assertTrue(plansEquivalent);
--- End diff --
agree this would be a minor nice-to-have -- probably as a new method `assertPlansEquivalent(plan1, plan2, summary)` that does `if (!RT.arePlansEquivalent(p1, p2)) { Assert.fail("Plans differ: "+summary+"\n", plan1, plan2); }`
---
[GitHub] brooklyn-server issue #969: Add registered types test
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/969
I'm uncomfortable with the changes in third commit (https://github.com/apache/brooklyn-server/pull/969/commits/e6dee4e591cb2d14acb24a9270a01d2c4bbf5e4b). Legacy plans that have been stored with the text-based comparison might fail. Probably not, as Brooklyn will have been restarted and these tags regenerated so it's probably okay, but the minor efficiency gain of only computing one hash seems not worth it. I think the code only gets invoked when plans have identical IDs so it will rarely make a difference -- thus err on side of correctness not speed.
Suggest a comment to the effect of your idea, which I think on balance is probably right, but just I'm not certain and caution seems the better part of valor here.
---
[GitHub] brooklyn-server pull request #969: Add registered types test
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/969#discussion_r193327856
--- Diff: core/src/test/java/org/apache/brooklyn/core/typereg/RegisteredTypesTest.java ---
@@ -0,0 +1,123 @@
+package org.apache.brooklyn.core.typereg;
+
+import org.apache.brooklyn.api.typereg.BrooklynTypeRegistry;
+import org.apache.brooklyn.api.typereg.RegisteredType;
+import org.testng.annotations.Test;
+
+import javax.annotation.Nullable;
+
+import static org.testng.Assert.*;
+
+public class RegisteredTypesTest {
+
+ public static final String DRBD_YAML = "brooklyn.catalog:\n" +
--- End diff --
also they would be easier to read if you used `Strings.lines()` to join them
---
[GitHub] brooklyn-server pull request #969: Add registered types test
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/brooklyn-server/pull/969
---
[GitHub] brooklyn-server pull request #969: Add registered types test
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/969#discussion_r193328474
--- Diff: core/src/test/java/org/apache/brooklyn/core/typereg/RegisteredTypesTest.java ---
@@ -0,0 +1,123 @@
+package org.apache.brooklyn.core.typereg;
+
+import org.apache.brooklyn.api.typereg.BrooklynTypeRegistry;
+import org.apache.brooklyn.api.typereg.RegisteredType;
+import org.testng.annotations.Test;
+
+import javax.annotation.Nullable;
+
+import static org.testng.Assert.*;
+
+public class RegisteredTypesTest {
+
+ public static final String DRBD_YAML = "brooklyn.catalog:\n" +
+ " version: \"1.0.0-SNAPSHOT\" # BROOKLYN_DRBD_VERSION\n" +
+ " publish:\n" +
+ " description: |\n" +
+ " DRBD is a distributed replicated storage system for the Linux platform. It is implemented as a kernel driver, \n" +
+ " several userspace management applications, and some shell scripts. DRBD is traditionally used in high availability (HA) computer clusters to provide a\n" +
+ " RAID 1 like configuration over a network.\n" +
+ " license_code: Apache-2.0\n" +
+ " defaults:\n" +
+ " drbdIconUrl: \"https://s3.eu-central-1.amazonaws.com/misc-csft/drbd.jpg\"\n" +
+ " classpathIconUrl: &drbdIconUrl 'classpath://io.brooklyn.drbd.brooklyn-drbd:drbd.png'\n" +
+ " items:\n" +
+ " - \"https://github.com/brooklyncentral/common-catalog-utils/releases/download/v0.1.0/common.bom\"\n" +
+ " - id: drbd-node\n";
+
+ public static final String DRBD_TESTS_YAML = "brooklyn.catalog:\n" +
+ " version: \"1.0.0-SNAPSHOT\" # BROOKLYN_DRBD_VERSION\n" +
+ " items:\n" +
+ " - https://github.com/brooklyncentral/common-catalog-utils/releases/download/v0.1.0/common.tests.bom\n" +
+ " - id: drbd-test\n" +
+ " name: DRBD Test\n" +
+ " itemType: template\n" +
+ " iconUrl: https://s3.eu-central-1.amazonaws.com/misc-csft/drbd.jpg\n" +
+ " license: Apache-2.0\n" +
+ " item:\n" +
+ " booklyn.config:\n" +
+ " defaultDisplayName: DRBD Test\n" +
+ " services:\n" +
+ " - type: drbd-webapp\n";
+
+ public static final String DRBR_YAML_WITH_COMMENTS = DRBD_YAML + "\n" +
+ "# this is a comment";
+ private static final String DRBR_YAML_WITHOUT_QUOTES = "brooklyn.catalog:\n" +
+ " version: 1.0.0-SNAPSHOT # BROOKLYN_DRBD_VERSION\n" +
+ " publish:\n" +
+ " description: |\n" +
+ " DRBD is a distributed replicated storage system for the Linux platform. It is implemented as a kernel driver, \n" +
+ " several userspace management applications, and some shell scripts. DRBD is traditionally used in high availability (HA) computer clusters to provide a\n" +
+ " RAID 1 like configuration over a network.\n" +
+ " license_code: Apache-2.0\n" +
+ " defaults:\n" +
+ " drbdIconUrl: https://s3.eu-central-1.amazonaws.com/misc-csft/drbd.jpg\n" +
+ " classpathIconUrl: &drbdIconUrl 'classpath://io.brooklyn.drbd.brooklyn-drbd:drbd.png'\n" +
+ " items:\n" +
+ " - https://github.com/brooklyncentral/common-catalog-utils/releases/download/v0.1.0/common.bom\n" +
+ " - id: drbd-node\n";
+
+ @Test
+ public void testIdenticalPlansAreEquivalent() {
+ BasicRegisteredType a = makeTypeWithYaml(DRBD_YAML);
+
+ BasicRegisteredType b = makeTypeWithYaml(DRBD_YAML);
+
+ boolean plansEquivalent = RegisteredTypes.arePlansEquivalent(
+ a, b);
+ assertTrue(plansEquivalent);
--- End diff --
worth adding the extra `message` parameter here to give more of an explanation if the test fails
---
[GitHub] brooklyn-server issue #969: Add registered types test
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/969
@duncangrant can you respond to https://github.com/apache/brooklyn-server/pull/969#issuecomment-396177108 before we merge?
---
[GitHub] brooklyn-server pull request #969: Add registered types test
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/969#discussion_r193327506
--- Diff: core/src/test/java/org/apache/brooklyn/core/typereg/RegisteredTypesTest.java ---
@@ -0,0 +1,123 @@
+package org.apache.brooklyn.core.typereg;
+
+import org.apache.brooklyn.api.typereg.BrooklynTypeRegistry;
+import org.apache.brooklyn.api.typereg.RegisteredType;
+import org.testng.annotations.Test;
+
+import javax.annotation.Nullable;
+
+import static org.testng.Assert.*;
+
+public class RegisteredTypesTest {
+
+ public static final String DRBD_YAML = "brooklyn.catalog:\n" +
--- End diff --
It's hard to tell when reading the code what the significant differences are between these various plans - think it would be worth cutting them down to something smaller where you could read the code and see more easily what comparison you can make.
---
[GitHub] brooklyn-server issue #969: Add registered types test
Posted by duncangrant <gi...@git.apache.org>.
Github user duncangrant commented on the issue:
https://github.com/apache/brooklyn-server/pull/969
@ahgittin in response to your comment https://github.com/apache/brooklyn-server/pull/969#issuecomment-396177108 I have removed that commit from the PR as I wasn't really comfortable with it either. The reason I wanted to make that change was just to simplify the function in terms of size and readability. That's actually why I added the tests - to try to get some sort of control of the function.
I don't see much value in adding a comment of the form - would like to simplify this but too high risk.
---
[GitHub] brooklyn-server issue #969: Add registered types test
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/969
> I don't see much value in adding a comment of the form - would like to simplify this but too high risk.
lol
nice one thx @duncangrant @geomacy
---
[GitHub] brooklyn-server issue #969: Add registered types test
Posted by duncangrant <gi...@git.apache.org>.
Github user duncangrant commented on the issue:
https://github.com/apache/brooklyn-server/pull/969
@geomacy regarding the naming I don't think tags is relevant to the tests. The mechanism for comparison could be changed later without changing the tests. I see this as a file of tests for some of the utility functions in the RegisteredTypes class and it would be fine to add tests for other utility functions here.
---