Unit testable index creation task on MetaDataCreateIndexService #25961

fred84 · 2017-07-30T07:09:52Z

Converting anonymous subclass of AckedClusterStateUpdateTask in MetaDataCreateIndexService to inner class to make it unit testable. Relates to #25373 and #25380 .

@jasontedor I've tried to make diff as small as possible. Please take a look.

elasticmachine · 2017-07-30T07:09:54Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

jasontedor · 2017-08-04T14:10:04Z

@fred84 I'm only letting you know that I'm going on vacation until 2017-08-14 and I will look at this when I return (unless someone else reviews it before me).

fred84 · 2017-08-04T14:46:23Z

@jasontedor have a good holiday :)

…ex_creation_task

jasontedor · 2017-08-22T23:17:06Z

This is on my list to review tomorrow.

jasontedor · 2017-08-24T03:38:47Z

test this please

…ex_creation_task

fred84 · 2017-08-24T12:57:01Z

@jasontedor Jenkins failures seems unrelated to my changes. I cant reproduce them locally.

jasontedor · 2017-08-24T12:59:27Z

retest this please

jasontedor

This is a great start. I left some feedback on the tests.

jasontedor · 2017-08-25T11:58:31Z

core/src/test/java/org/elasticsearch/cluster/metadata/IndexCreationTaskTests.java

+            fail("validation exception expected");
+        } catch (IllegalArgumentException e) {
+            assertTrue(e.getMessage().contains("invalid wait_for_active_shards"));
+        }


Can you rewrite this block using expectThrows? Also, instead of assertTrue, it's more effective to use the built-in matchers like assertThat(e.getMessage(), hasToString(containsString("invalid wait_for_active_shards"));. The reason for this is because if the assertion fails with assertTrue, the failure message only says something like "expected true but was false" where as with the matchers we get something like "expected "invalid wait_for_active_shards" but was ..." so we already have immediately from the failure message more information about what is happening.

jasontedor · 2017-08-25T11:58:45Z

core/src/test/java/org/elasticsearch/cluster/metadata/IndexCreationTaskTests.java

+        assertFalse(result.metaData().index("test").getAliases().containsKey("alias1"));
+        assertFalse(result.metaData().index("test").getCustoms().containsKey("custom1"));
+        assertNull(result.metaData().index("test").getSettings().get("key1"));
+        assertFalse(getMappingsFromResponse().containsKey("mapping1"));


This is another case where I would use a more effective matcher.

jasontedor · 2017-08-25T11:59:00Z

core/src/test/java/org/elasticsearch/cluster/metadata/IndexCreationTaskTests.java

+            fail("exception not thrown");
+        } catch (RuntimeException e) {
+            verify(indicesService, times(1)).removeIndex(anyObject(), anyObject(), anyObject());
+        }


Can you rewrite this to use expectThrows?

jasontedor · 2017-08-25T11:59:42Z

core/src/test/java/org/elasticsearch/cluster/metadata/IndexCreationTaskTests.java

+        final ClusterState result = executeTask();
+
+        assertEquals("12", result.getMetaData().index("test").getSettings().get(SETTING_NUMBER_OF_SHARDS));
+        assertEquals("3", result.metaData().index("test").getAliases().get("alias1").getSearchRouting());


Can you rewrite these to use assertThat(..., equalTo(...)). I prefer this form because it's clearer which is the expectation and which is the value under test whereas with assertEquals it often gets confused.

jasontedor · 2017-08-25T11:59:59Z

core/src/test/java/org/elasticsearch/cluster/metadata/IndexCreationTaskTests.java

+        final ClusterState result = executeTask();
+
+        assertEquals("12", result.getMetaData().index("test").getSettings().get(SETTING_NUMBER_OF_SHARDS));
+        assertEquals("3", result.metaData().index("test").getAliases().get("alias1").getSearchRouting());


Same comment here about assertEquals, let's use assertThat(..., equalTo(...)) instead.

jasontedor · 2017-08-25T12:00:18Z

core/src/test/java/org/elasticsearch/cluster/metadata/IndexCreationTaskTests.java

