[NP] Move visTypeVislib into NP #63963

sulemanof · 2020-04-20T14:19:56Z

Summary

Moving the vis_type_vislib into NP.
All possible unit tests were moved to jest setup, other were moved into src/legacy/core_plugins/kibana/public/__tests__/vis_type_vislib to be run via karma setup.

src/legacy/core_plugins/kibana/public/__tests__/**/* path was included to be ignored in .eslintrc.js for the @kbn/eslint/no-restricted-paths rule. So that it's applicable for all the next tests which will be move to that folder and which will import files from migrated to NP plugins

Checklist

Delete any items that are not applicable to this PR.

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
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser
This was checked for cross-browser compatibility, including a check against IE11

For maintainers

This was checked for breaking API changes and was labeled appropriately

# Conflicts: # x-pack/legacy/plugins/tilemap/index.js

elasticmachine · 2020-04-21T11:59:25Z

Pinging @elastic/kibana-app (Team:KibanaApp)

flash1293

Tested and looks mostly good to me, just left two comments about the separation of legacy tests.

flash1293 · 2020-04-22T12:42:37Z

src/plugins/vis_type_vislib/public/fixtures/_vis_fixture.js

@@ -20,13 +20,20 @@
 import _ from 'lodash';


It seems like most of this file is only used in the legacy tests, can we move it over there? The only thing used in NP jest tests as well is the getMockUiState function, looks like it makes sense to move that one into a separate file so we don't have to import things deeply from within other plugins and mess with jquery in the new platform (and keep it in the legacy world instead).

flash1293 · 2020-04-22T12:50:54Z

.eslintignore

@@ -10,7 +10,7 @@ bower_components
 /html_docs
 /src/plugins/data/common/es_query/kuery/ast/_generated_/**
 /src/plugins/vis_type_timelion/public/_generated_/**
-src/legacy/core_plugins/vis_type_vislib/public/vislib/__tests__/lib/fixtures/mock_data
+/src/plugins/vis_type_vislib/public/fixtures/mock_data


Might be a good opportunity to fix the formatting in these files

Okay, I've done it in a separate commit in case of any concerns (the diff has increased significantly)

# Conflicts: # .i18nrc.json

flash1293

Checked the diff and testing worlds are nicely separated now. LGTM

jgowdyelastic

ML changes LGTM

mistic

The changes in the files under operations team code owners LGTM

cchaos · 2020-04-23T15:39:10Z

src/legacy/core_plugins/kibana/public/index.scss

-@import 'src/legacy/core_plugins/vis_type_vislib/public/index';
+// vis_type_vislib UI styles are imported here for running karma Browser tests
+// should be somehow included through the "vis_type_vislib" plugin initialization
+@import '../../../../plugins/vis_type_vislib/public/index';


Since you have this imported here and in the plugin, doesn't it duplicate the compiled styles?

Yes, it's duplicated temporary. We'll get rid of this legacy compilation for public code and will only use this for testing.

@sulemanof For now we can comment out the import in the plugin because the chart will always render in the legacy platform. When cutting over visualize and dashboard to the new platform we can revisit this again.

Thanks @flash1293 That's what I'd suggest as well. We really want to avoid duplicate styles

@sulemanof I found a workaround we can use here. The tests_bundle is stripped out for the production build, but it's loaded in karma tests. We can put the "duplicated" styles there, so we don't pollute anything and won't have problems when doing the cutover. PR: sulemanof#2

@flash1293 thanks for your effort! I think it's the best solution!
Merged into current branch.

@cchaos are you OK with the changes?

move test styles to tests_bundle

# Conflicts: # .i18nrc.json

cchaos · 2020-04-24T17:02:58Z

src/legacy/core_plugins/tests_bundle/public/index.scss

+// vis_type_vislib UI styles are imported here for running karma Browser tests
+// should be somehow included through the "vis_type_vislib" plugin initialization
+@import '../../../../plugins/vis_type_vislib/public/index';
+
+// Visualization styles are imported here for running karma Browser tests
+// should be somehow included through the "visualizations" plugin initialization
+@import '../../../../plugins/visualizations/public/index';


This is fine, though you probably don't have to repeat this same comment. Just add one at the very top of the file that explains the necessity of the file itself.

@cchaos you are right, done.

This reverts commit 6a82d9c.

flash1293 · 2020-04-26T06:08:49Z

@elasticmachine merge upstream

flash1293 · 2020-04-26T08:51:42Z

Jenkins, test this.

flash1293 · 2020-04-28T07:00:36Z

@elasticmachine merge upstream

kibanamachine · 2020-04-28T09:01:10Z

💛 Build succeeded, but was flaky

continuous-integration/kibana-ci/pull-request
Commit: 62400d6
Flaky suites:
- xpack-kibana-ciGroup2

History

💚 Build #43254 succeeded 0382708
💔 Build #43252 failed 0382708
💔 Build #43251 failed 9b12ef6
💔 Build #43088 failed c59d718
💚 Build #43038 succeeded eefb960

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

cchaos

SASS is good

sulemanof added 4 commits April 20, 2020 16:47

Move vis_type_vislib into NP

3911e57

Merge remote-tracking branch 'upstream/master' into np/vis_type_vislib

a7cb184

# Conflicts: # x-pack/legacy/plugins/tilemap/index.js

Move the rest of tests to jest, do a clean up

57f8338

Merge remote-tracking branch 'upstream/master' into np/vis_type_vislib

99197ad

sulemanof requested a review from flash1293 April 21, 2020 11:58

sulemanof marked this pull request as ready for review April 21, 2020 11:58

sulemanof requested a review from a team April 21, 2020 11:58

sulemanof requested review from a team as code owners April 21, 2020 11:58

sulemanof added Feature:NP Migration Feature:Vislib Vislib chart implementation release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.8.0 v8.0.0 labels Apr 21, 2020

flash1293 mentioned this pull request Apr 21, 2020

Meta: Kibana app NP cutover #60097

Closed

69 tasks

flash1293 reviewed Apr 22, 2020

View reviewed changes

sulemanof added 4 commits April 23, 2020 11:41

Merge branch 'master' into np/vis_type_vislib

a861935

Divide legacy helpers

2a038b2

Add licenses to mock data, fix eslint issues

05a5e9e

Merge remote-tracking branch 'upstream/master' into np/vis_type_vislib

ebbb1be

# Conflicts: # .i18nrc.json

flash1293 approved these changes Apr 23, 2020

View reviewed changes

jgowdyelastic approved these changes Apr 23, 2020

View reviewed changes

mistic approved these changes Apr 23, 2020

View reviewed changes

cchaos reviewed Apr 23, 2020

View reviewed changes

flash1293 and others added 3 commits April 24, 2020 10:10

move test styles to tests_bundle

9c0f47e

Merge pull request #2 from flash1293/np/vis_type_vislib-styles

eefb960

move test styles to tests_bundle

Merge remote-tracking branch 'upstream/master' into np/vis_type_vislib

c59d718

# Conflicts: # .i18nrc.json

cchaos reviewed Apr 24, 2020

View reviewed changes

flash1293 added 4 commits April 25, 2020 22:13

Merge remote-tracking branch 'upstream/master' into np/vis_type_vislib

cfa58b1

fix eslint warnings

6a82d9c

Revert "fix eslint warnings"

835cf4a

This reverts commit 6a82d9c.

fix linting exception

9b12ef6

Merge branch 'master' into np/vis_type_vislib

0382708

Merge branch 'master' into np/vis_type_vislib

62400d6

cchaos approved these changes Apr 28, 2020

View reviewed changes

flash1293 merged commit da89856 into elastic:master Apr 28, 2020

flash1293 mentioned this pull request Apr 28, 2020

[7.x] [NP] Move visTypeVislib into NP (#63963) #64642

Merged

flash1293 pushed a commit to flash1293/kibana that referenced this pull request Apr 28, 2020

[NP] Move visTypeVislib into NP (elastic#63963)

c0f0353

flash1293 added a commit that referenced this pull request Apr 28, 2020

[NP] Move visTypeVislib into NP (#63963) (#64642)

298edf9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NP] Move visTypeVislib into NP #63963

[NP] Move visTypeVislib into NP #63963

sulemanof commented Apr 20, 2020 •

edited

Loading

elasticmachine commented Apr 21, 2020

flash1293 left a comment

flash1293 Apr 22, 2020 •

edited

Loading

sulemanof Apr 23, 2020

flash1293 Apr 22, 2020

sulemanof Apr 23, 2020

flash1293 left a comment

jgowdyelastic left a comment

mistic left a comment

cchaos Apr 23, 2020

sulemanof Apr 23, 2020

flash1293 Apr 23, 2020

cchaos Apr 23, 2020

flash1293 Apr 24, 2020 •

edited

Loading

sulemanof Apr 24, 2020

sulemanof Apr 24, 2020

cchaos Apr 24, 2020

flash1293 Apr 26, 2020

flash1293 commented Apr 26, 2020

flash1293 commented Apr 26, 2020

flash1293 commented Apr 28, 2020

kibanamachine commented Apr 28, 2020

cchaos left a comment

[NP] Move visTypeVislib into NP #63963

[NP] Move visTypeVislib into NP #63963

Conversation

sulemanof commented Apr 20, 2020 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Apr 21, 2020

flash1293 left a comment

Choose a reason for hiding this comment

flash1293 Apr 22, 2020 • edited Loading

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

flash1293 left a comment

Choose a reason for hiding this comment

jgowdyelastic left a comment

Choose a reason for hiding this comment

mistic left a 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

flash1293 Apr 24, 2020 • edited Loading

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

flash1293 commented Apr 26, 2020

flash1293 commented Apr 26, 2020

flash1293 commented Apr 28, 2020

kibanamachine commented Apr 28, 2020

💛 Build succeeded, but was flaky

History

cchaos left a comment

Choose a reason for hiding this comment

sulemanof commented Apr 20, 2020 •

edited

Loading

flash1293 Apr 22, 2020 •

edited

Loading

flash1293 Apr 24, 2020 •

edited

Loading