MINOR: refactor streams system test class hierachy#2392
MINOR: refactor streams system test class hierachy#2392mjsax wants to merge 2 commits intoapache:trunkfrom
Conversation
|
@enothereska @dguy @guozhangwang Reworking in preparation to add new system tests. Also for |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
LGTM overall, could you trigger one |
|
Refer to this link for build results (access rights to CI server needed): |
|
I did test with For The error I get in stderr is (seems to be unrelated to the changes though -- WDYT?): |
1e70ba7 to
d36791c
Compare
|
Build finished. 2890 tests run, 0 skipped, 1 failed. |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Given that this changes the python code, it'd be good to trigger all system tests, including the smoke one and bounce one before merging. |
|
Also the benchmark test has always ran nightly, that needs to be part of tests to be run (as @mjsax has already done, thanks) |
|
@guozhangwang in an ideal world, this PR and PR: https://github.com/apache/kafka/pull/2391/files should go together. |
|
@enothereska System tests will take about 5 hours, and given that we are going to have nightly builds in another 10 hour, I'd say just watch the upcoming system test after merging this. |
|
LGTM. Merged to trunk and 0.10.2. |
|
@guozhangwang I think this is really bad. System tests for streams take just 5 mins/test, you don't have to run all the system tests, just streams. |
|
I agree with @enothereska, a change like this should not be merged before running the Streams system tests via the branch builder. It doesn't take long since there are a small number of Streams tests. For Core, whenever we change shared security code, we run all the tests before merging even though they take several hours as there is a lot of value in catching issues before the code is merged. |
| if len(self.pids(node)) == 0: | ||
| raise RuntimeError("No process ids recorded") | ||
|
|
||
| def collect_data(self, node): |
There was a problem hiding this comment.
This did not happen intentionally... Will open a PR to fix it.
|
@enothereska @ijuma My apologies, I mis-understood Eno's comment before and was careless for merging the PR. |
| with node.account.monitor_log(self.STDOUT_FILE) as monitor: | ||
| node.account.ssh(self.start_cmd(node)) | ||
| monitor.wait_until('StreamsSmokeTest instance started', timeout_sec=15, err_msg="Never saw message indicating StreamsSmokeTest finished startup on " + str(node.account)) | ||
| monitor.wait_until('StreamsTest instance started', timeout_sec=60, err_msg="Never saw message indicating StreamsTest finished startup on " + str(node.account)) |
There was a problem hiding this comment.
This probably doesn't need to be increased since they start fast.
Author: Matthias J. Sax <matthias@confluent.io> Reviewers: Eno Thereska, Guozhang Wang Closes apache#2392 from mjsax/minor-system-test-rework
No description provided.