[CCR] Client tests auto-follow pattern & follower index forms by sebelga · Pull Request #34449 · elastic/kibana

sebelga · 2019-04-03T14:54:17Z

This PR adds the client integration tests for the

Add auto-follow pattern
Edit auto-follow pattern
Add follower index
Edit follower index

I also fixed a small bug in the client side validation of the auto-follow pattern input. It is better to have errors as objects with a "message" property. This way, we can add other properties to the error object (like "alwaysVisible") and give our validation UI all the flexibility needed.

elasticmachine · 2019-04-03T15:32:08Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

There was a bug introduced in the client side validation of the auto-follow patterns by mixing the type of error (object vs JSX element). The correct way to handle errors is to use an object with a "message" property. This way, we can add other properties to the error object (like "alwaysVisible") and give our validation UI all the flexibility needed.

elasticmachine · 2019-04-03T16:12:50Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…or'.

cjcenizal

Awesome work! I have some feedback in sebelga#9 and will re-review once that's resolved.

elasticmachine · 2019-04-04T10:09:48Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-04-04T14:13:56Z

Pinging @elastic/es-ui

elasticmachine · 2019-04-04T14:54:53Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

Clarify UX intentions with comments and use null to represent 'no error'.

sebelga · 2019-04-06T04:33:45Z

@cjcenizal I updated the PR with your proposal. Can you have another look at it? I also added the tests for the follower index add & edit. cheers!

elasticmachine · 2019-04-06T05:15:37Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-04-06T12:21:51Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

cjcenizal

Looks great!

cjcenizal · 2019-04-08T20:18:37Z

