Skip to content

ui/resize_checker 👉 src/plugins/kibana_utils #44750

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Nov 17, 2019
Merged

ui/resize_checker 👉 src/plugins/kibana_utils #44750

merged 4 commits into from
Nov 17, 2019

Conversation

@sainthkh
Copy link
Contributor

@sainthkh sainthkh commented Sep 4, 2019 •
edited by lizozom
Loading

Summary

Fixes #44366.

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

For maintainers

@elasticmachine
Copy link
Contributor

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?

sainthkh
sainthkh commented Sep 4, 2019
View reviewed changes
@streamich streamich mentioned this pull request Sep 5, 2019
Closed
13 tasks
@sainthkh sainthkh force-pushed the move-resize-checker branch from 31381d2 to 931ed53 Compare September 5, 2019 22:38
@sainthkh sainthkh marked this pull request as ready for review September 6, 2019 01:24
@sainthkh sainthkh requested review from a team as code owners September 6, 2019 01:24
@mattkime
Copy link
Contributor

mattkime commented Sep 6, 2019

retest

@elasticmachine
Copy link
Contributor

💔 Build Failed

@sainthkh sainthkh requested a review from a team as a code owner September 6, 2019 07:49
peteharverson
peteharverson approved these changes Sep 6, 2019
View reviewed changes
Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ML edits LGTM

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch

@ppisljar
Copy link
Contributor

retest

@elasticmachine
Copy link
Contributor

💔 Build Failed

lizozom
lizozom suggested changes Sep 11, 2019
View reviewed changes
Copy link
Contributor

@lizozom lizozom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@sainthkh
Copy link
Contributor Author

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
Copy link
Contributor

what was the reason for reverting tests to karma ?

@sainthkh
Copy link
Contributor Author

sainthkh commented Sep 12, 2019 •
edited
Loading

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
Copy link
Contributor

you can run jest tests with node scripts/jest

@sainthkh
Copy link
Contributor Author

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
Copy link
Contributor

@elasticmachine merge upstream

1 similar comment
@ppisljar
Copy link
Contributor

@elasticmachine merge upstream

lizozom
lizozom reviewed Nov 14, 2019
View reviewed changes
src/plugins/kibana_utils/public/resize_checker/resize_checker.ts Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a convention of naming those IResizeChecker

@lizozom
Copy link
Contributor

lizozom commented Nov 14, 2019

@elasticmachine merge upstream

@lizozom
Copy link
Contributor

lizozom commented Nov 14, 2019

jenkins tests this

@lizozom
Copy link
Contributor

lizozom commented Nov 14, 2019

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
Copy link
Contributor

lizozom commented Nov 14, 2019

Jenkins, test this

@elasticmachine
Copy link
Contributor

💔 Build Failed

@sainthkh
Copy link
Contributor Author

sainthkh commented Nov 15, 2019 •
edited
Loading

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.

@sainthkh sainthkh force-pushed the move-resize-checker branch from 5145490 to 418b3fa Compare November 15, 2019 02:44
@sainthkh sainthkh force-pushed the move-resize-checker branch from 418b3fa to c4808ae Compare November 15, 2019 02:51
@lizozom
Copy link
Contributor

lizozom commented Nov 15, 2019

jenkins test this

@lizozom
Copy link
Contributor

lizozom commented Nov 15, 2019

@sainthkh this looks good. Let me test this.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@lizozom
Copy link
Contributor

lizozom commented Nov 17, 2019

@elasticmachine merge upstream

@lizozom
Copy link
Contributor

lizozom commented Nov 17, 2019

jenkins test this

lizozom
lizozom approved these changes Nov 17, 2019
View reviewed changes
Copy link
Contributor

@lizozom lizozom left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

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

@elasticmachine
Copy link
Contributor

💔 Build Failed

@lizozom
Copy link
Contributor

lizozom commented Nov 17, 2019

@elasticmachine merge upstream

@lizozom
Copy link
Contributor

lizozom commented Nov 17, 2019

jenkins test this

@lizozom
Copy link
Contributor

lizozom commented Nov 17, 2019

retest

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@lizozom 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
@sainthkh
ui/resize_checker 👉 src/plugins/kibana_utils (elastic#44750)
2596c9d 
* Moved ResizeChecker to kibana_utils.

* Removed ResizeChecker from __LEGACY.
@lizozom lizozom mentioned this pull request Nov 17, 2019
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.
jloleysens added a commit to jloleysens/kibana that referenced this pull request Nov 18, 2019
@jloleysens
Merge remote-tracking branch 'upstream/master' into np-migration/watc…
58b7d0c 
…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)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@peteharverson peteharverson peteharverson approved these changes

+1 more reviewer

@lizozom lizozom lizozom approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Feature:NP Migration release_note:skip Skip the PR/issue when compiling release notes review v7.6.0 v8.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ui/resize_checker 👉 src/plugins/kibana_utils

6 participants

@sainthkh @elasticmachine @mattkime @ppisljar @lizozom @peteharverson