+    public void testDefaultSettings() throws Exception {
+        final ClusterState result = executeTask();
+
+        assertEquals("5", result.getMetaData().index("test").getSettings().get(SETTING_NUMBER_OF_SHARDS));


Let's use assertThat(..., equalTo(...)).

jasontedor · 2017-08-25T12:00:21Z

core/src/test/java/org/elasticsearch/cluster/metadata/IndexCreationTaskTests.java

+        assertEquals(mergedCustom, result.metaData().index("test").getCustoms().get("custom1"));
+        assertEquals("fromReq", result.metaData().index("test").getAliases().get("alias1").getSearchRouting());
+        assertEquals("reqValue", result.metaData().index("test").getSettings().get("key1"));
+        assertEquals("{type={properties={field={type=keyword}}}}", getMappingsFromResponse().get("mapping1").toString());


Let's use assertThat(..., equalTo(...)).

jasontedor · 2017-08-25T12:00:48Z

core/src/test/java/org/elasticsearch/cluster/metadata/IndexCreationTaskTests.java

+        assertTrue(result.metaData().index("test").getAliases().containsKey("alias1"));
+        assertTrue(result.metaData().index("test").getCustoms().containsKey("custom1"));
+        assertEquals("value1", result.metaData().index("test").getSettings().get("key1"));
+        assertTrue(getMappingsFromResponse().containsKey("mapping1"));


Let's replace the assertTrue by more effective matchers, and replace the assertEquals by assertThat(..., equalTo(...)).

jasontedor · 2017-08-25T12:00:59Z

core/src/test/java/org/elasticsearch/cluster/metadata/IndexCreationTaskTests.java

+        assertTrue(result.metaData().index("test").getAliases().containsKey("alias1"));
+        assertTrue(result.metaData().index("test").getCustoms().containsKey("custom1"));
+        assertEquals("value1", result.metaData().index("test").getSettings().get("key1"));
+        assertTrue(getMappingsFromResponse().containsKey("mapping1"));


Let's replace the assertTrue by more effective matchers, and replace the assertEquals by assertThat(..., equalTo(...)).

jasontedor · 2017-08-25T12:01:15Z

core/src/test/java/org/elasticsearch/cluster/metadata/IndexCreationTaskTests.java

+
+        assertTrue(result.metaData().index("test").getAliases().containsKey("alias_from_template_1"));
+        assertTrue(result.metaData().index("test").getAliases().containsKey("alias_from_template_2"));
+        assertFalse(result.metaData().index("test").getAliases().containsKey("alias_from_template_3"));


Let's replace the assertTrue and assertFalse by more effective matchers.

…ex_creation_task

fred84 · 2017-08-26T05:45:27Z

@jasontedor PR updated. Thanks for suggestions on matchers.
I created 2 custom matchers for ImmutableOpenMap and not sure it should be placed inside this test class.

jasontedor · 2017-08-27T19:28:56Z

retest this please

jasontedor

I left one more suggestion. The production code looks good. I started another CI run but I suspect it will be fine.

jasontedor · 2017-08-27T19:27:11Z

core/src/test/java/org/elasticsearch/cluster/metadata/IndexCreationTaskTests.java

        when(indicesService.createIndex(anyObject(), anyObject())).thenReturn(service);
    }
+
+    private <K> Matcher<ImmutableOpenMap<K, ?>> containsKey(final K key) {


You can put these matchers in test-framework under org.elasticsearch.test.hamcrest.

fred84 · 2017-08-28T07:57:01Z

@jasontedor PR updated.

jasontedor · 2017-08-28T17:07:40Z

retest this please

jasontedor

LGTM.

jasontedor · 2017-08-29T00:58:54Z

I will merge this tomorrow. Thanks for the great work here @fred84.

fred84 · 2017-08-29T07:00:10Z

@jasontedor Thanks for review and suggestions.

This commit refactors MetaDataCreateIndexService so that it is unit testable. Relates #25961

fred84 added 5 commits July 30, 2017 08:34

unit test for MataDataCreateIndexService.IndexCreationTask

5d1f3d8

unit test for MataDataCreateIndexService.IndexCreationTask

0054af0

reorder members of MetaDataCreateIndexService

333f7ed

make diff on MetaDataCreateIndexService smaller

e3ee74d

make diff on MetaDataCreateIndexService smaller

2e615f0

Merge remote-tracking branch 'upstream/master' into unit_testable_ind…

1748e5c

…ex_creation_task

Merge remote-tracking branch 'upstream/master' into unit_testable_ind…

7a0a3a9

…ex_creation_task

jasontedor requested changes Aug 25, 2017

View reviewed changes

fred84 added 3 commits August 25, 2017 20:42

Merge remote-tracking branch 'upstream/master' into unit_testable_ind…

18d9eab

…ex_creation_task

Merge remote-tracking branch 'upstream/master' into unit_testable_ind…

88e5260

…ex_creation_task

more descriptive matchers in index creation task test

3e0d7be

jasontedor reviewed Aug 27, 2017

View reviewed changes

move custom matchers to test/framework/test/hamcrest/*

f181dcb

jasontedor approved these changes Aug 29, 2017

View reviewed changes

jasontedor added >enhancement v6.0.0 v6.1.0 labels Aug 29, 2017

jasontedor added v7.0.0 :Core/Infra/Core Core issues without another label labels Aug 29, 2017

jasontedor merged commit c075323 into elastic:master Aug 29, 2017

jasontedor pushed a commit that referenced this pull request Aug 29, 2017

Refactor create index service to be unit testable

83e2d1f

This commit refactors MetaDataCreateIndexService so that it is unit testable. Relates #25961

colings86 added v6.0.0-rc1 and removed v6.0.0 labels Sep 22, 2017

lcawl removed the v6.1.0 label Dec 12, 2017

ywelsch mentioned this pull request Jun 19, 2018

Preserve response headers on cluster update task #31421

Merged

jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Unit testable index creation task on MetaDataCreateIndexService #25961

Unit testable index creation task on MetaDataCreateIndexService #25961

Uh oh!

Conversation

fred84 commented Jul 30, 2017

Uh oh!

elasticmachine commented Jul 30, 2017

Uh oh!

jasontedor commented Aug 4, 2017

Uh oh!

fred84 commented Aug 4, 2017

Uh oh!

jasontedor commented Aug 22, 2017

Uh oh!

jasontedor commented Aug 24, 2017

Uh oh!

fred84 commented Aug 24, 2017

Uh oh!

jasontedor commented Aug 24, 2017

Uh oh!

jasontedor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fred84 commented Aug 26, 2017

Uh oh!

jasontedor commented Aug 27, 2017

Uh oh!

jasontedor left a comment

Choose a reason for hiding this comment

Uh oh!

jasontedor Aug 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fred84 commented Aug 28, 2017

Uh oh!

jasontedor commented Aug 28, 2017

Uh oh!

jasontedor left a comment

Choose a reason for hiding this comment

Uh oh!

jasontedor commented Aug 29, 2017

Uh oh!

fred84 commented Aug 29, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jasontedor Aug 27, 2017 •

edited

Loading