[TEST] Make sure we restart the suite cluster after each test failure#9015
[TEST] Make sure we restart the suite cluster after each test failure#9015javanna wants to merge 1 commit intoelastic:masterfrom
Conversation
There was a problem hiding this comment.
it was missing, I fixed it :)
|
I left some minor comments, otherwise LGTM. This does adds more complexity to our already complex test infra; I wonder if there is a simpler way to "run something after the test failed" in JUnit. E.g. is tearDown invoked after the listeners see the test failure? Or maybe instead of a Listener we should be making a TestRule and insert it at the right place, to record the failure? I don't know JUnit/RandomizedTest well enough but it seems like there could be a simpler fix here ... |
|
I totally agree @mikemccand I spent some time trying to think of alternatives but this is the only one I came up with, and it does add complexity. This is kind of a catch-22 problem, since we want to do things after test, depending on two events: 1) whether something went wrong in the after test 2) whether the test failed, which we get to know only later on either through listeners or rules.
the @after methods in the class are invoked before any other after method (either rules or listeners)
I did turn the That said I am definitely open to alternative solutions! |
|
Thanks @javanna I think we should just commit this approach since it works, and simplify it later. After, tearDown, RunListener, TestRule/RuleChain: it's too much ;) |
045a675 to
60bca6b
Compare
18d2f16 to
09944b5
Compare
|
I just rebased this again and applied feedback. Also removed any mention of the global cluster since it was removed. This PR is still valid for suite level clusters though, as we currently never restart the cluster after any failure although the code suggests that we should. I would like to get this in if it still makes sense to restart the cluster after each failure, to make sure that subsequent tests get a clean new cluster. Thoughts? Review please? |
09944b5 to
8b2d6a9
Compare
|
LGTM |
CurrentTestFailedMarker is a RunListener that gets notified whenever a test fails, and we were using it to be able to restart the global cluster after each failure. We were checking whether a test had failed in the @after method though, which runs before the listener gets notified, so the failed flag would always be false. This commit makes sure that the global cluster gets restarted not only when there are problems in the afterInternal method, but also after each test failure. In order to achieve this, we need to reset the cluster afterwards, when we get to know about both of the events (problem in afterInternal and test failure), and before resetting the currentCluster. Introduced a TestRule that keeps track of test failures and allows to execute arbitrary tasks when a test fails and when a test is completed (regardless of its result). Allows also to force the execution of the failure task (used in case of afterInternal issues rather than actual test failure). Also updated ElasticsearchRestTests to make sure that the RestClient gets re-initialized in case we restart the global cluster, otherwise all the subsequent tests fail. Improved this mechanism also to relate it directly to the restart of the cluster instead of checking whether the addresses have changed, which doesn't work anyway as the new cluster will use the same addresses but the client needs to be recreated anyway. Closes elastic#9015
8b2d6a9 to
2957066
Compare
…failure CurrentTestFailedMarker is a RunListener that gets notified whenever a test fails, and we were using it to be able to restart the global/suite cluster after each failure. We were checking whether a test had failed in the @after method though, which runs before the listener gets notified, so the failed flag would always be false. This commit makes sure that the global/suite cluster gets restarted not only when there are problems in the afterInternal method, but also after each test failure. In order to achieve this, we need to reset the cluster afterwards, when we get to know about both of the events (problem in afterInternal and test failure), and before resetting the currentCluster. Introduced a TestRule that keeps track of test failures and allows to execute arbitrary tasks when a test fails and when a test is completed (regardless of its result). Allows also to force the execution of the failure task (used in case of afterInternal issues rather than actual test failure). Also updated ElasticsearchRestTests to make sure that the RestClient gets re-initialized in case we restart the global/suite cluster, otherwise all the subsequent tests fail. Improved this mechanism also to relate it directly to the restart of the cluster instead of checking whether the addresses have changed, which doesn't work anyway as the new cluster will use the same addresses but the client needs to be recreated anyway. Closes elastic#9015
CurrentTestFailedMarker is a RunListener that gets notified whenever a test fails, and we were using it to be able to restart the suite cluster after each failure. We were checking whether a test had failed in the @after method though, which runs before the listener gets notified, so the failed flag would always be false. This commit makes sure that the suite cluster gets restarted not only when there are problems in the afterInternal method, but also after each test failure. In order to achieve this, we need to reset the cluster afterwards, when we get to know about both of the events (problem in afterInternal and test failure), and before resetting the currentCluster. Introduced a TestRule that keeps track of test failures and allows to execute arbitrary tasks when a test fails and when a test is completed (regardless of its result). Allows also to force the execution of the failure task (used in case of afterInternal issues rather than actual test failure). Also updated ElasticsearchRestTests to make sure that the RestClient gets re-initialized in case we restart the suite cluster, otherwise all the subsequent tests fail. Improved this mechanism also to relate it directly to the restart of the cluster instead of checking whether the addresses have changed, which doesn't work anyway as the new cluster will use the same addresses but the client needs to be recreated anyway. Closes elastic#9015
…failure CurrentTestFailedMarker is a RunListener that gets notified whenever a test fails, and we were using it to be able to restart the global/suite cluster after each failure. We were checking whether a test had failed in the @after method though, which runs before the listener gets notified, so the failed flag would always be false. This commit makes sure that the global/suite cluster gets restarted not only when there are problems in the afterInternal method, but also after each test failure. In order to achieve this, we need to reset the cluster afterwards, when we get to know about both of the events (problem in afterInternal and test failure), and before resetting the currentCluster. Introduced a TestRule that keeps track of test failures and allows to execute arbitrary tasks when a test fails and when a test is completed (regardless of its result). Allows also to force the execution of the failure task (used in case of afterInternal issues rather than actual test failure). Also updated ElasticsearchRestTests to make sure that the RestClient gets re-initialized in case we restart the global/suite cluster, otherwise all the subsequent tests fail. Improved this mechanism also to relate it directly to the restart of the cluster instead of checking whether the addresses have changed, which doesn't work anyway as the new cluster will use the same addresses but the client needs to be recreated anyway. Closes elastic#9015
CurrentTestFailedMarkeris aRunListenerthat gets notified whenever a test fails, and we were using it to be able to restart the global cluster after each failure. We were checking whether a test had failed in the@Aftermethod though, which runs before the listener gets notified, so the failed flag would always be false.This PR makes sure that the suite/global cluster gets restarted not only when there are problems in the
afterInternalmethod, but also after each test failure. In order to achieve this, we need to reset the cluster afterwards, when we get to know about both of the events (problem in afterInternal and test failure), and before resetting thecurrentCluster. Introduced aTestRulethat keeps track of test failures and allows to execute arbitrary tasks when a test fails and when a test is completed (regardless of its result). Allows also to force the execution of the failure task (used in case ofafterInternalissues rather than actual test failure).Also updated
ElasticsearchRestTeststo make sure that theRestClientgets re-initialized in case we restart the cluster, otherwise all the subsequent tests fail. Improved this mechanism also to relate it directly to the restart of the cluster instead of checking whether the addresses have changed, which doesn't work anyway as the new cluster will use the same addresses but the client needs to be recreated anyway.