fix broken sensor order test, clarify expected behaviour
Project: http://git-wip-us.apache.org/repos/asf/brooklyn-server/repo Commit: http://git-wip-us.apache.org/repos/asf/brooklyn-server/commit/16a44c25 Tree: http://git-wip-us.apache.org/repos/asf/brooklyn-server/tree/16a44c25 Diff: http://git-wip-us.apache.org/repos/asf/brooklyn-server/diff/16a44c25 Branch: refs/heads/master Commit: 16a44c25a0908dc4ce68bdcdd3e108b6c1fba259 Parents: bc0b392 Author: Alex Heneveld <[email protected]> Authored: Mon Oct 23 09:25:37 2017 +0100 Committer: Alex Heneveld <[email protected]> Committed: Mon Oct 23 09:25:37 2017 +0100 ---------------------------------------------------------------------- .../apache/brooklyn/core/entity/Entities.java | 1 - .../stock/AbstractMultipleSensorAggregator.java | 9 +- .../ApplicationLifecycleStateStressTest.java | 11 ++- .../entity/ApplicationLifecycleStateTest.java | 99 ++++++++++++++------ 4 files changed, 85 insertions(+), 35 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/16a44c25/core/src/main/java/org/apache/brooklyn/core/entity/Entities.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/brooklyn/core/entity/Entities.java b/core/src/main/java/org/apache/brooklyn/core/entity/Entities.java index 02ea9b3..2e38a34 100644 --- a/core/src/main/java/org/apache/brooklyn/core/entity/Entities.java +++ b/core/src/main/java/org/apache/brooklyn/core/entity/Entities.java @@ -76,7 +76,6 @@ import org.apache.brooklyn.core.sensor.DependentConfiguration; import org.apache.brooklyn.util.collections.MutableMap; import org.apache.brooklyn.util.collections.MutableSet; import org.apache.brooklyn.util.core.ResourceUtils; -import org.apache.brooklyn.util.core.config.ConfigBag; import org.apache.brooklyn.util.core.task.DynamicTasks; import org.apache.brooklyn.util.core.task.ParallelTask; import org.apache.brooklyn.util.core.task.TaskTags; http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/16a44c25/core/src/main/java/org/apache/brooklyn/enricher/stock/AbstractMultipleSensorAggregator.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/brooklyn/enricher/stock/AbstractMultipleSensorAggregator.java b/core/src/main/java/org/apache/brooklyn/enricher/stock/AbstractMultipleSensorAggregator.java index 7de27ca..8249348 100644 --- a/core/src/main/java/org/apache/brooklyn/enricher/stock/AbstractMultipleSensorAggregator.java +++ b/core/src/main/java/org/apache/brooklyn/enricher/stock/AbstractMultipleSensorAggregator.java @@ -18,7 +18,6 @@ */ package org.apache.brooklyn.enricher.stock; -import java.util.Collection; import java.util.Collections; import java.util.LinkedHashMap; import java.util.Map; @@ -102,7 +101,13 @@ public abstract class AbstractMultipleSensorAggregator<U> extends AbstractAggreg initialVal = null; } vs.put(producer, initialVal != null ? initialVal : defaultMemberValue); - // NB: see notes on possible race, in Aggregator#onProducerAdded + // there may be sensor events older than initialVal because this is run in + // the _parents_ synched subscription block. we live with this for now, + // as those sensor events will catch up, but val here might see + // second -> first -> second on child publishing second -> first . + // could resolve by running this init in the subscription manager thread; + // but it's not necessary yet; see Aggregator#onProducerAdded and + // ApplicationLifecycleStateTest#testSensorInitAndPublishOrder } } } http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/16a44c25/core/src/test/java/org/apache/brooklyn/core/entity/ApplicationLifecycleStateStressTest.java ---------------------------------------------------------------------- diff --git a/core/src/test/java/org/apache/brooklyn/core/entity/ApplicationLifecycleStateStressTest.java b/core/src/test/java/org/apache/brooklyn/core/entity/ApplicationLifecycleStateStressTest.java index aa1790d..581957a 100644 --- a/core/src/test/java/org/apache/brooklyn/core/entity/ApplicationLifecycleStateStressTest.java +++ b/core/src/test/java/org/apache/brooklyn/core/entity/ApplicationLifecycleStateStressTest.java @@ -26,6 +26,7 @@ public class ApplicationLifecycleStateStressTest extends ApplicationLifecycleSta // TODO Is there a nicer way to get all the super-class public methods to run with the // invocationCount defined on this class, rather than the simple `@Test` annotation on // the super-class (without redefining all the public methods here!)? + // Alex replying: I don't think so, unless you possibly ran the "parent" as suite 100 times @Override public void testHappyPathEmptyApp() throws Exception { @@ -87,10 +88,14 @@ public class ApplicationLifecycleStateStressTest extends ApplicationLifecycleSta super.testSettingSensorFromThreads(); } - @Test(groups="Broken") @Override - public void testWrongSensorInitValue() { - super.testWrongSensorInitValue(); + public void testSensorInitAndPublishOrder() { + super.testSensorInitAndPublishOrder(); + } + + @Override + public void testSensorInitAndPublishOrderNonStrict() { + super.testSensorInitAndPublishOrderNonStrict(); } @Override http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/16a44c25/core/src/test/java/org/apache/brooklyn/core/entity/ApplicationLifecycleStateTest.java ---------------------------------------------------------------------- diff --git a/core/src/test/java/org/apache/brooklyn/core/entity/ApplicationLifecycleStateTest.java b/core/src/test/java/org/apache/brooklyn/core/entity/ApplicationLifecycleStateTest.java index 470d0bf..4c5a09e 100644 --- a/core/src/test/java/org/apache/brooklyn/core/entity/ApplicationLifecycleStateTest.java +++ b/core/src/test/java/org/apache/brooklyn/core/entity/ApplicationLifecycleStateTest.java @@ -19,12 +19,10 @@ package org.apache.brooklyn.core.entity; import static org.testng.Assert.assertEquals; -import static org.testng.Assert.assertTrue; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; -import java.util.Iterator; import java.util.List; import java.util.Map; import java.util.concurrent.atomic.AtomicReference; @@ -57,6 +55,7 @@ import org.apache.brooklyn.util.collections.QuorumCheck; import org.apache.brooklyn.util.core.task.ValueResolver; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import org.testng.Assert; import org.testng.annotations.Test; import com.google.common.base.Predicates; @@ -65,7 +64,6 @@ import com.google.common.collect.FluentIterable; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Iterables; -import com.google.common.collect.Ordering; @Test public class ApplicationLifecycleStateTest extends BrooklynMgmtUnitTestSupport { @@ -272,7 +270,8 @@ public class ApplicationLifecycleStateTest extends BrooklynMgmtUnitTestSupport { public static class RecodingChildSensorEnricher extends AbstractMultipleSensorAggregator<Void> { public static AttributeSensor<String> RECORDED_SENSOR = Sensors.newStringSensor("recorded.sensor"); - List<String> seenValues = new ArrayList<>(); + List<String> seenValuesAllCollected = Collections.synchronizedList(new ArrayList<>()); + List<String> seenValuesPublished = Collections.synchronizedList(new ArrayList<>()); @Override protected Collection<Sensor<?>> getSourceSensors() { @@ -289,44 +288,72 @@ public class ApplicationLifecycleStateTest extends BrooklynMgmtUnitTestSupport { } @Override + public void onEvent(SensorEvent<Object> event) { + super.onEvent(event); + + if (event.getSensor().equals(RECORDED_SENSOR)) { + seenValuesPublished.add((String)event.getValue()); + } + } + + @Override protected void onUpdated() { - Iterator<String> values = getValues(RECORDED_SENSOR).values().iterator(); - if (values.hasNext()) { - seenValues.add(values.next()); + // called in onEvent, this is longwinded way to add the event.value we are interested in + Collection<String> vv = getValues(RECORDED_SENSOR).values(); + if (!vv.isEmpty()) { + seenValuesAllCollected.add(vv.iterator().next()); } } } - /** - * Enricher sees state in the wrong order -> running, starting, running - * - * Indeterministic, fails a couple of times per 100 invocations when run with "mvn test" in the - * brooklyn-itest docker container. - */ - @Test(groups="Broken") - public void testWrongSensorInitValue() { + @Test + public void testSensorInitAndPublishOrderNonStrict() { + // this mirrors how most code is working - the initial values seen for an added child may be later than other published values + doTestSensorInitAndPublishOrder(false); + } + + @Test + public void testSensorInitAndPublishOrder() { + // this enforces canonical order by waiting on expected events + doTestSensorInitAndPublishOrder(true); + } + + private void doTestSensorInitAndPublishOrder(boolean strict) { TestApplication app = mgmt.getEntityManager().createEntity(EntitySpec.create(TestApplication.class) .impl(TestApplicationNoEnrichersImpl.class) .enricher(EnricherSpec.create(RecodingChildSensorEnricher.class)) .child(EntitySpec.create(TestEntity.class) .impl(TestEntityNoEnrichersImpl.class))); + + final RecodingChildSensorEnricher enricher = getFirstEnricher(app, RecodingChildSensorEnricher.class); + if (strict) { + Asserts.eventually(Suppliers.ofInstance(enricher.seenValuesAllCollected), CollectionFunctionals.sizeEquals(2)); + } Entity child = Iterables.get(app.getChildren(), 0); child.sensors().set(RecodingChildSensorEnricher.RECORDED_SENSOR, "first"); child.sensors().set(RecodingChildSensorEnricher.RECORDED_SENSOR, "second"); - - final RecodingChildSensorEnricher enricher = getFirstEnricher(app, RecodingChildSensorEnricher.class); - - // setEntity -> onUpdate - // CHILD_ADDED -> onUpdate - // set RECORDED_SENSOR=first -> onUpdate - // set RECORDED_SENSOR=second -> onUpdate - Asserts.eventually(Suppliers.ofInstance(enricher.seenValues), CollectionFunctionals.sizeEquals(4)); - - boolean isOrdered = Ordering.explicit(MutableList.of("first", "second")) - .nullsFirst() - .isOrdered(enricher.seenValues); - assertTrue(isOrdered, "Unexpected ordering for " + enricher.seenValues); + + // usually: + // setEntity -> onUpdate (sees current value) + // CHILD_ADDED -> onUpdate (sees current value) + // set RECORDED_SENSOR=first -> onUpdate (sees event) + // set RECORDED_SENSOR=second -> onUpdate (sees event) + + Asserts.eventually(Suppliers.ofInstance(enricher.seenValuesAllCollected), CollectionFunctionals.sizeEquals(4)); + + Assert.assertEquals(enricher.seenValuesPublished, MutableList.of("first", "second"), + "Unexpected ordering for vP " + enricher.seenValuesPublished + " (vC "+enricher.seenValuesAllCollected+")"); + + List<String> vC = MutableList.copyOf(enricher.seenValuesAllCollected); + if (!strict) { + // CHILD_ADDED (and other handlers) are running in parallel to this thread, however, + // their order is guaranteed to be the above, but it is in parallel to this main thread, thus the current value + // it takes can be any of null, first, or second, unless we run in strict mode + vC.set(1, null); + } + Assert.assertEquals(vC, MutableList.of(null, null, "first", "second"), + "Unexpected ordering for vC " + enricher.seenValuesAllCollected + " (vP "+enricher.seenValuesPublished+")"); } public static class EmittingEnricher extends AbstractEnricher { @@ -428,5 +455,19 @@ public class ApplicationLifecycleStateTest extends BrooklynMgmtUnitTestSupport { .first() .get(); } - + + // for running the tests below many times and stopping when they fail; + // they seem happy (and this breaks the subclass @Test class annotation) + // but kept as comment just in case +// public static void main(String[] args) throws Exception { +// int i = 0; +// while (true) { +// log.info("test "+(++i)); +// ApplicationLifecycleStateTest l = new ApplicationLifecycleStateTest(); +// l.setUp(); +// l.testSensorInitAndPublishOrder(); +// l.testSensorInitAndPublishOrderNonStrict(); +// l.tearDown(); +// } +// } }