x-pack/plugins/cross_cluster_replication/public/app/components/remote_clusters_provider.js

        this.setState({
          isLoading: false,
-          remoteClusters: sortClusterByName(remoteClusters)
+          remoteClusters


Would you mind helping me understand why you made this change? The reason I ask is because this change adds a little complexity by creating an additional Promise (via the added then) and passes remoteClusters to sortClusterByName implicitly instead of explicitly, so the original code seems both simpler and more expressive to me.

This is directly related to the video you shared to me: "first-class functions". The same way we don't need to be explicit (and use imperative programming) with map, and forEach. I must say that I'm a bit confused and don't understand why here you think being declarative adds complexity:

loadRemoteClusters() .then(sortClusterByName) .then((remoteClusters) => { ... })

but here no

await Promise.all(names.map(deleteCluster));

Sure, I can explain. I'm measuring complexity by how many "moving parts" are involved in an expression. In the example of piping through .then we've created an additional promise, which adds another moving part. By removing the .then we've removed that moving part which would be a reduction in complexity according to how I'm measuring it.

In this regard, I think your example of using Promise.all is as simple as you can get. It has the minimum number of moving parts required.

In terms of expressiveness, loadRemoteClusters().then(sortClusterByName) doesn't contain all of the information that names.map(deleteCluster) has. The latter example communicate that deleteCluster will accept name as its argument because that's what map does, but the first example implies that remoteClusters will be the argument passed to sortClusterByName because we assume that's what loadRemoteClusters resolves to.

Anyway, the bottom line is that I don't see the imperative programming style or functional programming style as inherently good or bad, I think they're both useful tools in various situations and I just wanted to share my perspective in this example -- just another way of thinking about code. :)

Thanks for explaining your thoughts, I get it. Not saying I share your opinion on complexity but I understand better your idea. I don't have any problem of reverting the change if you prefer it the other way.
This is, in the end, the result of doing so much testing. From time to time you bump on some code and think: "oh, this would be more elegant written this way" (purely at the preference level 😊 )

cjcenizal · 2019-04-08T20:30:54Z

...lugins/cross_cluster_replication/__jest__/client_integration/auto_follow_pattern_add.test.js

+  breadcrumbs: { set: () => {} },
+}));
+
+jest.mock('ui/index_patterns', () => {


What do you think about adding some newlines so we don't need to disable the max-len rule? This will also make the code easier to read when the code editor is narrow. This comment applies to the other test files, too.

jest.mock('ui/index_patterns', () => { const { INDEX_PATTERN_ILLEGAL_CHARACTERS_VISIBLE } = jest.requireActual('../../../../../src/legacy/ui/public/index_patterns/constants'); const { validateIndexPattern, ILLEGAL_CHARACTERS, CONTAINS_SPACES } = jest.requireActual('../../../../../src/legacy/ui/public/index_patterns/validate/validate_index_pattern'); return { INDEX_PATTERN_ILLEGAL_CHARACTERS_VISIBLE, ILLEGAL_CHARACTERS, CONTAINS_SPACES, validateIndexPattern, }; });

cjcenizal · 2019-04-08T20:45:43Z

...lugins/cross_cluster_replication/__jest__/client_integration/auto_follow_pattern_add.test.js

+    setLoadRemoteClusteresResponse();
+
+    // Mock all HTTP Requests that have not been handled previously
+    server.respondWith([200, {}, '']);


Same with this mocked response. I think we can remove it?

Nooooooooooo 😄 This is our safeguard against any not handled HTTP requests (that we don't care in our test but could break the CI at any point).

cjcenizal · 2019-04-08T20:46:37Z

...lugins/cross_cluster_replication/__jest__/client_integration/auto_follow_pattern_add.test.js

+    } = registerHttpRequestMockHelpers(server));
+
+    // Set "default" mock responses by not providing any arguments
+    setLoadRemoteClusteresResponse();


Is this needed? I removed it and the tests seemed to run fine without it.

Also I just noticed that this method name contains a typo -- "Clusters" is spelled "Clusteres", so it should be setLoadRemoteClustersResponse.

😄 I must say that you make me smile here! So when I am explicit, you ask me not to be, when I am not, you ask me to be.
So this line is indeed not needed as we have a safeguard for HTTP requests not handled by any mock. To be even more explicit it should have been setLoadRemoteClusteresResponse([]); (with empty Array), but the default response is an empty array.
So even though it is not necessary, I decided to be explicit and make it clear that there is a request to load the remote clusters.

I am not sure though to what extent we should add comments to the test code to explain that the form depends on the remote clusters being loaded before displaying the component.

cjcenizal · 2019-04-08T20:53:40Z

...lugins/cross_cluster_replication/__jest__/client_integration/auto_follow_pattern_add.test.js

+        expect(getFormErrorsMessages()).toContain(`Name can't begin with an underscore.`);
+      });
+
+      test('should not allow a "," (coma)', () => {


Small typo: "coma" -> "comma"

cjcenizal · 2019-04-08T21:20:08Z

x-pack/plugins/cross_cluster_replication/__jest__/client_integration/follower_index_add.test.js

+    setLoadRemoteClusteresResponse();
+
+    // Mock all HTTP Requests that have not been handled previously
+    server.respondWith([200, {}, '']);


Seems like both lines 56 and 59 are unnecessary, since the tests seem to execute normally when they're removed. This comment applies to follower_index_add.test.js, too.

cjcenizal · 2019-04-08T21:54:53Z

x-pack/plugins/cross_cluster_replication/__jest__/client_integration/follower_index_add.test.js

+      // The implementation of the remote cluster "Select" + validation is
+      // done inside the <RemoteClustersFormField /> component. The same component that we use in the <AutoFollowPatternAdd /> section.
+      // To avoid copy/pasting the same tests here, we simply make sure that both sections use the <RemoteClustersFormField />
+      test('should use the same <RemoteClustersFormField /> component that in the <AutoFollowPatternAdd /> section', async () => {


I think "that in the" would read a bit more naturally if it was changed to "as the".

cjcenizal · 2019-04-08T21:58:52Z

x-pack/plugins/cross_cluster_replication/__jest__/client_integration/follower_index_add.test.js

+          await nextTick(550); // we need to wait as there is a debounce of 500ms on the http validation
+
+          expect(server.requests.length).toBe(totalRequests + 1);
+          expect(server.requests[totalRequests].url).toBe('/api/index_management/indices');


Minor nit: I think this would be clearer if changed to:

expect(server.requests[server.requests.length - 1].url).toBe('/api/index_management/indices');

Using server.requests.length - 1 more clearly communicates to me that we're looking at the most recent request, whereas referencing totalRequests is a bit more of a circuitous way of saying the same thing.

Good point 👍

cjcenizal · 2019-04-08T22:05:40Z

...ck/plugins/cross_cluster_replication/__jest__/client_integration/follower_index_edit.test.js

+     * the "create" follower index, we won't test it again but simply make sure that
+     * the form component is indeed shared between the 2 app sections.
+     */
+    test('should use the same Form component that the "<FollowerIndexAdd />" component', async () => {


"that the" -> "as the"?

CR + English class = ❤️ 😊

cjcenizal · 2019-04-08T22:11:04Z

...ck/plugins/cross_cluster_replication/__jest__/client_integration/follower_index_edit.test.js

+      expect(formAdd.length).toBe(1);
+    });
+
+    test('should populate the form fields with the values from the follower index loaded', () => {


What do you think of using a data structure to dedupe some of these assertions and make it easier to compare inputs to expected values?

test('should populate the form fields with the values from the follower index loaded', () => { const { remoteCluster, leaderIndex, name, maxReadRequestOperationCount, maxOutstandingReadRequests, maxReadRequestSize, maxWriteRequestOperationCount, maxWriteRequestSize, maxOutstandingWriteRequests, maxWriteBufferCount, maxWriteBufferSize, maxRetryDelay, readPollTimeout, } = FOLLOWER_INDEX; const inputToValueMap = { ccrRemoteClusterInput: remoteCluster, ccrFollowerIndexFormLeaderIndexInput: leaderIndex, ccrFollowerIndexFormFollowerIndexInput: name, ccrFollowerIndexFormMaxReadRequestOperationCountInput: maxReadRequestOperationCount, ccrFollowerIndexFormMaxOutstandingReadRequestsInput: maxOutstandingReadRequests, ccrFollowerIndexFormMaxReadRequestSizeInput: maxReadRequestSize, ccrFollowerIndexFormMaxWriteRequestOperationCountInput: maxWriteRequestOperationCount, ccrFollowerIndexFormMaxWriteRequestSizeInput: maxWriteRequestSize, ccrFollowerIndexFormMaxOutstandingWriteRequestsInput: maxOutstandingWriteRequests, ccrFollowerIndexFormMaxWriteBufferCountInput: maxWriteBufferCount, ccrFollowerIndexFormMaxWriteBufferSizeInput: maxWriteBufferSize, ccrFollowerIndexFormMaxRetryDelayInput: maxRetryDelay, ccrFollowerIndexFormReadPollTimeoutInput: readPollTimeout, }; Object.keys(inputToValueMap).forEach(input => { expect(find(input).props().value).toBe(inputToValueMap[input]); }); });

elasticmachine · 2019-04-09T07:52:10Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-04-09T08:50:24Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

sebelga · 2019-04-09T09:42:45Z

Thanks for the review @cjcenizal ! I added your suggested changes. We can talk offline about the comments I made if you want.

…c#34449)

#34789)

sebelga requested review from cjcenizal and jen-huang April 3, 2019 14:54

sebelga added Feature:CCR and Remote Clusters non-issue Indicates to automation that a pull request should not appear in the release notes test-jest-integration v7.2.0 v8.0.0 labels Apr 3, 2019

sebelga added 5 commits April 3, 2019 17:34

[CCR] Client integration tests for auto-follow pattern form

f0e0091

Test auto-follow pattern validation

c173642

Test indices generated preview

ff97e76

Add Auto-follow pattern Edit tests

c671d99

sebelga force-pushed the test/ccr_forms branch from dc80949 to c671d99 Compare April 3, 2019 15:34

Clarify UX intentions with comments and use null to represent 'no err…

8d180d1

…or'.

cjcenizal mentioned this pull request Apr 3, 2019

Clarify UX intentions with comments and use null to represent 'no error'. sebelga/kibana#9

Merged

cjcenizal reviewed Apr 3, 2019

View reviewed changes

sebelga and others added 2 commits April 4, 2019 10:50

Small refactor

255e729

Update jest snapshot

5d4a1bd

Merge remote-tracking branch 'upstream/master' into test/ccr_forms

7b7adb7

sebelga added the Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more t// label Apr 4, 2019

sebelga and others added 3 commits April 6, 2019 06:26

Add CR changes

d7453da

Merge pull request #9 from cjcenizal/ccr_forms/ux

3e8e381

Clarify UX intentions with comments and use null to represent 'no error'.

Merge remote-tracking branch 'upstream/master' into test/ccr_forms

433a507

sebelga added 5 commits April 6, 2019 07:38

Test follower index component mount + remote cluster validation

b3be437

Add missing validation for spaces in leader index + index name

008b290

Add test for follower index + advanced settings

aeb5335

Add client tests for Follower Index edit

68dfa07

Update jest snapshot

1edb053

sebelga changed the title ~~[CCR] Client integration test Add/Edit auto-follow pattern~~ [CCR] Client integration test auto-follow pattern & follower index forms Apr 6, 2019

sebelga changed the title ~~[CCR] Client integration test auto-follow pattern & follower index forms~~ [CCR] Client tests auto-follow pattern & follower index forms Apr 6, 2019

cjcenizal approved these changes Apr 8, 2019

View reviewed changes

cjcenizal mentioned this pull request Apr 9, 2019

[DISCUSS] Create FollowerIndexFormPageObject and apply it to follower_index_add tests sebelga/kibana#10

Closed

Add CR changes

7680894

Fix tipo

fccb8da

sebelga merged commit 3bc39bd into elastic:master Apr 9, 2019

sebelga deleted the test/ccr_forms branch April 9, 2019 09:49

sebelga mentioned this pull request Apr 9, 2019

[7.x] [CCR] Client tests auto-follow pattern & follower index forms (#34449) #34789

Merged

sebelga added a commit to sebelga/kibana that referenced this pull request Apr 9, 2019

[CCR] Client tests auto-follow pattern & follower index forms (elasti…

d8988fa

…c#34449)

sebelga added a commit that referenced this pull request Apr 9, 2019

[CCR] Client tests auto-follow pattern & follower index forms (#34449) (

7447d79

#34789)

Conversation

sebelga commented Apr 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Apr 3, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Apr 3, 2019

💔 Build Failed

Uh oh!

cjcenizal left a comment

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Apr 4, 2019

💚 Build Succeeded

Uh oh!

elasticmachine commented Apr 4, 2019

Uh oh!

elasticmachine commented Apr 4, 2019

💚 Build Succeeded

Uh oh!

sebelga commented Apr 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Apr 6, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Apr 6, 2019

💚 Build Succeeded

Uh oh!

cjcenizal 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!

sebelga Apr 9, 2019 • 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!

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!

cjcenizal Apr 8, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Apr 9, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Apr 9, 2019

💚 Build Succeeded

sebelga commented Apr 3, 2019 •

edited

Loading

sebelga commented Apr 6, 2019 •

edited

Loading

sebelga Apr 9, 2019 •

edited

Loading

cjcenizal Apr 8, 2019 •

edited

Loading