[form lib] Fix issues + add test coverage by sebelga · Pull Request #64647 · elastic/kibana

sebelga · 2020-04-28T13:42:18Z

This is the first of several PR to get the form lib ready for v1.0.0. See the meta issue (#65654) for the detail plan.

Adding the tests revealed in the discovery of an issue in the lib. Fixing the issue lead to fixing the consuming code (mainly the mappings editor) and fixing the tests. At the same time, I finally realized why we had so many flakiness in the CI for our component integration tests and hopefully got rid of it for the mappings editor (we still might have other falkiness elsewhere but that was out of scope for this PR).

I re-wrote the git history with the following commit

Testbed (`029db06`)

Make a temporary copy of the testbed to be able to write tests for the form lib. This does not need to be reviewed as the copy will be deleted in an upcoming PR. No need to review this commit, I will delete the folder in the next PR.

Form lib tests (`3c9c232`)

Initial tests for the public API of

useForm()
<Form />
<UseField />
<FormDataProvider />

From lib fixes and refactors (`7d20029`)

Don't update the form state if component is unmounted
Refactor <UseField /> to forward any unknown prop to the children component
Refactor logic to set the default value of <UseField />
Initiate formData$ with an empty object. This allows us to be declarative: what is declared in the JSX is what we get out of the form.
Fix new Subject() memory leak. There is now a getter (getFormData$()), this avoids creating a new instance of the Subject on each re-render.
Removed setTimeout call in the Subject class. We want to be as synchronous as possible.
Fix form.getData() that was not passing the returned data through the serializer.

Mappings editor fixes and refactors (`6a60f57`)

Fix <EditField /> that was warpping the whole content with a <FormDataProvider />.
The EditField component was wrapping the whole content with a FormDataProvider, watching for the "type" and "subType" changes (but declaring them inside its children() render). This was a very bad design and it is not supported anymore as the fields now need to be present in the DOM in order to have their data populated and thus being able to subscribe to their changes.
Refactor logic of "Type" / "SubType" (e.g. "Numeric" > "Float")
Refactor imperative (and hacky) logic for the "Type" / "SubType" fields when creating a new field or updating a field. We now have a nice reusable component <SubTypeParameter /> used in both places.
Refactor logic to save form data (dynamic templates and advanced settings) in the global state when the component is unmounted.

Fix tests (`d12ac63`)

Mappings editor: Removed all nextTick, waitFor and waitForFunc calls inside the tests to make them deterministic.
Mappings editor: Use sync Testbed setup() to mount component
Mappings editor: fix flaky <LoadMappingsProvider /> tests
Snapshot & Restore: Skipped flaky tests checking for the "Loading..." text present in the UI. I manually checked and it is present. I guess there might be something happening in the useRequest hook but I didn't investigate.

Other changes (`139ce47`)

Ingest pipelines: Fix TS contract with form lib
Fix i18n issue
Small updates to testBed

Fixes #59030
Fixes #65741

elasticmachine · 2020-04-28T13:44:56Z

Pinging @elastic/es-ui (Team:Elasticsearch UI)

mistic · 2020-05-04T13:49:38Z

@elasticmachine merge upstream

sebelga · 2020-05-11T18:05:29Z

@elasticmachine merge upstream

sebelga · 2020-05-12T14:33:52Z

@XavierM I set you as reviewer. You don't need to review the code, I mainly want to make sure the changes in the form lib did not create any regressions in your forms. Cheers! 👍

jloleysens

Read through the code and most of the changes make sense to me! Great work @sebelga .

Left a few comments, but nothing to block on.

jloleysens · 2020-05-12T14:12:40Z

src/plugins/es_ui_shared/static/forms/hook_form_lib/components/form_data_provider.test.tsx

jloleysens · 2020-05-13T07:46:47Z

src/plugins/es_ui_shared/static/forms/hook_form_lib/hooks/use_form.ts

@@ -21,7 +21,7 @@ import { useState, useRef, useEffect, useMemo } from 'react';
 import { get } from 'lodash';


I did not see changes to the returned form object to prevent update loops for form subscribers. IIRC I did see form.subscribe being wrapped in a useCallback? I assume that change was made somewhere else?

Are you referring to the PR4 in the meta? #65654

Ah yes that is exactly what I had in mind 👍

jloleysens · 2020-05-13T07:48:36Z

src/plugins/es_ui_shared/static/forms/hook_form_lib/lib/subject.ts

-    setTimeout(() => {
-      fn(this.value);
-    });
+    fn(this.value);


I recall talking about this observable previously, what issue was this causing (and what does executing sync fix)?

This was one of the guilty to create latency and flakiness in the tests. We want to be as synchronous as possible and having to wait the next tick had some weird consequences in a

<FormDataProvider pathsToWatch="type"> // We want this logic to render immediately. </FormDataProvider>

Going synchronous made it all work as expected.

jloleysens · 2020-05-13T07:59:18Z

x-pack/plugins/snapshot_restore/__jest__/client_integration/home.test.ts


-    test('should display a loading while fetching the repositories', () => {
+    /**
+     * TODO: investigate why we need to skip this test.


Weird, what changes here triggered these failures?

I can't tell, but we already had to do it for the exact same reason on L547. Needs some investigation. I checked that those skipped tests actually work

sebelga · 2020-05-13T13:39:54Z

Thanks for the review @jloleysens ! Let me know if "I did not see changes to the returned form object to prevent update loops for form subscribers" is indeed the PR4 that I planned on doing or if it something else 👍 Cheers!

sebelga · 2020-05-13T13:40:13Z

@elasticmachine merge upstream

sebelga · 2020-05-13T15:54:57Z

@elasticmachine merge upstream

sebelga · 2020-05-13T18:07:59Z

@elasticmachine merge upstream

kibanamachine · 2020-05-13T20:00:47Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 3d5f05c

History

💚 Build #47716 succeeded cd06a6e
💚 Build #47661 succeeded 396c6cf
💔 Build #47550 failed fb78234
💔 Build #47545 failed 089bd22
💔 Build #47361 failed 5d20fdc

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

XavierM

I tested all the forms in the SIEM app and I did not see any problems due to all this good work. Thanks @sebelga to make our life easier.

I am wondering if it will be possible to get the FormDataProvider to respect the props pathsToWatch. It is still being executed even if the field changing is not the one set in the props pathsToWatch. If you are interested, I can show you but it is still the same behavior as before.

sebelga · 2020-05-14T07:08:36Z

Thanks for the review @XavierM !
The <FormDataProvider /> being executed is because of the form not being memoized and I will address it in PR 4 of this meta: #65654

The current issue is that whenever the form object changes, the <FormDataProvider> executes. Once that PR 4 will be merged it should be fixed. 👍

sebelga added Feature:Unit Testing non-issue Indicates to automation that a pull request should not appear in the release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more t// labels Apr 28, 2020

sebelga force-pushed the test/form-lib branch from a3217a7 to 0eeecb6 Compare April 30, 2020 08:03

cjcenizal mentioned this pull request May 5, 2020

Remove es_ui_shared static directory #65301

Closed

sebelga force-pushed the test/form-lib branch from bb8e69f to 77010c8 Compare May 5, 2020 17:06

sebelga mentioned this pull request May 7, 2020

[Form lib] Get ready for v1.0.0 #65654

Closed

24 tasks

sebelga force-pushed the test/form-lib branch 3 times, most recently from 04f97bb to 886c653 Compare May 11, 2020 14:23

sebelga mentioned this pull request May 12, 2020

[Ingest Node Pipeline] Pipeline Processors Editor #63694

Closed

sebelga added 5 commits May 12, 2020 14:58

[testbed] Make a temporary copy of the testbed in es_ui_shared folder

029db06

From lib fixes and refactors

7d20029

Mappings editor fixes and refactors

6a60f57

Fix tests (mainly flakyness)

d12ac63

Misc

139ce47

sebelga changed the title ~~[form lib] Add test coverage~~ [form lib] Fix issues + add test coverage May 12, 2020

sebelga force-pushed the test/form-lib branch from 8b1c2cd to 0428f3d Compare May 12, 2020 13:55

sebelga requested a review from jloleysens May 12, 2020 14:02

sebelga marked this pull request as ready for review May 12, 2020 14:02

sebelga requested a review from a team as a code owner May 12, 2020 14:02

sebelga requested a review from XavierM May 12, 2020 14:02

sebelga added v7.9.0 v8.0.0 release_note:skip Skip the PR/issue when compiling release notes labels May 12, 2020

Add form lib tests

3c9c232

Re-enable Shape Datatype tests

5d20fdc

sebelga force-pushed the test/form-lib branch from 0428f3d to 5d20fdc Compare May 12, 2020 15:56

sebelga added 2 commits May 13, 2020 05:08

Fix TS issue

089bd22

Fix TS issue

fb78234

jloleysens approved these changes May 13, 2020

View reviewed changes

Merge branch 'master' into test/form-lib

396c6cf

Merge branch 'master' into test/form-lib

cd06a6e

Merge branch 'master' into test/form-lib

3d5f05c

XavierM approved these changes May 14, 2020

View reviewed changes

sebelga merged commit 6a3db2c into elastic:master May 14, 2020

sebelga deleted the test/form-lib branch May 14, 2020 07:34

sebelga mentioned this pull request May 14, 2020

[7.x] [form lib] Fix issues + add test coverage (#64647) #66525

Merged

sebelga added a commit that referenced this pull request May 14, 2020

[7.x] [form lib] Fix issues + add test coverage (#64647) (#66525)

53ce396

sebelga mentioned this pull request May 18, 2020

[test_utils/Testbed] Move to src/test_utils folder (OSS) #66898

Merged

		@@ -21,7 +21,7 @@ import { useState, useRef, useEffect, useMemo } from 'react';
		import { get } from 'lodash';

Conversation

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

Uh oh!

Testbed (029db06)

Form lib tests (3c9c232)

From lib fixes and refactors (7d20029)

Mappings editor fixes and refactors (6a60f57)

Fix tests (d12ac63)

Other changes (139ce47)

Uh oh!

elasticmachine commented Apr 28, 2020

Uh oh!

mistic commented May 4, 2020

Uh oh!

sebelga commented May 11, 2020

Uh oh!

sebelga commented May 12, 2020

Uh oh!

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

sebelga commented May 13, 2020

Uh oh!

sebelga commented May 13, 2020

Uh oh!

sebelga commented May 13, 2020

Uh oh!

sebelga commented May 13, 2020

Uh oh!

kibanamachine commented May 13, 2020

💚 Build Succeeded

History

Uh oh!

XavierM left a comment

Choose a reason for hiding this comment

Uh oh!

sebelga commented May 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

sebelga commented Apr 28, 2020 •

edited

Loading

Testbed (`029db06`)

Form lib tests (`3c9c232`)

From lib fixes and refactors (`7d20029`)

Mappings editor fixes and refactors (`6a60f57`)

Fix tests (`d12ac63`)

Other changes (`139ce47`)