STORM-354: Testing: allow users to pass TEST-TIMEOUT-MS as param for complete-topology

[aaronlevin]

It would be nice if complete-topology allowed a user to pass in the default timeout as a parameter (rather than just setting it as 5000ms). I had a test that kept failing by taking too long.

This PR adds functionality without breaking any existing code. Tests pass.

JIRA ticket: https://issues.apache.org/jira/browse/STORM-354

[revans2]

All pull requests need a JIRA associated with them. Could you please file a JIRA if one does not already exist and update the title of this pull request to have the JIRA number in it, i.e. STORM-12345.

The code itself looks fine to me. But I would almost rather have a second defined timeout that is based off of the original timeout. Simply because in complete-topology simulate-wait within a loop. simulate-wait also has the same timeout code in it but in a loop 10 times, so if you get unlucky and get close to the timeout even once in simulate-wait the outer loop will timeout. We could simply have it be 3 * TEST-TIMEOUT-MS

[d2r]

@weirdcanada, any update on this PR?

[aaronlevin]

I just got back from vacation last week. I'll submit the JIRA ticket and update the PR tomorrow (Sunday). Thanks!

[aaronlevin]

@revans2 I've created the JIRA ticket and updated the pull request. Let me take into account your feedback and re-submit a pull request. It may be a couple of days.

[nsj007]

Is there any progress on this. The 5000ms timeout pretty much fails all my test when upgrading from STORM 0.9.01 to apache-storm-0.9.2-incubator

[revans2]

I am really sorry it has taken me so long to review things. The changes look good I am +1, I had to do an upmerge, because of whitespace issues due to a reformatting patch that went in previously.

I'll merge this into master

[vkaggalumn]

Hello!
I am also wondering on how to set this parameter in java? Would somebody comment please?

Thanks

[revans2]

https://github.com/apache/storm/blob/master/storm-core/src/clj/backtype/storm/testing.clj#L190-192

Shows that the environment variable STORM_TEST_TIMEOUT_MS, can be used to set the value globally. We probably should document this better somewhere.

[harshach]

@revans2 its in DEVELOPER.md https://github.com/apache/storm/blob/master/DEVELOPER.md#build-the-code-and-run-the-tests

[revans2]

So it is. I guess I should read the documentation before I start digging though the code :) Thanks @harshach

[vkaggalumn]

Thanks @harshach.

If I am reading the documentation right, I see the following which instructs how to build the storm code base with the STORM_TEST_TIMEOUT_MS set? Being a new user, I am wondering about the this - How should we be setting this (STORM_TEST_TIMEOUT_MS) when using Eclipse to run our tests in our storm project? Or am I not reading this right?

# Build the code and run the tests, with specifying default test timeout (in millisecond)
$ export STORM_TEST_TIMEOUT_MS=10000
$ mvn clean install

Any pointers would be greatly appreciated.

Many thanks!

[harshach]

@vkaggalumn I don't think we tested that part of dev cycle. Its definitely a valid use case. Most of the time storm tests timeout in a build env for unit tests when a user running all the test suite.
I just poked around eclipse you can add Environment variable under Run configurations for a project or set a system wide environment variable for STORM_TEST_TIMEOUT_MS.

[vkaggalumn]

@harshach Sorry for the delay in my response.

Thanks for your response. I had already tried to pass STORM_TEST_TIMEOUT_MS at runtime (Run Configuration-> environment) which did not seem to work. However, I will double check, try again.

Thanks for your help.