[Rollup] Redux integration test job creation #32223

sebelga · 2019-02-28T17:57:21Z

This PR adds a "redux" integration test for the creation of a rollup job. I was in doubt if creating multiple files for each step or one big test file. I finally sticked with the later to avoid duplication of testbed setup. I also realize it was easy to navigate the test file by folding the blocks thanks to well organized described sections.

elasticmachine · 2019-02-28T18:25:13Z

Pinging @elastic/es-ui

elasticmachine · 2019-02-28T18:44:24Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

cjcenizal

👏 👏 👏 Wow, bravo! This is terrific, Seb. I love how this test suite basically makes functional tests unnecessary. Thanks for doing this!! I had a few suggestions that I think we should address before merging (e.g. more verbose test description, and extracting a bug fix to a separate PR so we can backport it). But for all of the other stuff, I'd be OK with addressing them in later PRs.

I had a pretty hard time reading the test file because it was so long, though. I would really like to see it split up into separate files per step. I also had a few questions about various tests which I believe would make more sense within component-specific test files -- but if you disagree please help me understand your approach better. This is something we can address in a later PR.

I also had some ideas about that page object pattern we talked about -- take a look and let me know what you think! Also something that can be addressed later.

cjcenizal · 2019-03-01T00:57:26Z

x-pack/plugins/rollup/public/crud_app/services/humanized_numbers.js

@@ -21,8 +21,8 @@ const dayOrdinalToDayNameMap = {
 const monthOrdinalToMonthNameMap = {
  0: i18n.translate('xpack.rollupJobs.util.month.january', { defaultMessage: 'January' }),
  1: i18n.translate('xpack.rollupJobs.util.month.february', { defaultMessage: 'February' }),
-  2: i18n.translate('xpack.rollupJobs.util.month.march', { defaultMessage: 'April' }),
-  3: i18n.translate('xpack.rollupJobs.util.month.april', { defaultMessage: 'March' }),
+  2: i18n.translate('xpack.rollupJobs.util.month.march', { defaultMessage: 'March' }),


This seems like a bug fix we should extract into a separate PR, and backport to 6.7.

Good point, I will create a separate PR for it

cjcenizal · 2019-03-01T01:08:53Z

x-pack/plugins/rollup/__jest__/integration_tests/job_create.test.js

+  interval: '24h'
+};
+
+const initUserActions = (component, findTestSubject) => {


For large test files like this, it might make sense to try extracting all of the helper logic into a sibling helper file, e.g. job_create.test_helpers.js. If this is possible then it might also enable splitting this file up into multiple files.

cjcenizal · 2019-03-01T01:23:22Z

x-pack/plugins/rollup/__jest__/integration_tests/job_create.test.js

+  }
+};
+
+const initGoToStep = (fillFormFields, clickNextStep) => async (step) => {


I haven't been able to test this, but perhaps this could be made terser with a loop?

const initGoToStep = (fillFormFields, clickNextStep) => async (step) => { const completeStep = [ async () => await fillFormFields('logistics'), async () => await fillFormFields('date-histogram'), ]; let currentStep; while (currentStep < targetStep) { if (completeStep[currentStep]) { completeStep[currentStep](); } clickNextStep(); currentStep++; } };

Will try that 👍

cjcenizal · 2019-03-01T04:04:35Z

x-pack/plugins/rollup/__jest__/integration_tests/job_create.test.js

+          expect(findTestSubject('rollupJobNextButton').props().disabled).toBe(true);
+        });
+
+        it('should not allow spaces', async () => {


Many of these tests seem more appropriate as unit tests specific to the validation functions in steps_config, though maybe I'm just not understanding the testing paradigm we're using here. Can you help me understand why they should be here instead?

So the idea is to be as close as possible to how our app (or app section in this case) will be used by a user. If we test in isolation all the files in step_configs:

We'll create a lot of files 😊

We don't have any guarantee that our component uses them. So we might have a green CI but a broken app.

Here we have the best of both world (functional and unit testing) without the complexity of a full browser + API integration testing. It also allows us to refactor without worying about the implementation details.

And... if a product/QA person reads the spec files, he understands a first sight and in 1 place the validation of our form, withouth the need to go throught dozens of files and hoping things are wired correctly.

cjcenizal · 2019-03-01T04:06:28Z

x-pack/plugins/rollup/__jest__/integration_tests/job_create.test.js

+        });
+      });
+
+      describe('rollup cron', () => {


Similarly, my instinct would be to move these tests into test file that's specific to the cron editor component, i.e. steps/components/cron_editor/cron_editor.test.js. Could you help me understand the rationale for putting them here?

If we were to create the cron editor as a reusable component, it might make sense. But we don't so I prefer to test it in integration.

This article helps in understanding the reasoning: https://kentcdodds.com/blog/write-tests

Integration tests strike a great balance on the trade-offs between confidence and speed/expense. This is why it's advisable to spend most (not all, mind you) of your effort there.

cjcenizal · 2019-03-01T04:18:58Z

x-pack/plugins/rollup/__jest__/integration_tests/job_create.test.js

+              expect(testSubjectExists('rollupJobCreateFrequencyHourlyMinuteSelect')).toBe(true);
+            });
+
+            it('should allow to select any minute from 00 -> 59', () => {


I've seen Jest tests tend to use test over it. It might help people differentiate between a Mocha and Jest test at a glance if we use test in our Jest tests.

I prefer to stick to the RSpec and use describe and it. (see comment here https://stackoverflow.com/a/54907283/5642633). Aside, if I'm not wrong, files ended in .test.js are all Jest, no?

Yeah, Jest test files end in .test.js. But in terms of optimizing for readability, it reduces a bit of friction when it's easier to identify code by just looking at it than by looking at the file name. And if you look at any of the Jest docs, they all use test. Not a blocker though, just pointing it out. 😄

in terms of optimizing for readability

Funny, when I read a test (to.equal vs toEqual) I don't wonder what is the testing framework underneath. I do when I write it obviously. 😄

cjcenizal · 2019-03-01T04:19:12Z

x-pack/plugins/rollup/__jest__/integration_tests/job_create.test.js

+        });
+      });
+
+      describe('when no terms are availalbe', () => {


Typo: available

cjcenizal · 2019-03-01T04:20:39Z

x-pack/plugins/rollup/__jest__/integration_tests/job_create.test.js

+          await goToStepAndOpenFieldChooser();
+        });
+
+        it('should display the numeric & keyword fields available', async () => {


Similar to my other comments, I wonder if we could move this into tests for the specific component, i.e. components/field_list/field_list.test.js?

As I commented above, this would bring less "confidence" to the test.

cjcenizal · 2019-03-01T04:25:52Z

x-pack/plugins/rollup/__jest__/integration_tests/job_create.test.js

+        addHistogramFieldToList();
+      });
+
+      describe('--> invalid', () => {


Could we add a more verbose description? I think we're testing all of the different ways the interval input can enter into an invalid state?

cjcenizal · 2019-03-01T04:38:06Z

x-pack/plugins/rollup/__jest__/integration_tests/job_create.test.js

+      form,
+      goToStep,
+      getEuiStepsHorizontalActive,
+    } = initTestBed());


Seeing this, I feel like it really begs to be refactored into the page object model. Then instead of having findTestSubject calls everywhere, like this:

expect(findTestSubject('rollupJobNextButton').props().disabled).toBe(true);

The page object could expose individual parts of the UI, so the code reads a bit more like a story:

expect(rollupJobWizard.nextButton.props().disabled).toBe(true);

This would also mean testSubjectExists is no longer necessary, since we could do this instead:

expect(rollupJobWizard.nextButton.exists()).toBe(true);

I will have to give it some thought as it will complicate things and I'm not sure it brings that much value. For a few objects always present on the screen, it might be nice. But for elements that only appear in the DOM on a specific step (or when the detail panel opens) then it either does not work, or we have to add special methods to dynamically update the objects (the complication I'm talking about).

And then we'd have 2 lines (1 to update the objects and 1 to test) instead of one. We can talk about it over zoom if u want.

Cool, I'm also willing to take a stab at it myself in a separate PR.

sebelga · 2019-03-05T10:47:56Z

Thanks for the review @cjcenizal ! I will try to break the spec in multiple files to make it easier to follow. 👍

…reate_integration

sebelga · 2019-03-05T16:14:57Z

@cjcenizal Just updated the PR, can u have another look? Cheers!

elasticmachine · 2019-03-05T17:10:12Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-03-05T18:34:50Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

cjcenizal

Thanks for breaking up this file into multiple files -- I find them much easier to scan and digest now. Code LGTM, but don't forget to extract out that bugfix (#32223 (comment)).

cjcenizal · 2019-03-05T22:32:38Z

x-pack/plugins/rollup/__jest__/integration_tests/job_create.test_helpers.js

+  if (step > 5) {
+    clickNextStep();
+  }
+};


Did the loop idea not work out?

Forgot to update that part. Just did it 😊

elasticmachine · 2019-03-07T16:08:29Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

sebelga · 2019-03-07T17:36:44Z

retest

elasticmachine · 2019-03-07T18:28:53Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-03-08T10:54:49Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

bmcconaghy · 2019-03-08T14:48:48Z

Looks like you have an issue:

17:53:37     Cannot find module 'test_utils/enzyme_helpers' from 'testbed.js'
17:53:37 
17:53:37     However, Jest was able to find:
17:53:37     	'./testbed.js'
17:53:37 
17:53:37     You might want to include a file extension in your import, or update your 'moduleFileExtensions', which is currently ['js', 'json', 'ts', 'tsx'].

bmcconaghy

LGTM, I like the organization of this, very easy to follow what is happening and the tests themselves are very terse and clear.

sebelga · 2019-03-08T15:30:19Z

retest

sebelga · 2019-03-08T15:37:47Z

Thanks for the review @bmcconaghy ! I'm actually fighting with the CI and import resolves... I know that eventually I will win 😊

elasticmachine · 2019-03-08T16:01:27Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

sebelga · 2019-03-11T07:41:34Z

retest

elasticmachine · 2019-03-11T08:12:30Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

tylersmalley · 2019-03-12T23:44:14Z

These are failing for me locally with the same errors:

node scripts/jest_integration --ci --no-cache --verbose

Should these tests not be in integration tests?

sebelga · 2019-03-13T09:33:43Z

@tylersmalley Those tests are not the integrations tests from Kibana. I call them integration because there are not unit tests either. 😊 Those tests load a whole app section (a react router "page") and test redux integration on the full page. But no real Http requests calls are made on the Kibana server, they are all mocked.

So locally these tests pass without issue with:
node x-pack/scripts/jest.js --testPathPattern <file-path>

The last failing CI complains about the plugins/rollup/__jest__/integration_tests/job_create_review.test.js test file (error: Could not locate module ui/management (...)).

Locally this test file executes without issue:

 PASS  plugins/rollup/__jest__/integration_tests/job_create_review.test.js (15.073s)
  Create Rollup Job, step 5: Metrics
    layout
      ✓ should have the horizontal step active on "Review" (576ms)
      ✓ should have the title set to "Review" (365ms)
      ✓ should have the "next" and "save" button visible (289ms)
      ✓ should go to the "Metrics" step when clicking the back button (297ms)
    tabs
      ✓ should have a "Summary" & "JSON" tabs to review the Job (298ms)
      ✓ should have a "Summary", "Terms" & "JSON" tab if a term aggregation was added (666ms)
      ✓ should have a "Summary", "Histogram" & "JSON" tab if a histogram field was added (531ms)
      ✓ should have a "Summary", "Metrics" & "JSON" tab if a histogram field was added (513ms)
    save()
      ✓ should call the "create" Api server endpoint (359ms)

Test Suites: 1 passed, 1 total
Tests:       9 passed, 9 total
Snapshots:   0 total
Time:        18.051s
Ran all test suites matching /\/Users\/sebastien\/workspace\/elastic\/kibana\/x-pack\/plugins\/rollup\/__jest__\/integration_tests\/job_create_review.test.js/i.
Waiting for the debugger to disconnect...
[1]    97281 killed     env  /Users/sebastien/.nvm/versions/node/v10.15.2/bin/node --inspect-brk=2074

elasticmachine · 2019-03-13T13:32:52Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…reate_integration

elasticmachine · 2019-03-13T14:28:15Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-03-13T15:59:04Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-03-13T16:51:53Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

sebelga · 2019-03-14T10:41:14Z

x-pack/plugins/rollup/__jest__/client_integration/job_create_logistics.test.js

+import sinon from 'sinon';
+
+import { MINUTE, HOUR, DAY, WEEK, MONTH, YEAR } from '../../public/crud_app/services';
+import { INDEX_PATTERN_ILLEGAL_CHARACTERS_VISIBLE } from '../../../../../src/legacy/ui/public/index_patterns';


@cjcenizal @njd5475 For info: this relative import from .test.js works 😊

sebelga added 11 commits February 27, 2019 16:35

test(create rollup job): Logistics -> Validate index pattern

1b58dbc

test(create rollup job): Logistics -> Validate rollup index name & cron

6790f77

test(create rollup job): Logistics -> Validate page size

8cdd23c

test(create rollup job): Logistics -> Validate navigation to step 2

9f292da

test(create rollup job): validate step 2 date histogram

aaadeb2

Small refactor

e26f23b

test(create rollup job): step 3 terms

f170bcd

test(create rollup job): link to the docs

f7c4522

test(create rollup job): step 3 histogram

eee97d7

test(create rollup job): step 5 metrics

1d3f79b

test(create rollup job): step 6 review

cf25359

sebelga requested review from cjcenizal and bmcconaghy February 28, 2019 17:57

cjcenizal added v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.2.0 Feature:Rollups labels Feb 28, 2019

cjcenizal reviewed Mar 1, 2019

View reviewed changes

sebelga added 3 commits March 5, 2019 12:45

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

3d807f4

…reate_integration

Split job create test steps into files

64ef85d

Fix tipo

2a6eb2e

Fix absolute import

8147bcf

cjcenizal approved these changes Mar 5, 2019

View reviewed changes

Fix absolute webpack import

33d5a38

Fix enzyme_helpers import path

98ae910

bmcconaghy approved these changes Mar 8, 2019

View reviewed changes

sebelga mentioned this pull request Mar 11, 2019

Move testbed utils to x-pack/test_utils #32893

Merged

Rename test folder to "client_integration"

bdbbe91

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

9a409a2

…reate_integration

Fix failing test

0193443

Fix absolute import from webpack alias

6a1908d

sebelga commented Mar 14, 2019

View reviewed changes

sebelga merged commit bc51461 into elastic:master Mar 14, 2019

sebelga deleted the test/rollup_job_create_integration branch March 14, 2019 10:42

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

[Rollup] Client integration test for job creation (elastic#32223)

e2e54c7

sebelga mentioned this pull request Mar 14, 2019

[7.x] [Rollup] Client integration test for job creation (#32223) #33232

Merged

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

[Rollup] Client integration test for job creation (#32223) (#33232)

59436fb

sebelga added the non-issue Indicates to automation that a pull request should not appear in the release notes label Mar 27, 2019

[Rollup] Redux integration test job creation #32223

[Rollup] Redux integration test job creation #32223

Conversation

sebelga commented Feb 28, 2019

elasticmachine commented Feb 28, 2019

elasticmachine commented Feb 28, 2019

💔 Build Failed

cjcenizal left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal Mar 1, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal Mar 5, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebelga Mar 5, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebelga commented Mar 5, 2019

sebelga commented Mar 5, 2019

elasticmachine commented Mar 5, 2019

💔 Build Failed

elasticmachine commented Mar 5, 2019

💔 Build Failed

cjcenizal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Mar 7, 2019

💔 Build Failed

sebelga commented Mar 7, 2019

elasticmachine commented Mar 7, 2019

💔 Build Failed

elasticmachine commented Mar 8, 2019

💔 Build Failed

bmcconaghy commented Mar 8, 2019

bmcconaghy left a comment

Choose a reason for hiding this comment

sebelga commented Mar 8, 2019

sebelga commented Mar 8, 2019

elasticmachine commented Mar 8, 2019

💔 Build Failed

sebelga commented Mar 11, 2019

elasticmachine commented Mar 11, 2019

💔 Build Failed

tylersmalley commented Mar 12, 2019

sebelga commented Mar 13, 2019 • edited

elasticmachine commented Mar 13, 2019

💔 Build Failed

elasticmachine commented Mar 13, 2019

💔 Build Failed

elasticmachine commented Mar 13, 2019

💔 Build Failed

elasticmachine commented Mar 13, 2019

💚 Build Succeeded

Choose a reason for hiding this comment

cjcenizal left a comment •

edited

cjcenizal Mar 1, 2019 •

edited

cjcenizal Mar 5, 2019 •

edited

sebelga Mar 5, 2019 •

edited

sebelga commented Mar 13, 2019 •

edited