Skip to content

Move deprecations getters test helpers to package#137793

Merged
TinaHeiligers merged 6 commits intoelastic:mainfrom
TinaHeiligers:move-depcrecation-test-helpers
Aug 3, 2022
Merged

Move deprecations getters test helpers to package#137793
TinaHeiligers merged 6 commits intoelastic:mainfrom
TinaHeiligers:move-depcrecation-test-helpers

Conversation

@TinaHeiligers
Copy link
Copy Markdown
Contributor

@TinaHeiligers TinaHeiligers commented Aug 1, 2022

Summary

Core's deprecations service exposes two test helpers that are used in other core packages and exposed in legacy src/core/server/test_utils. At the moment, having these test helpers explicitly exported from the main implementation violates the @kbn/imports/no_boundary_crossing rule.

This PR attempts to fix that by moving the test helpers to a new package.

Note, if moving shared test utilities to packages is the way forward, we should consider consolidating all core-related test utils into a smaller collection of packages. However, this carries the burden of having to ensure we don't unintentionally introduce circular dependencies.

@TinaHeiligers TinaHeiligers added Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// release_note:skip Skip the PR/issue when compiling release notes backport:skip This PR does not require backporting v8.5.0 labels Aug 1, 2022
@TinaHeiligers TinaHeiligers requested a review from spalger August 1, 2022 22:17
@TinaHeiligers TinaHeiligers marked this pull request as ready for review August 1, 2022 23:29
@TinaHeiligers TinaHeiligers requested a review from a team as a code owner August 1, 2022 23:29
@elasticmachine
Copy link
Copy Markdown
Contributor

Pinging @elastic/kibana-core (Team:Core)

Copy link
Copy Markdown
Member

@afharo afharo left a comment

Choose a reason for hiding this comment

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

It looks great to me! I just added 2 minor changes that slipped through the cracks :)

"include": [
"src/**/*"
]
, "../../test-helpers/core-test-helpers-deprecations-getters/src/test_utils.ts" ]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Q: Do we need this? AFAIK, it's an "npm" dependency, so we don't need to reference it like that. Do we?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I didn't notice this, this is referencing a file outside of the package, which isn't going to be available in bazel. We should just be able to import the @kbn/core-test-helpers-deprecations-getters package in the code and be fine

Copy link
Copy Markdown
Contributor Author

@TinaHeiligers TinaHeiligers Aug 2, 2022

Choose a reason for hiding this comment

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

FWIW, the change came from VSCode, with choosing the lazy option of updating all paths.

@@ -0,0 +1,3 @@
# @kbn/core-test-helpers-deprecations-getters

Empty package generated by @kbn/generate
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would you mind adding a description for this package? 😇

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch! I threw up this PR as a POC and didn't think to come back to the README.

@TinaHeiligers TinaHeiligers requested review from a team and afharo August 2, 2022 17:45
@TinaHeiligers TinaHeiligers enabled auto-merge (squash) August 3, 2022 00:30
Copy link
Copy Markdown
Member

@afharo afharo left a comment

Choose a reason for hiding this comment

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

Momentarily blocking the PR to apply the author suggestion and avoid auto merge to merge

@kibana-ci
Copy link
Copy Markdown

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/core-config-server-internal 13 4 -9
@kbn/core-test-helpers-deprecations-getters - 9 +9
total -0
Unknown metric groups

API count

id before after diff
@kbn/core-config-server-internal 15 4 -11
@kbn/core-test-helpers-deprecations-getters - 11 +11
total -0

ESLint disabled line counts

id before after diff
@kbn/core-config-server-internal 1 0 -1

Total ESLint disabled count

id before after diff
@kbn/core-config-server-internal 1 0 -1

History

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

@TinaHeiligers TinaHeiligers merged commit b0f5a63 into elastic:main Aug 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:skip This PR does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// v8.5.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants