Add EntityAsserts and HttpUtils. #994

geomacy · 2015-10-30T12:03:57Z

This are added to allow core code to make assertions on entities without
without having a dependency on TestNG.

For now, deprecate EntityTestUtils and HttpTestUtils, and in the future these
can be removed in favour of the new classes.

This are added to allow core code to make assertions on entities without without having a dependency on TestNG. For now, deprecate EntityTestUtils and HttpTestUtils, and in the future these can be removed in favour of the new classes.

ahgittin · 2015-11-02T15:58:58Z

core/src/main/java/org/apache/brooklyn/core/entity/EntityAsserts.java

+
+
+    @Beta @SafeVarargs
+    public static <T> void assertAttributeNever(final Entity entity, final AttributeSensor<T> attribute, T... disallowed) {


can we call this assertAttributeContinuallyNotEqualTo?

add javadoc noting that this loops checking the value, it does not currently set up a subscription so a disallowed value might come up very quickly and be missed. (we could fix this if needed.)

cleaner would be for this to call the next method, assertAttribute...(Map, ...), with null flags, or perhaps to introduce an assertAttributeContinually(..., Predicate) and build up a predicate of not(or(equalTo(disallowed[0]), equalTo(disallowed[1]), ...))

ahgittin · 2015-11-02T16:05:35Z

need tests for the http assertions i think.

apart from that this looks real good and only minor comments.

Renamed assertAttributeNever to assertAttributeContinuallyNotEqualTo (apache#994 (comment)). Updates as noted to deserializingClassRenames.properties (apache#994 (diff)) Fix package name in comment in EntityTestUtils (apache#994 (diff)) Removed a TODO in EntityTestUtils (apache#994 (diff)) Renamed HttpUtils to HttpAsserts; moved methods that weren't assertions to HttpTool, which involved bringing it out of core and into the utils module (apache#994 (comment))

…t-utils

geomacy · 2015-11-02T20:35:06Z

hi Alex et al. I have made the changes related to the comments by Alex in the commits above.

The only one I haven't yet done is to add unit tests for HttpUtils. I don't want to hold back the others so I propose to do this in a separate pull request. I will get on to the tests for HttpUtils tomorrow morning.

Note that to nicely divide up the HttpAsserts from the non assertion methods I have moved HttpTool out of core and into the utils module. This results in imports changing in numerous files, so change 03dbd28 looks scarier than it really is (47 changed files).

ahgittin · 2015-11-02T22:04:25Z

For backwards compatibility we need to leave an HttpTool class in the old place, and same for HttpToolResponse. I think these can just extend the newly created classes, so that any methods and creation still work. And add the usual @Deprecation markers to these classes.

ahgittin · 2015-11-02T22:07:32Z

very minor, good to go once those two last items are addressed

aledsage · 2015-11-03T20:40:06Z

usage/test-support/src/main/java/org/apache/brooklyn/test/EntityTestUtils.java

+/**
+ * A utility class containing tests on Entities.
+ *
+ * @deprecated Prefer core assertions class org.apache.brooklyn.core.entity.EntityAsserts.


Can you include a "since 0.9.0" in the deprecated javadoc line. Same for anywhere else you've added @deprecated.

@deprecated

Added TrustingSslSocketFactory to deserializingClassRenames.properties (apache#994 (comment)) Added back an HttpTool and HttpToolResponse in the old location, with @deprecated markers, for backward compatibility (apache#994 (comment)) Added "since 0.9.0" to classes I have deprecated (apache#994 (comment))

geomacy · 2015-11-04T09:10:43Z

Thanks @ahgittin and @aledsage for the comments. I have made some further updates which I think address them all, but let me know if I've missed anything.

ahgittin · 2015-11-04T15:43:47Z

looks great, merging

neykov · 2015-11-05T13:42:20Z

utils/common/src/test/java/org/apache/brooklyn/util/HttpAssertsTest.java

+        final String simple = testUri("/simple");
+        HttpAsserts.assertUrlUnreachable(simple);
+        startAfter(2);
+        HttpAsserts.assertContentEventuallyMatches(ImmutableMap.of("timeout", "3s"), testUri("/simple"), "[Oo][Kk]");


Causes test failures on the apache jenkins CI. The servers there are quite overloaded so delays tend not to be exact, usually much longer.
Also tests which have explicit delays are usually marked as integration as those seconds easily add up. Is it possible to restructure the tests so that they don't need fixed sleeps?

A couple of example failures:

https://builds.apache.org/job/incubator-brooklyn-pull-requests/2069/console

https://builds.apache.org/job/incubator-brooklyn-pull-requests/2067/console

geomacy · 2015-11-05T14:27:29Z

The sleeps are there to allow the test HTTP server time to shut down. That's not required in all tests, so I could speed things up a bit by changing the test condition, but it is needed in some cases (for the 'eventually becomes unavailable' type tests). I could add a timeout to the shutdown, but it occurs to me that it might be better to mark these as integration tests, because

(a) they are not going to run any faster (they already take several minutes) , and
(b) the code under test is fairly 'low level' and not likely to break because of other code within Brooklyn changing

so it may not add a lot of value to run these tests on every build, and it will slow things down a bit.

What do you think?

neykov · 2015-11-05T15:01:15Z

Moving them to integration will address the immediate problem so +1.
At some point we will start running the integration tests on apache infrastructure (read - busy, we already do, but they are failing for a number of other reasons), so the same issue will crop up - but can deal with it then.

The code could be restructured such that it doesn't use waits at all:

tests which need non-running server be put in a separate test, which doesn't start the server by default
no sleep on server start, if the server doesn't come up immediately, then always use eventuallySucceeds
for the cases where there's xxxxAfter(5); succeedsEventually, could be changed to failsNow; task = succeedsEventuallyInAnotherTask; startNow(); task.get(). What could be a problem here is that connections to closed ports take a long time to timeout?
The only place where a fixed wait is really needed is tests which expect to fail and have a timeout, those go in integration.

All of the above said, I am perfectly fine with moving all tests to integration as is.

geomacy · 2015-11-05T15:09:41Z

I will move them to integration now and add a note to restructure in future, but I will try to make that the near future while it's fresh in my head. Thanks.

Later task will be to restructure to avoid sleeps where possible, see conversation at apache#994 (comment).

neykov · 2015-11-12T19:57:36Z

core/src/main/java/org/apache/brooklyn/util/core/http/HttpTool.java

+ * @deprecated since 0.9.0. Prefer org.apache.brooklyn.util.http.HttpTool
+ */
+@Deprecated
+public class HttpTool extends org.apache.brooklyn.util.http.HttpTool {


This doesn't make the class backwards compatible because return types will be coming from the new package, while users of this will be expecting the old classes.
HttpTool is used frequently so better handle this gracefully (had a downstream project build failure). @geomacy Can you get the old implementation of HttpTool back here while keeping it marked as @Deprecated. Same for HttpToolResponse.

geomacy · 2015-11-13T15:59:54Z

hi @neykov, sorry I had indeed missed this comment. Will make changes as you say.

geomacy · 2015-11-13T18:03:03Z

Have added the changes in #1032

…ibility. This is in relation to the issue noted at apache#994 (comment) and apache#1032 (comment)

Renamed assertAttributeNever to assertAttributeContinuallyNotEqualTo (apache/incubator-brooklyn#994 (comment)). Updates as noted to deserializingClassRenames.properties (apache/incubator-brooklyn#994 (diff)) Fix package name in comment in EntityTestUtils (apache/incubator-brooklyn#994 (diff)) Removed a TODO in EntityTestUtils (apache/incubator-brooklyn#994 (diff)) Renamed HttpUtils to HttpAsserts; moved methods that weren't assertions to HttpTool, which involved bringing it out of core and into the utils module (apache/incubator-brooklyn#994 (comment))

@deprecated

Added TrustingSslSocketFactory to deserializingClassRenames.properties (apache/incubator-brooklyn#994 (comment)) Added back an HttpTool and HttpToolResponse in the old location, with @deprecated markers, for backward compatibility (apache/incubator-brooklyn#994 (comment)) Added "since 0.9.0" to classes I have deprecated (apache/incubator-brooklyn#994 (comment))

Later task will be to restructure to avoid sleeps where possible, see conversation at apache/incubator-brooklyn#994 (comment).

Renamed assertAttributeNever to assertAttributeContinuallyNotEqualTo (apache/incubator-brooklyn#994 (comment)). Updates as noted to deserializingClassRenames.properties (apache/incubator-brooklyn#994 (diff)) Fix package name in comment in EntityTestUtils (apache/incubator-brooklyn#994 (diff)) Removed a TODO in EntityTestUtils (apache/incubator-brooklyn#994 (diff)) Renamed HttpUtils to HttpAsserts; moved methods that weren't assertions to HttpTool, which involved bringing it out of core and into the utils module (apache/incubator-brooklyn#994 (comment))

@deprecated

Added TrustingSslSocketFactory to deserializingClassRenames.properties (apache/incubator-brooklyn#994 (comment)) Added back an HttpTool and HttpToolResponse in the old location, with @deprecated markers, for backward compatibility (apache/incubator-brooklyn#994 (comment)) Added "since 0.9.0" to classes I have deprecated (apache/incubator-brooklyn#994 (comment))

Later task will be to restructure to avoid sleeps where possible, see conversation at apache/incubator-brooklyn#994 (comment).

Add EntityAsserts and HttpUtils.

c316e21

This are added to allow core code to make assertions on entities without without having a dependency on TestNG. For now, deprecate EntityTestUtils and HttpTestUtils, and in the future these can be removed in favour of the new classes.

ahgittin reviewed Nov 2, 2015
View reviewed changes

ahgittin mentioned this pull request Nov 2, 2015

pass flags to assertContentEventuallyMatches() for setting timeout #995

Merged

m4rkmckenna mentioned this pull request Nov 2, 2015

Implementation of proposed Test Entities to allow testing directly from blueprints #999

Merged

geomacy added 3 commits November 2, 2015 18:59

Merge remote-tracking branch 'mccabe/eventualregex' into decouple-tes…

a1e21b3

…t-utils

Update HttpAsserts with equivalent change to that merged in a1e21b3

70a7189

geomacy added 2 commits November 3, 2015 17:38

Add unit tests for HttpAsserts.

885b6ac

Merge branch 'master' into decouple-test-utils

019666c

aledsage reviewed Nov 3, 2015
View reviewed changes

ahgittin added a commit to ahgittin/incubator-brooklyn that referenced this pull request Nov 4, 2015

This closes apache#994

a504093

asfgit merged commit c466240 into apache:master Nov 4, 2015

neykov reviewed Nov 5, 2015
View reviewed changes

geomacy mentioned this pull request Nov 5, 2015

HTTP connection pooling for BrooklynApi clients #1000

Merged

geomacy added a commit to geomacy/incubator-brooklyn that referenced this pull request Nov 5, 2015

Make the HttpAssertsTest an integration test for now.

1433113

Later task will be to restructure to avoid sleeps where possible, see conversation at apache#994 (comment).

geomacy mentioned this pull request Nov 5, 2015

Make the HttpAssertsTest an integration test for now. #1004

Merged

neykov reviewed Nov 12, 2015
View reviewed changes

geomacy mentioned this pull request Nov 13, 2015

Restore old HttpTool / Response for backward compatibility #1032

Merged

geomacy added a commit to geomacy/incubator-brooklyn that referenced this pull request Nov 16, 2015

Move HttpTool and adjuncts back into core to preserve backward compat…

d9ed4fa

…ibility. This is in relation to the issue noted at apache#994 (comment) and apache#1032 (comment)

johnmccabe mentioned this pull request Nov 26, 2015

Updates to support Clocker with Brooklyn 0.9.0 brooklyncentral/advanced-networking#85

Merged

asfgit pushed a commit to apache/brooklyn-server that referenced this pull request Feb 1, 2016

Make the HttpAssertsTest an integration test for now.

3033288

Later task will be to restructure to avoid sleeps where possible, see conversation at apache/incubator-brooklyn#994 (comment).

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add EntityAsserts and HttpUtils. #994

Add EntityAsserts and HttpUtils. #994

geomacy commented Oct 30, 2015

ahgittin Nov 2, 2015

ahgittin commented Nov 2, 2015

geomacy commented Nov 2, 2015

ahgittin commented Nov 2, 2015

ahgittin commented Nov 2, 2015

aledsage Nov 3, 2015

geomacy commented Nov 4, 2015

ahgittin commented Nov 4, 2015

neykov Nov 5, 2015

neykov Nov 5, 2015

geomacy commented Nov 5, 2015

neykov commented Nov 5, 2015

geomacy commented Nov 5, 2015

neykov Nov 12, 2015

geomacy commented Nov 13, 2015

geomacy commented Nov 13, 2015



		@Beta @SafeVarargs
		public static <T> void assertAttributeNever(final Entity entity, final AttributeSensor<T> attribute, T... disallowed) {

Add EntityAsserts and HttpUtils. #994

Add EntityAsserts and HttpUtils. #994

Conversation

geomacy commented Oct 30, 2015

ahgittin Nov 2, 2015

Choose a reason for hiding this comment

ahgittin commented Nov 2, 2015

geomacy commented Nov 2, 2015

ahgittin commented Nov 2, 2015

ahgittin commented Nov 2, 2015

aledsage Nov 3, 2015

Choose a reason for hiding this comment

geomacy commented Nov 4, 2015

ahgittin commented Nov 4, 2015

neykov Nov 5, 2015

Choose a reason for hiding this comment

neykov Nov 5, 2015

Choose a reason for hiding this comment

geomacy commented Nov 5, 2015

neykov commented Nov 5, 2015

geomacy commented Nov 5, 2015

neykov Nov 12, 2015

Choose a reason for hiding this comment

geomacy commented Nov 13, 2015

geomacy commented Nov 13, 2015