ClusterClientIT refactor #38872
ClusterClientIT refactor #38872
Conversation
issuing a cluster health request. Removing these assertions as they are not a good fit for an integration test: the we should assume as little as possible about the global state of the cluster here. Looking at the code, it seems a bit arbitrary to test for this particular side effect (that issuing a cluster health request would unintentionally create an index).
|
Pinging @elastic/es-core-features |
| createIndex(firstIndex, Settings.EMPTY); | ||
| createIndex(secondIndex, Settings.EMPTY); | ||
| createIndex(ignoredIndex, Settings.EMPTY); | ||
| ClusterHealthRequest request = new ClusterHealthRequest(firstIndex, secondIndex); |
There was a problem hiding this comment.
I scoped the request to indices created, and because I wasn't able to reproduce the test failures we saw on #35450 locally, I created a third index in order to test the scoping is correct.
| @@ -178,15 +175,19 @@ public void testClusterHealthYellowClusterLevel() throws IOException { | |||
|
|
|||
| logger.info("Shard stats\n{}", EntityUtils.toString( | |||
| client().performRequest(new Request("GET", "/_cat/shards")).getEntity())); | |||
| assertYellowShards(response); | |||
There was a problem hiding this comment.
removed this line because it's duplicated from another test that covers this case, see method below.
| assertThat(response.getDelayedUnassignedShards(), equalTo(0)); | ||
| assertThat(response.getInitializingShards(), equalTo(0)); | ||
| assertThat(response.getUnassignedShards(), equalTo(0)); | ||
| assertThat(response.getActiveShardsPercent(), equalTo(100d)); |
There was a problem hiding this comment.
I am not sure that I fully understand the removal of assertNoIndices. In the comment you mention that you removed it from testClusterHealthGreen, but it looks like it was removed from testClusterHealthNotFoundIndex where it checks that we didn't get anything back when we asked for a non-existing index. I think it might be better to return this check back and, maybe, randomly create a bogus index to make sure that existing indices in the test don't interfere with it (basically do something similar to what you did in testClusterHealthYellowIndicesLevel). And just in case we can just remove this line with ActiveShardsPercent from here?
There was a problem hiding this comment.
I've pushed a fix for this
There was a problem hiding this comment.
Total nitpick, but if you move this method below testClusterHealthNotFoundIndex() it will make diffs better.
| String ignoredIndex = "tasks"; | ||
| createIndex(firstIndex, Settings.EMPTY); | ||
| createIndex(secondIndex, Settings.EMPTY); | ||
| createIndex(ignoredIndex, Settings.EMPTY); |
There was a problem hiding this comment.
I think if we can surround this with if (randomBoolean()) { ... } this way we can test both situations when our cluster have only these two indices and when something else is present.
| assertThat(response.getDelayedUnassignedShards(), equalTo(0)); | ||
| assertThat(response.getInitializingShards(), equalTo(0)); | ||
| assertThat(response.getUnassignedShards(), equalTo(0)); | ||
| assertThat(response.getActiveShardsPercent(), equalTo(100d)); |
There was a problem hiding this comment.
Total nitpick, but if you move this method below testClusterHealthNotFoundIndex() it will make diffs better.
* elastic/master: Ensure global test seed is used for all random testing tasks (elastic#38991) re-mutes SmokeTestWatcherWithSecurityIT (elastic#38995) Rollup jobs should be cleaned up before indices are deleted (elastic#38930) relax ML Info Docs expected response (elastic#38993) Re-enable single node tests (elastic#38852) ClusterClientIT refactor (elastic#38872) Fix typo in Index API doc Edits to text & formatting in Term Suggester doc (elastic#38963) (elastic#38989) Migrate Streamable to Writeable for WatchStatus (elastic#37390)
Add fixes for ClusterClientIT test and unmute tests.
Add fixes for ClusterClientIT test and unmute tests.
danielmitterdorfer
added
the
>test
Fixes #35450. Goal of this PR is to uncover places where we assert on global cluster state, and, where possible, move to assert only on operations in the test. I've added comments in the PR for removals, but will also summarize here:
removed an
assertNoIndicesfromtestClusterHealthGreen. The goal of the test is to assert on a response from a green cluster, asserting that indices were not created on the cluster in the process of issuing the request seems a bit arbitrary (i.e. there are lots of side effects we could test for, but don't). and testing for this particular side effect has proved unreliable.add an ignored index to
testClusterHealthYellowIndicesLevelto ensure that extra indices aren't throwing off our assertions. The one problematic assertion in this regard isgetActiveShardsPercent(), which is calculated on global cluster state (i.e. the index parameter passed in the request isn't used in this calculation). I don't think this assertion is worth the overhead of maintaining a deterministic global cluster state here.