Skip to content
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.

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

[Upgrade Assistant] Refactor CITs #109536

Merged
merged 2 commits into from
Aug 20, 2021
Merged

Conversation

alisonelizabeth
Copy link
Contributor

@alisonelizabeth alisonelizabeth commented Aug 20, 2021

Addresses the second part of #107053 (comment).

I've co-located the helper files with the tests. For example, es_deprecations.helpers.ts now lives in __jest__/client_integration/es_deprecations, rather than the top-level helpers directory.

I think we could consider breaking es_deprecations.helpers.ts into smaller files to map to the different test files within this directory, but I'm not sure it's necessary at this time. For now, I tried to scope the actions so it's a little more clear when scanning the file.

I also removed the TestSubjects types, as I don't find it very useful and is burdensome to maintain.

@alisonelizabeth alisonelizabeth added Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more release_note:skip Skip the PR/issue when compiling release notes Feature:Upgrade Assistant v7.16.0 labels Aug 20, 2021
@@ -46,27 +47,7 @@ describe('Overview - Fix deprecated settings step', () => {
expect(find('esStatsPanel.criticalDeprecations').text()).toContain('1');
});

test('Handles network failure', async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test was duplicated in this file.

@@ -78,7 +59,7 @@ describe('Overview - Fix deprecated settings step', () => {
expect(exists('noDeprecationsLabel')).toBe(true);
});

test('Stats panel navigates to deprecations list if clicked', () => {
test('Stats panel contains link to ES deprecations page', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reworded this a bit since the test doesn't actually click the link

@alisonelizabeth
Copy link
Contributor Author

alisonelizabeth commented Aug 20, 2021

@sabarasaba do you recall why you introduced the KibanaContextProvider in #106521?

I didn't notice it while reviewing your PR, but when I was looking at the tests again, I found it a bit unclear on what is provided by the Kibana context services object vs. the app context. It seems like we might only need one or the other, but it's possible I might be missing something 😅 .

@alisonelizabeth alisonelizabeth marked this pull request as ready for review August 20, 2021 19:01
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-stack-management (Team:Stack Management)

Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

So much easier to navigate! Thanks for doing this Alison.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

@sabarasaba
Copy link
Member

@sabarasaba do you recall why you introduced the KibanaContextProvider in #106521?

I saw in other apps they were using KibanaContext for plugin dependencies and AppContext for the rest of the things (cant exactly recall which ones right now) so I thought it was a pattern or something that everyone follows. But from your comment I understand that is not a thing, so I can work on getting rid of it and just passing everything through AppContext. I've created this issue to keep track of this and will address it soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Feature:Upgrade Assistant release_note:skip Skip the PR/issue when compiling release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.16.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants