[CCR] Client integration tests (table lists) by sebelga · Pull Request #33525 · elastic/kibana

sebelga · 2019-03-19T18:20:58Z

This PR adds the client integration tests for the CCR app for

Follower indices list + detail panel
Auto-follow patterns list + detail panel
Home page

I went and refactored a bit the HTML structure of both detail panels and created <section> to organize the content. By doing this refactor I realized that the follower indices detail could be better structured by separating what are the "settings" from the main values and thus the text of the callout when the index is paused is more meaningful.

elasticmachine · 2019-03-19T18:25:13Z

Pinging @elastic/es-ui

…_indices_client # Conflicts: # x-pack/plugins/rollup/__jest__/utils/index.js # x-pack/test/api_integration/apis/management/rollup/lib/utils.js # x-pack/test_utils/index.js # x-pack/test_utils/lib/index.js # x-pack/test_utils/testbed/testbed.js

elasticmachine · 2019-03-19T19:07:52Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-03-19T19:47:51Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-03-20T18:25:54Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…_indices_client

elasticmachine · 2019-03-21T10:36:45Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-03-22T15:52:08Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-03-22T17:41:37Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

cjcenizal

I only had time to look through a couple files so far, so I'm going to leave these comments for now and come back to this on Monday. 😄

cjcenizal · 2019-03-22T23:32:39Z

x-pack/plugins/cross_cluster_replication/__jest__/client_integration/test_helpers.js

