You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by aledsage <gi...@git.apache.org> on 2015/03/18 14:42:02 UTC
[GitHub] incubator-brooklyn pull request: Fix UsageResourceTest.testListApp...
GitHub user aledsage opened a pull request:
https://github.com/apache/incubator-brooklyn/pull/558
Fix UsageResourceTest.testListApplicationUsages
Context: this test is checking that the REST api for application-usage will report usage in the specified time range. If we ask for usage between `Wed Mar 18 11:45:25 GMT 2015` and `Wed Mar 18 11:45:25 GMT 2015` then we expect to be told about all apps that were in use during that single second (and not to be told about the usage of those apps outside of that specific second).
- Was failing occasionally if the requested start/end date rounded down to a different value than the postStart date.
- I suspect 1 in 1000 chance, because depended if afterPostStart was in a different second from postStart - it was adding 1ms to it. I suspect the original intent was to add 1 second.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/aledsage/incubator-brooklyn fix/UsageResourceTest
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-brooklyn/pull/558.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 #558
----
commit 1c9479b8804239c782ba05278d63c18771c93b98
Author: Aled Sage <al...@gmail.com>
Date: 2015-03-18T13:38:59Z
Fix UsageResourceTest.testListApplicationUsages
- Was failing occasionally if the requested start/end date rounded down
to a different value than the postStart date.
- I suspect 1 in 1000 chance, because depended if afterPostStart was in
a different second from postStart - it was adding 1ms to it.
I suspect the original intent was to add 1 second.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Fix UsageResourceTest.testListApp...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-brooklyn/pull/558
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Fix UsageResourceTest.testListApp...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/558#issuecomment-83590315
You are right, the comparison is inclusive. LGTM with or without the extra ms.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Fix UsageResourceTest.testListApp...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/558#discussion_r26755841
--- Diff: usage/rest-server/src/test/java/brooklyn/rest/resources/UsageResourceTest.java ---
@@ -426,6 +432,10 @@ public void release(SshMachineLocation machine) {
}
}
+ private void waitForFuture(Date futureTime) throws InterruptedException {
--- End diff --
Looks like this is not used any more?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Fix UsageResourceTest.testListApp...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/558#issuecomment-83583488
Thanks @neykov - I've incorporated your suggestions.
Not sure I entirely follow. We're using `.compareTo(...) > 0` and `.compareTo(...) < 0`, so they should cope fine with it being equal. It should always return us event details about `startDate` to `endDate` (i.e. `afterPostStart` to `afterPostStart`, because that is what we're now passing in as the time range).
I've fine with the test taking an extra 1ms to be cautious though.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Fix UsageResourceTest.testListApp...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/558#issuecomment-83057921
I was the last one who took a stab at fixing this test so can provide some context.
The 1ms delay is on purpose due to the check in `UsageResource`
```
if (eventStartDate.compareTo(endDate) > 0 || eventEndDate.compareTo(startDate) < 0) {
continue;
}
```
We have to make sure that we are requesting times at least 1ms *after* `eventStartDate` (thus the +1ms). The `waitForFuture(postStart+1ms)` is because `eventEndDate=new Date()`, to make sure that the requested time is not in the future. The comparison is done on the full values, they are cropped at serialization time only, so no need to add 1s.
Your other half of the fix is correct though, the test should assert against `afterPostStart` due to
```
if (eventStartDate.compareTo(startDate) < 0) {
eventStartDate = startDate;
}
if (eventEndDate.compareTo(endDate) > 0) {
eventEndDate = endDate;
}
```
I can see another potential problem. It could (quite unlikely) happen that `afterPostStart` is equal to `eventEndDate = new Date()` Due to the `eventEndDate.compareTo(startDate) < 0` the test will fail. To make sure this doesn't happen I'd suggest changing the original test to
```
long afterPostStart = postStart.getTime()+1;
waitForFuture(afterPostStart+1);
```
so that we are completely sure that `afterPostStart` is at least 1ms after the entity start time and at least 1ms before `new Date()`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---