ui/resize_checker 👉 src/plugins/kibana_utils #44750

sainthkh · 2019-09-04T07:26:07Z

Summary

Note: this PR removes passing down ResizeChecker as a dependency to console, and replaces it with importing from plugins/kibana_utils.

Why custom mock?

1.
In resize_checker.test.ts, custom mocks are added. I used MockElement instead of jsdom element created by document.createElement('div'). I mocked resize-observer-polyfill.

All of this happened because Element instanceof Object is false.

Let me explain.

In resize-observer-polyfill's observer(), it checks several things before adding target to observation.

One of them is Element instanceof Object. It should be true to pass the test. But it fails if we run the code in jest environment. (I cannot be sure why, but it seems that it is false to determine we're running the code in a real browser or console environment.)

The thing we need to test is whether the callback function works correctly, not resize-observer-polyfill does.

Because of that, I decided to bypass resize-observer-polyfill. That's why it is mocked.

2.
Besides, clientHeight and clientWidth is readonly. We can reset them with Object.defineProperty(). But this doesn't send any message to the created element.

It just complicates the problem. So, I created MockElement. And added some simple implementation for them.

In real browser code, resizing elements should be like div.style.height = '100px'. But it doesn't send any event in jest environment. So, I added MockElement and changed the clientHeight directly.

3.
I named event resize because it is used in the code.

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

~~- [ ] This was checked for cross-browser compatibility, including a check against IE11~~
~~- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
~~- [ ] Documentation was added for features that require explanation or tutorials~~

Unit or functional tests were updated or added to match the most common scenarios
~~- [ ] This was checked for keyboard-only and screenreader accessibility~~

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

elasticmachine · 2019-09-04T07:26:09Z

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?

src/legacy/ui/public/resize_checker/resize_checker.test.ts

mattkime · 2019-09-06T02:25:58Z

retest

elasticmachine · 2019-09-06T03:41:23Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

...gins/canvas/public/components/workpad_page/workpad_interactive_page/interaction_boundary.tsx

peteharverson

ML edits LGTM

elasticmachine · 2019-09-11T08:59:28Z

Pinging @elastic/kibana-app-arch

ppisljar · 2019-09-11T08:59:40Z

retest

elasticmachine · 2019-09-11T10:19:16Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

lizozom

Hey @sainthkh

Thank you for this PR!

I added a couple of small comments.

However, although I understand you explanation on why you had to mock Element, I'm not sure why this PR would have caused any problems with these tests, that are working on master.

Could you please see if you can get the test working the way they were?

src/legacy/core_plugins/console/public/src/sense_editor_resize.js

...gins/canvas/public/components/workpad_page/workpad_interactive_page/interaction_boundary.tsx

sainthkh · 2019-09-12T02:32:03Z

In #44366, it says to jest-ify tests. So, I did. (Also, I didn't know that I had to run tests with yarn run test:dev.) I reverted tests to be run with karma. And removed jest-ified tests.

I rerun the integration test in my local Ubuntu setup and found that the test passed without fail.

ppisljar · 2019-09-12T10:38:44Z

what was the reason for reverting tests to karma ?

sainthkh · 2019-09-12T12:16:41Z

I might have misunderstood the comment. But it was the request of @lizozom. As you know, test:dev doesn't work with jest. So, I had to revert it to mocha and karma.

ppisljar · 2019-09-12T12:32:01Z

you can run jest tests with node scripts/jest

sainthkh · 2019-09-12T13:05:20Z

ui/public tests are run by yarn run test:dev. And original resize checker test was written for that.

And @lizozom's request was to make tests work in that setup.

ppisljar · 2019-09-23T09:42:25Z

@elasticmachine merge upstream

lizozom · 2019-11-14T15:49:44Z

@elasticmachine merge upstream

lizozom · 2019-11-14T15:50:01Z

jenkins tests this

...ole/np_ready/public/application/containers/editor/legacy/subscribe_console_resize_checker.ts

lizozom · 2019-11-14T15:56:44Z

Hey @sainthkh the tests and functional test look great.
However, I don't fully understand why you added ResizeChecker to the ContextValue and are passing it around. ResizeChecker class can be imported when it's used, as it's only a class definition.

lizozom · 2019-11-14T17:22:17Z

Jenkins, test this

elasticmachine · 2019-11-14T18:32:11Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 592aa498f9e47845f158cb030ee3774fc6753197

sainthkh · 2019-11-15T01:26:19Z

1. Typecheck failed because of #49349 merged to master yesterday. It removed docLinkVersion from__LEGACY.

2. About ResizeChecker and context: When ResizeChecker was in ui/public, console developers couldn't import them directly because it was legacy, because of that, they used legacy.ts and context to import it. Because of that the import process was that complicated.

As ResizeChecker is now in kibana_utils, this procedure was unnecessary. So, all of the legacy import procedure can be removed and we can import them directly.

(In the code fix above, I just focused on removing anys and didn't think about why they became any. Thanks for the point.)

3. It seems that something went wrong with merging master. Infra failed without any error message.

So, I decided to start from scratch. And the result is below.

lizozom · 2019-11-15T08:28:53Z

jenkins test this

lizozom · 2019-11-15T08:29:13Z

@sainthkh this looks good. Let me test this.

elasticmachine · 2019-11-15T09:49:55Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: c4808ae

lizozom · 2019-11-17T09:43:23Z

@elasticmachine merge upstream

lizozom · 2019-11-17T09:44:03Z

jenkins test this

lizozom

LGTM.

@sainthkh error is due to an error on master.
Once it's fixed, I'll merge :)

elasticmachine · 2019-11-17T11:04:35Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 671a8e5

lizozom · 2019-11-17T12:09:29Z

@elasticmachine merge upstream

lizozom · 2019-11-17T12:33:37Z

jenkins test this

lizozom · 2019-11-17T12:52:22Z

retest

elasticmachine · 2019-11-17T14:21:58Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: b86646d

* Moved ResizeChecker to kibana_utils. * Removed ResizeChecker from __LEGACY.

…her [skip ci] * upstream/master: (54 commits) allows plugins to define validation schema for "enabled" flag (elastic#50286) Add retry to find.existsByDisplayedByCssSelector (elastic#48734) [i18n] integrate latest translations (elastic#50864) ui/resize_checker 👉 src/plugins/kibana_utils (elastic#44750) Fix @reach/router types (elastic#50863) [ML] Adding ML node warning to overview and analytics pages (elastic#50766) Bump storybook dependencies (elastic#50752) [APM Replace usage of idx with optional chaining (elastic#50849) [SIEM] Fix eslint errors (elastic#49713) Improve "Browser client is out of date" error message (elastic#50296) [SIEM][Detection Engine] REST API improvements and changes from UI/UX feedback (elastic#50797) Move @kbn/es-query into data plugin - es-query folder (elastic#50182) Index Management new platform migration (elastic#49359) Increase retry for cloud snapshot to finish (elastic#50781) Removing EuiCode from inside EuiPanel (elastic#50683) [SIEM] Tests for search_after and bulk index (elastic#50129) Make babel understand TypeScript 3.7 syntax (elastic#50772) Fixing mocha tests and broken password change status codes (elastic#50704) [Canvas] Use compressed forms in sidebar (elastic#49419) Add labels to shell scripts in Jenkins (elastic#49657) ...

sainthkh commented Sep 4, 2019

View reviewed changes

src/legacy/ui/public/resize_checker/resize_checker.test.ts Outdated Show resolved Hide resolved

streamich mentioned this pull request Sep 5, 2019

Visualizations 👉 New Platform #44121

Closed

13 tasks

sainthkh force-pushed the move-resize-checker branch from 31381d2 to 931ed53 Compare September 5, 2019 22:38

sainthkh marked this pull request as ready for review September 6, 2019 01:24

sainthkh requested review from a team as code owners September 6, 2019 01:24

sainthkh requested a review from a team as a code owner September 6, 2019 07:49

sainthkh commented Sep 6, 2019

View reviewed changes

...gins/canvas/public/components/workpad_page/workpad_interactive_page/interaction_boundary.tsx Outdated Show resolved Hide resolved

peteharverson approved these changes Sep 6, 2019

View reviewed changes

ppisljar added Feature:NP Migration release_note:skip Skip the PR/issue when compiling release notes review Team:AppArch v7.5.0 v8.0.0 labels Sep 11, 2019

lizozom suggested changes Sep 11, 2019

View reviewed changes

src/legacy/core_plugins/console/public/src/sense_editor_resize.js Outdated Show resolved Hide resolved

...gins/canvas/public/components/workpad_page/workpad_interactive_page/interaction_boundary.tsx Outdated Show resolved Hide resolved

sainthkh force-pushed the move-resize-checker branch from 6e2743d to ae30316 Compare September 13, 2019 01:33

lizozom reviewed Nov 14, 2019

View reviewed changes

...ole/np_ready/public/application/containers/editor/legacy/subscribe_console_resize_checker.ts Outdated Show resolved Hide resolved

Moved ResizeChecker to kibana_utils.

72b9511

sainthkh force-pushed the move-resize-checker branch from 5145490 to 418b3fa Compare November 15, 2019 02:44

Removed ResizeChecker from __LEGACY.

c4808ae

sainthkh force-pushed the move-resize-checker branch from 418b3fa to c4808ae Compare November 15, 2019 02:51

Merge branch 'master' into move-resize-checker

671a8e5

lizozom approved these changes Nov 17, 2019

View reviewed changes

Merge branch 'master' into move-resize-checker

b86646d

lizozom merged commit d7f2ebc into elastic:master Nov 17, 2019

lizozom pushed a commit to lizozom/kibana that referenced this pull request Nov 17, 2019

ui/resize_checker 👉 src/plugins/kibana_utils (elastic#44750)

2596c9d

* Moved ResizeChecker to kibana_utils. * Removed ResizeChecker from __LEGACY.

lizozom mentioned this pull request Nov 17, 2019

[7.x] ui/resize_checker 👉 src/plugins/kibana_utils (#44750) #50875

Merged

lizozom pushed a commit that referenced this pull request Nov 17, 2019

ui/resize_checker 👉 src/plugins/kibana_utils (#44750) (#50875)

8586f45

* Moved ResizeChecker to kibana_utils. * Removed ResizeChecker from __LEGACY.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ui/resize_checker 👉 src/plugins/kibana_utils #44750

ui/resize_checker 👉 src/plugins/kibana_utils #44750

sainthkh commented Sep 4, 2019 •

edited by lizozom

Loading

elasticmachine commented Sep 4, 2019

mattkime commented Sep 6, 2019

elasticmachine commented Sep 6, 2019

peteharverson left a comment

elasticmachine commented Sep 11, 2019

ppisljar commented Sep 11, 2019

elasticmachine commented Sep 11, 2019

lizozom left a comment

sainthkh commented Sep 12, 2019

ppisljar commented Sep 12, 2019

sainthkh commented Sep 12, 2019 •

edited

Loading

ppisljar commented Sep 12, 2019

sainthkh commented Sep 12, 2019

ppisljar commented Sep 23, 2019

lizozom commented Nov 14, 2019

lizozom commented Nov 14, 2019

lizozom commented Nov 14, 2019

lizozom commented Nov 14, 2019

elasticmachine commented Nov 14, 2019

sainthkh commented Nov 15, 2019 •

edited

Loading

lizozom commented Nov 15, 2019

lizozom commented Nov 15, 2019

elasticmachine commented Nov 15, 2019

lizozom commented Nov 17, 2019

lizozom commented Nov 17, 2019

lizozom left a comment •

edited

Loading

elasticmachine commented Nov 17, 2019

lizozom commented Nov 17, 2019

lizozom commented Nov 17, 2019

lizozom commented Nov 17, 2019

elasticmachine commented Nov 17, 2019

ui/resize_checker 👉 src/plugins/kibana_utils #44750

ui/resize_checker 👉 src/plugins/kibana_utils #44750

Conversation

sainthkh commented Sep 4, 2019 • edited by lizozom Loading

Summary

Why custom mock?

Checklist

For maintainers

elasticmachine commented Sep 4, 2019

mattkime commented Sep 6, 2019

elasticmachine commented Sep 6, 2019

💔 Build Failed

peteharverson left a comment

Choose a reason for hiding this comment

elasticmachine commented Sep 11, 2019

ppisljar commented Sep 11, 2019

elasticmachine commented Sep 11, 2019

💔 Build Failed

lizozom left a comment

Choose a reason for hiding this comment

sainthkh commented Sep 12, 2019

ppisljar commented Sep 12, 2019

sainthkh commented Sep 12, 2019 • edited Loading

ppisljar commented Sep 12, 2019

sainthkh commented Sep 12, 2019

ppisljar commented Sep 23, 2019

lizozom commented Nov 14, 2019

lizozom commented Nov 14, 2019

lizozom commented Nov 14, 2019

lizozom commented Nov 14, 2019

elasticmachine commented Nov 14, 2019

💔 Build Failed

sainthkh commented Nov 15, 2019 • edited Loading

lizozom commented Nov 15, 2019

lizozom commented Nov 15, 2019

elasticmachine commented Nov 15, 2019

💚 Build Succeeded

lizozom commented Nov 17, 2019

lizozom commented Nov 17, 2019

lizozom left a comment • edited Loading

Choose a reason for hiding this comment

elasticmachine commented Nov 17, 2019

💔 Build Failed

lizozom commented Nov 17, 2019

lizozom commented Nov 17, 2019

lizozom commented Nov 17, 2019

elasticmachine commented Nov 17, 2019

💚 Build Succeeded

sainthkh commented Sep 4, 2019 •

edited by lizozom

Loading

sainthkh commented Sep 12, 2019 •

edited

Loading

sainthkh commented Nov 15, 2019 •

edited

Loading

lizozom left a comment •

edited

Loading