You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by neykov <gi...@git.apache.org> on 2015/06/19 17:33:47 UTC
[GitHub] incubator-brooklyn pull request: Fix integration tests (core and s...
GitHub user neykov opened a pull request:
https://github.com/apache/incubator-brooklyn/pull/700
Fix integration tests (core and software-base)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/neykov/incubator-brooklyn fix/integration-tests
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-brooklyn/pull/700.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 #700
----
commit fae666cfbc0502af7b0afeac135c4604fd429bf9
Author: Svetoslav Neykov <sv...@cloudsoftcorp.com>
Date: 2015-06-18T10:00:15Z
Fix integration tests - brooklyn-core
commit 9ad0d8c8e7dfba53acdd3a3895d0ba8fd5661d7d
Author: Svetoslav Neykov <sv...@cloudsoftcorp.com>
Date: 2015-06-19T10:07:26Z
Fix integration tests - brooklyn-software-base
----
---
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 integration tests (core and s...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/700#issuecomment-113638225
This is caused by a "fix" in this PR. Will correct in a new PR.
Looks like it took 5 hours for the build, and completed only a couple of minutes after the merge :).
---
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 integration tests (core and s...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/700#issuecomment-113633349
@neykov thanks, great improvements/fixes!
Merging this now, so we can get the benefits of the fixes in the next test runs. Any review comments that you think deserve code changes can be done later.
---
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 integration tests (core and s...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/700#discussion_r32864345
--- Diff: core/src/main/java/brooklyn/util/internal/ssh/ShellAbstractTool.java ---
@@ -296,9 +296,10 @@ public ToolAbstractAsyncExecScript(Map<String,?> props) {
* The executed command will return immediately, but the output from the script
* will continue to be written
* note that some modes require \$RESULT passed in order to access a variable, whereas most just need $ */
+ @Override
protected List<String> buildRunScriptCommand() {
String touchCmd = String.format("touch %s %s %s %s", stdoutPath, stderrPath, exitStatusPath, pidPath);
- String cmd = String.format("( %s > %s 2> %s < /dev/null ; echo $? > %s ) & disown", scriptPath, stdoutPath, stderrPath, exitStatusPath);
--- End diff --
Your change makes sense, I think. It was probably fine before, because stdin is /dev/null and and stdout is the file.
As it says in [1], "if the terminal is destroyed..., the program will fail as soon as it tries to read from standard input or write from standard output." Other than that, it's very similar behaviour (from a user's perspective) to `nohup ... &`.
@richardcloudsoft anything to add?
[1] http://unix.stackexchange.com/questions/3886/difference-between-nohup-disown-and
---
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 integration tests (core and s...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/700#issuecomment-113639486
Ah, yes. It works for me with your `http://0.0.0.0` change (which makes perfect sense). I guess asfgit's environment behaves differently.
Very happy to leave this with you to investigate @neykov !
---
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 integration tests (core and s...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/700#issuecomment-113647042
Comments addressed and failure fixed in https://github.com/apache/incubator-brooklyn/pull/702
---
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 integration tests (core and s...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/700#discussion_r32865690
--- Diff: core/src/test/java/brooklyn/event/feed/http/HttpFeedTest.java ---
@@ -300,24 +300,11 @@ public void testStartSuspended() throws Exception {
}
- @Test(groups="Integration")
- /** marked as integration so it doesn't fail the plain build in environments
- * with dodgy DNS (ie where "unresolvable_hostname_or_one_with_no_webserver_on_port_80" resolves as a host run by the provider)
- * <p>
- * (a surprising number of ISP's do this,
- * happily serving adverts for your ISP, yielding "success" here,
- * or timing out, giving null here)
- * <p>
- * if you want to make this test work, you can e.g. set it to loopback IP assuming you don't have any servers on port 80,
- * with the following in /etc/hosts
- * <p>
- * 127.0.0.1 unresolvable_hostname_or_one_with_no_webserver_on_port_80
- // or some other IP which won't resolve
- */
+ @Test
public void testPollsAndParsesHttpErrorResponseWild() throws Exception {
feed = HttpFeed.builder()
.entity(entity)
- .baseUri("http://unresolvable_hostname_or_one_with_no_webserver_on_port_80")
+ .baseUri("http://0.0.0.0")
--- End diff --
Nice!
---
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 integration tests (core and s...
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/700#discussion_r32869026
--- Diff: core/src/test/java/brooklyn/location/geo/HostGeoLookupIntegrationTest.java ---
@@ -34,6 +35,7 @@
public static final Logger log = LoggerFactory.getLogger(HostGeoLookupIntegrationTest.class);
+ // Needs fast network connectivity to figure out the external IP. If response not returned in 2s fails.
--- End diff --
Can't wait longer/differentiate the result here. It's designed to avoid long blocks on purpose and fail gracefully.
We are already testing all implementations separately so it's worth investigating if we can use stubs here to test the higher level logic.
---
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 integration tests (core and s...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-brooklyn/pull/700
---
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 integration tests (core and s...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/700#issuecomment-113637001
asfgit build failure is for an unrelated test: `brooklyn.event.feed.http.HttpFeedTest.testPollsAndParsesHttpErrorResponseWild`. We should fix that in a separate PR.
---
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 integration tests (core and s...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/700#discussion_r32864523
--- Diff: core/src/main/java/brooklyn/location/basic/SshMachineLocation.java ---
@@ -876,7 +877,8 @@ public boolean isSshable() {
String cmd = "date";
try {
try {
- Socket s = new Socket(getAddress(), getPort());
+ Socket s = new Socket();
+ s.connect(new InetSocketAddress(getAddress(), getPort()), SSHABLE_CONNECT_TIMEOUT);
--- End diff --
Makes sense. Could overload `isSshable()` to allow a custom timeout to be passed in, but not sure where we'd make use of that. So happy to stick with your change.
---
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 integration tests (core and s...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/700#discussion_r32866045
--- Diff: core/src/test/java/brooklyn/location/geo/HostGeoLookupIntegrationTest.java ---
@@ -34,6 +35,7 @@
public static final Logger log = LoggerFactory.getLogger(HostGeoLookupIntegrationTest.class);
+ // Needs fast network connectivity to figure out the external IP. If response not returned in 2s fails.
--- End diff --
Interesting. Longer term do you think it's right to fail? Is it possible to differentiate between "still waiting" and failed? Could we wait longer? Or could we throw `SkipException` if we know that it's not a true failure?
Certainly happy with as-is for this release.
---
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 integration tests (core and s...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/700#discussion_r32865777
--- Diff: core/src/test/java/brooklyn/location/basic/SshMachineLocationIntegrationTest.java ---
@@ -63,7 +63,14 @@ public void tearDown() throws Exception {
mgmt = null;
}
- // Note: requires `named:localhost-passphrase` set up with a key whose passphrase is "localhost"
+ // Note: requires `named:localhost-passphrase` set up with a key whose passphrase is "localhost"
+ // * create the key with:
--- End diff --
Excellent! We need more of this kind of thing, and to get in the habit of writing it at the same time as the test (me included!).
---
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 integration tests (core and s...
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/700#discussion_r32840547
--- Diff: core/src/main/java/brooklyn/util/internal/ssh/ShellAbstractTool.java ---
@@ -296,9 +296,10 @@ public ToolAbstractAsyncExecScript(Map<String,?> props) {
* The executed command will return immediately, but the output from the script
* will continue to be written
* note that some modes require \$RESULT passed in order to access a variable, whereas most just need $ */
+ @Override
protected List<String> buildRunScriptCommand() {
String touchCmd = String.format("touch %s %s %s %s", stdoutPath, stderrPath, exitStatusPath, pidPath);
- String cmd = String.format("( %s > %s 2> %s < /dev/null ; echo $? > %s ) & disown", scriptPath, stdoutPath, stderrPath, exitStatusPath);
--- End diff --
This command was working fine with the tests, but using `disown` while ending the ssh session doesn't seem right, so I converted it to use `nohup`. Any reason it shouldn't be changed? cc @aledsage
---
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 integration tests (core and s...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/700#discussion_r32866496
--- Diff: software/base/src/test/java/brooklyn/entity/basic/lifecycle/StartStopSshDriverTest.java ---
@@ -113,10 +116,10 @@ public void testExecuteDoesNotLeaveRunningStreamGobblerThread() {
@Override
public void run() {
List<ThreadInfo> currentThreads = getThreadsCalling(StreamGobbler.class);
- List<Long> currentThreadIds = getThreadId(currentThreads);
+ Collection<Long> currentThreadIds = MutableSet.copyOf(getThreadId(currentThreads));
--- End diff --
Personal preference to declare as `Set<Long>`, given line 122 will compare it with a set.
But that's really minor, given that 3 lines earlier we can see it being instantiated!
---
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.
---