+  const mockLoadFollowerIndices = (response) => {
+    const defaultResponse = { indices: [] };
+
+    server.respondWith('GET', /api\/cross_cluster_replication\/follower_indices/,


Why are these paths written as regular expressions? Can we format them as regular strings so we don't need to escape the slashes?

I will check if we need the regex and if not I will use a string 😊

cjcenizal · 2019-03-22T23:45:58Z

x-pack/plugins/cross_cluster_replication/__jest__/client_integration/test_helpers.js

+
+  mockLoadFollowerIndices();
+  mockLoadAutoFollowPattern();
+  mockAutoFollowStats();


I'm having a hard time understanding why this helper needs to call these functions immediately. Can we make the consumer of this helper responsible for calling the mocking functions that it needs instead of calling them here? I think that will make the code more explicit and easier to follow.

So the mockServerResponses() is the helper that will mock all server requests needed. It is called once, on before each, to make things much terser in the tests (http request have been handled, we can move on).
What I decided to do, is export helper methods from it to be able to override the mock whenever needed.
So it first sets the required mock for the requests and export helpers for overrides. I can add a comment or maybe give a better name to the function.

cjcenizal · 2019-03-22T23:48:39Z

...ugins/cross_cluster_replication/__jest__/client_integration/auto_follow_pattern_list.test.js

+  beforeEach(() => {
+    server = sinon.fakeServer.create();
+    server.respondImmediately = true;
+    ({ mockLoadAutoFollowPattern, mockDeleteAutoFollowPattern, mockAutoFollowStats } = mockServerResponses(server));


It looks like each of these helpers is only used once in this file. Instead of defining them in a beforeEach, can we define them only within the test? Can we also declare them within those tests, to scope them more closely to where they're used? If you agree that this will help make it clearer where these helpers are being used, then I'd suggest applying this idea elsewhere too, if possible (I haven't had time to check yet).

As from my comments above, this would make things much more complex than needed. To keep simplicity 2 things happen:

Mock all server requests to a default value (once on before each at the top)

only when needed, override a request, for special use cases

What you are suggesting would mean having to setup the environment before each test

But as it seems it is not easy to understand, I will update the API to make things clearer 😊

sebelga · 2019-03-25T08:58:58Z

@cjcenizal thanks for your comments. I updated the helper API to hopefully make the code easier to follow. 😊

elasticmachine · 2019-03-25T09:44:34Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-03-25T10:48:14Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

cjcenizal

Thanks @sebelga! That helps me understand the intention behind these helpers.

What do you think of exporting each helper as a standalone function? For example...

export function setLoadFollowerIndicesResponse(server, response) {
  const defaultResponse = { indices: [] };

  server.respondWith('GET', 'api/cross_cluster_replication/follower_indices',
    mockResponse(defaultResponse, response));
}

export function setLoadAutoFollowPatternsResponse = (server, response) => {
  const defaultResponse = { patterns: [] };

  server.respondWith('GET', 'api/cross_cluster_replication/auto_follow_patterns',
    mockResponse(defaultResponse, response)
  );
}

// etc...

This way the test files can consume them more explicitly, and the code in test_helpers.js is reduced as a nice side effect. Because the test files will consume these helpers explicitly, you'll only need to import the functions you need. For example, follower_indices_list.test.js will only need to import setLoadFollowerIndicesResponse.

cjcenizal · 2019-03-25T17:34:06Z

x-pack/plugins/cross_cluster_replication/__jest__/client_integration/test_helpers.js

+      'loadFollowerIndices': setLoadFollowerIndicesResponse,
+      'loadAutoFollowPatterns': setLoadAutoFollowPatternsResponse,
+      'deleteAutoFollowPattern': setDeleteAutoFollowPatternResponse,
+      'autoFollowStats': setAutoFollowStatsResponse,


Minor nit: these keys don't need to be strings.

const mapRequestToHelper = { loadFollowerIndices: setLoadFollowerIndicesResponse, loadAutoFollowPatterns: setLoadAutoFollowPatternsResponse, deleteAutoFollowPattern: setDeleteAutoFollowPatternResponse, autoFollowStats: setAutoFollowStatsResponse, };

cjcenizal

Great work @sebelga! I had a few comments but nothing major. Thanks for using test! ❤️

cjcenizal · 2019-03-25T19:58:15Z

...ugins/cross_cluster_replication/__jest__/client_integration/auto_follow_pattern_list.test.js

+    const testSuffix = '_suffix';
+
+    const autoFollowPattern1 = getAutoFollowPatternClientMock({
+      name: `a${getRandomString()}`,


Silly question but what's the benefit of using a random string for names?

Short answer: no need to think about names! 😄
Long answer: I fought many times bugs in my tests because I gave the same name in multiple places.. so now I try to never go that road again whenever possible. Writing'foo' or getRandomString() is the same to me. With the later, I sleep better 😊

It's true that problems arise much more with integration tests, but I like to keep the same pattern everywhere.

cjcenizal · 2019-03-25T22:43:03Z

...ugins/cross_cluster_replication/__jest__/client_integration/auto_follow_pattern_list.test.js

+
+      test('should have a "settings" section', () => {
+        clickAutoFollowPatternAt(0);
+        expect(find('ccrAutoFollowPatternDetailPanelSettingsSection').find('h3').text()).toEqual('Settings');


Can we add a data-test-subj to this element instead of hardcoding the h3?

I knew! that you were going to ask for it. 😄 I should have written down the answer at that moment.
I really think that there is nothing wrong in targetting DOM elements, especially titles (h1, h2, h3), without feeling the need to write data-test-subj everywhere. Furthermore, It has its benefits here: a section should only have 1 title, add another h3 and you'll break the test. Or: we want all section titles to be h3, looking at 2 test files we would see that one "requires" an h3 (.find('h3')) and another one requires an h4 (.find('h4')). That might awake curiosity to the reader as why there is a difference.

cjcenizal · 2019-03-25T23:25:08Z

.../plugins/cross_cluster_replication/__jest__/client_integration/follower_indices_list.test.js

+
+      test('should set the correct follower index settings values', () => {
+        const mapSettingsToFollowerIndexProp = {
+          'Status': 'status',


Minor nit: these keys don't need to be strings.

cjcenizal · 2019-03-25T23:33:56Z

x-pack/plugins/cross_cluster_replication/__jest__/client_integration/test_helpers.js

+  return userActions[section]();
+};
+
+export { nextTick, getRandomString, findTestSubject } from '../../../../test_utils';


This seems odd... what's the benefit of re-exporting here? I would expect consumers to import these helpers directly from test_utils. This will make it easier to follow the import path back from the point of consumption to the source if you're not using an IDE.

The benefit is that the tests file don't need to know about the test_utils folder. We can move it around, rename it and simply update the test_helpers file.

cjcenizal · 2019-03-26T00:17:19Z

...on/public/app/sections/home/auto_follow_pattern_list/components/detail_panel/detail_panel.js

-    }
-    const indexManagementUri = getIndexListUri(indexManagementFilter);
-
+  renderAutoFollowPattern({ followIndexPatternPrefix, followIndexPatternSuffix, remoteCluster, leaderIndexPatterns }) {


Is there a benefit to requiring arguments? It seems more straightforward to me to destructure this.props within the function so it's clear where these variables are coming from.

So here it's purely preferences. The huge benefit of functional programming is this one: no need to ask or wonder where the arguments of a function come from (it is not its responsibility to know that). Simply write a function, give it an input, and expect an output.

The benefits?

If it was a complex function, it could be exported and tested very easily

If the component starts to grow unexpectedly, we'd cut and paste those functions in separate files and we already have our new Function components ready and working.

The mental model obviously has to change when reading the code. The question is not anymore: "but where do those arguments come from?", but: "does it return the correct jsx with the arguments that have been provided?".

cjcenizal · 2019-03-26T00:18:40Z

...on/public/app/sections/home/auto_follow_pattern_list/components/detail_panel/detail_panel.js

+    );
+  }

+  renderAutoFollowPatternErrors(autoFollowPattern) {


cjcenizal · 2019-03-26T00:21:17Z

x-pack/plugins/rollup/public/crud_app/sections/job_list/job_list.test.js

    expect(component.find('JobTableUi').length).toBeTruthy();
  });

-  describe('route query params change', () => {


I think this is worth preserving so that we verify that the job ID is stored in the URL instead of in memory. WDYT? Maybe we can add some comments here or improve the test description to make that clearer?

Thanks for reminding me of this. I should have disabled the test and added a comment on top.
As I improved the router testing in the CCR, I had this test failing in Rollup and added a todo in my head (!) to come back to it and add a proper route change test here. I am going to put back this test and disable it on my next PR with a TODO on top.

cjcenizal · 2019-03-26T00:22:38Z

x-pack/test_utils/testbed/testbed.js

+
+const withRoute = (WrappedComponent, componentRoutePath = '/', onRouter = () => {}) => {
+  return class extends Component {
+      static contextTypes = {


Minor nit: this looks a little over-indented.

Eagle eyes too!

sebelga · 2019-03-26T07:20:28Z

Thanks for the review @cjcenizal ! I will merge this PR and give some thought about exporting the helpers you mention. I think it would be (just) another way to achieve the same goal:

updateHttpMockResponse('loadFollowerIndices', { indices: followerIndices });
setLoadFollowerIndicesResponse(server, { indices: followerIndices });

I will give it some thoughts...

EDIT: I agree to be a bit more declarative! I added a commit on my next PR that get close to what you were proposing: sebelga@65159de

* chore(NA): first changes on every package.json order to support new babel 7. chore(NA): build for kbn-pm with babel 7. * chore(NA): patch babel register to load typescrit * chore(NA): first working version with babel 7 replacing typescript compiler. * fix(NA): common preset declaration in order to make it work with babel-loader. * chore(na): organizing babel preset env package json. * chore(NA): mocha tests enabled. * fix(NA): typo on importing * test(NA): majority of x-pack tests ported to use babel-jest * fix(NA): report info button test with babel-jest. * fix(NA): polling service tests. * test(na): fix server plugins plugin tests. * test(NA): batch of test fixs for jest tests under babel-jest hoisting. * chore(NA): add babel plugin to hoist mock prefixed vars on jest tests. * chore(NA): update yarn.lock file. * chore(NA): tests passing. * chore(NA): remove wrong dep * chore(NA): fix tsconfig * chore(NA): skip babel for ts-jest. * chore(NA): selectively apply the plugin to strip off namespace from ts files. * chore(NA): remove not needed changes from ts tests * chore(NA): removed ts-jest dependency. chore(NA): migrate ts tests on x-pack to use babel-jest with the new pattern. * chore(NA): migrate kibana default distribution typescript tests to run with babel-jest and the new test mock pattern. * chore(NA): merge and solve conflicts with master. * chore(NA): fix problems reported by eslint * chore(NA): fix license ovveride for babel-plugin-mock-imports * chore(NA): update jest integration tests for kbn pm * chore(NA): update babel jest integration tests for kbn pm. * test(NA): update jest integration snapshot for kbn pm. * chore(NA): apply changes according to the pull request reviews. * chore(NA): apply changes according to the pull request reviews. * refact(NA): migrate jest tests to the new pattern. * fix(NA): babel 7 polyfill in the tests bundle. * chore(NA): restore needed step in order to compile x-pack with typescript. * chore(NA): change build to compile typescript with babel for the oss code. chore(NA): change transpile typescript task to only transpile types for x-pack. refact(NA): common preset for babel 7 * Revert "chore(NA): change build to compile typescript with babel for the oss code. chore(NA): change transpile typescript task to only transpile types for x-pack. refact(NA): common preset for babel 7" This reverts commit 2707d53. * fix(NA): import paths for tabConfigConst * chore(NA): fix transpiling error on browser tests * chore(NA): simplify kbn babel preset package. * chore(NA): migrate build to use babel transpiler for typescript excluding xpack. * fix(NA): introduced error on test quick task. * fix(NA): fix preset for client side code on build. * fix(NA): build with babel * fix(NA): negated patterns in the end. * fix(NA): kbn_tp_sample_panel_action creation. * fix(NA): babel typescript transform plugin workaround when exporting interface name. * refact(NA): remove not needed type cast to any on jest test. * docs(NA): add developement documentation about jest mocks test pattern. * chore(NA): missing unmerged path. * chore(NA): fix jest tests for template. * [CCR] Client integration tests (table lists) (#33525) * Force user to re-authenticate if token refresh fails with `400` status code. (#33774) * Improve performance of the Logstash Pipeline Viewer (#33793) Resolves #27513. _This PR is a combination of #31293 (the code changes) + #33570 (test updates). These two PRs were individually reviewed and merged into a feature branch. This combo PR here simply sets up the merge from the feature branch to `master`._ Summary of changes, taken from #31293: ## Before this PR The Logstash Pipeline Viewer UI would make a single Kibana API call to fetch all the information necessary to render the Logstash pipeline. This included information necessary to render the detail drawer that opens up when a user clicks on an individual vertex in the pipeline. Naturally, this single API call fetched _a lot_ of data, not just from the Kibana server but also, in turn, from Elasticsearch as well. The "pro" of this approach was that the user would see instantaneous results if they clicked on a vertex in a pipeline and opened the detail drawer for that vertex. The "cons" were the amount of computation Elasticsearch had to perform and the amount of data being transferred over the wire between Elasticsearch and the Kibana server as well as between the Kibana server and the browser. ## With this PR This PR makes the Kibana API call to fetch data necessary for **initially** rendering the pipeline — that is, with the detail drawer closed — much lighter. When the user clicks on a vertex in a pipeline, a second API call is then made to fetch data necessary for the detail drawer. ## Gains, by the numbers Based on a simple, 1-input, 1-filter, and 1-output pipeline. * Before this PR, the Elasticsearch `logstash_stats` API responses (multiple calls were made using the `composite` aggregation over the `date_histogram` aggregation) generated a total of 1228 aggregation buckets (before any `filter_path`s were applied but across all `composite` "pages"). With this PR, the single `logstash_stats` API response (note that this is just for the initial rendering of the pipeline, with the detail drawer closed) generated 12 buckets (also before any `filter_path`s were applied). That's a **99.02% reduction** in number of buckets. * Before this PR, the Elasticsearch `logstash_stats` API responses added up to 70319 bytes. With this PR, the single `logstash_stats` API response for the same pipeline is 746 bytes. That's a **98.93% reduction** in size. * Before this PR, the Elasticsearch `logstash_state` API response was 7718 bytes. With this PR, the API response for the same pipeline is 2328 bytes. That's a **69.83% reduction** in size. * Before this PR the Kibana API response was 51777 bytes. With this PR, the API response for the same pipeline is 2567 bytes (again, note that this is just for the initial rendering of the pipeline, with the detail drawer closed). That's a **95.04% reduction** in size. * [Maps] split settings into layer and source panels (#33788) * [Maps] split settings into layer and source panels * fix SCSS import * [env] exit if starting as root (#21563) * [env] exit if starting as root * fix windows * s/--allow-root * Typescript sample panel action (#33602) * Typescript sample panel action * Update EUI version to match main cabana version * update yarn.lock * add back typings include * use correct relative path * Home page "recent links" should communicate saved object type #21896 (#33694) * adds object type for screen order * adds object type for pointer hovering * Update src/legacy/ui/public/chrome/directives/header_global_nav/components/header.tsx Co-Authored-By: rockfield <philipp.b@ya.ru>

Sébastien added 11 commits March 14, 2019 11:47

[Jest Testbed] Move utils folder in x-pack/test_utils

16cc866

Add missing testbed API change

c4ef638

Fix failing tests

34ea3b1

Setup test_helpers and testBed

d4aeac5

Add test for table content + action menu

2723c71

Rename test folder

ef198b4

Test table row context menu actions

9a29be8

Test table row context menu actions confirmation modals

6262175

Test follower index detail panel

0459b50

Test follower index detail panel (2)

68e876c

Test home page

1fa1ac9

sebelga requested review from bmcconaghy, cjcenizal and jen-huang March 19, 2019 18:21

sebelga added non-issue Indicates to automation that a pull request should not appear in the release notes Feature:CCR and Remote Clusters v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more t// v7.2.0 labels Mar 19, 2019

Remove unused translation

2140607

Merge remote-tracking branch 'upstream/master' into test/ccr_follower…

e46bc18

…_indices_client

Sébastien added 2 commits March 22, 2019 14:16

Fix wrong user action section

e9486f5

Move <MemoryRouter /> inside testBed

7b480e9

Add missing props to wrapped route

c012954

cjcenizal reviewed Mar 22, 2019

View reviewed changes

Update helper API to set/update http response mocks

c5861c7

Rename helper method name

bd06f7d

cjcenizal reviewed Mar 25, 2019

View reviewed changes

cjcenizal approved these changes Mar 26, 2019

View reviewed changes

sebelga merged commit 67acefc into elastic:master Mar 26, 2019

sebelga deleted the test/ccr_follower_indices_client branch March 26, 2019 07:47

sebelga mentioned this pull request Mar 26, 2019

[7.x] [CCR] Client integration tests (table lists) (#33525) #33830

Merged

sebelga added a commit to sebelga/kibana that referenced this pull request Mar 26, 2019

[CCR] Client integration tests (table lists) (elastic#33525)

1a424e3

sebelga added a commit that referenced this pull request Mar 26, 2019

[CCR] Client integration tests (table lists) (#33525) (#33830)

1393620

joelgriffith pushed a commit that referenced this pull request Mar 27, 2019

[CCR] Client integration tests (table lists) (#33525)

354c683

Conversation

sebelga commented Mar 19, 2019

Uh oh!

elasticmachine commented Mar 19, 2019

Uh oh!

elasticmachine commented Mar 19, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Mar 19, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Mar 20, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Mar 21, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Mar 22, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Mar 22, 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!

cjcenizal Mar 22, 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!

sebelga Mar 25, 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!

sebelga commented Mar 25, 2019

Uh oh!

elasticmachine commented Mar 25, 2019

💚 Build Succeeded

Uh oh!

elasticmachine commented Mar 25, 2019

💚 Build Succeeded

Uh oh!

cjcenizal left a comment

Choose a reason for hiding this comment

Uh oh!

cjcenizal Mar 25, 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!

cjcenizal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebelga Mar 26, 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 Mar 22, 2019 •

edited

Loading

sebelga Mar 25, 2019 •

edited

Loading

cjcenizal Mar 25, 2019 •

edited

Loading

sebelga Mar 26, 2019 •

edited

Loading

sebelga commented Mar 26, 2019 •

edited

Loading