Get rid of `axios` dependency in the Upgrade Assistant tests. by azasypkin · Pull Request #127122 · elastic/kibana

azasypkin · 2022-03-08T09:49:53Z

Summary

In #111655 (comment) we're trying to upgrade axios, but are blocked because of this axios usage in the tests (axios just happened to have methods signature similar to HttpSetup, but it's technically incorrect and quite fragile):

🚫 const httpSetup = axios.create({ adapter: axiosXhrAdapter }) as unknown as HttpSetup; 🚫

In this PR I switched Upgrade Assistant tests to use HttpSetup mock instead of axios + sinon server mock preserving test helpers shape as much as possible. Feel free to scratch this PR if you have a better approach in mind!

Re-merged after revert via: #127784

sabarasaba

Thanks for working on this @azasypkin! Most changes lgtm, though I left some questions/observations that I would like to get your opinion on 🎸

sabarasaba · 2022-03-11T10:11:51Z

...grade_assistant/__jest__/client_integration/es_deprecation_logs/es_deprecation_logs.test.tsx

-    httpRequestsMockHelpers.setLoadDeprecationLoggingResponse(getLoggingResponse(true));
-    testBed = await setupESDeprecationLogsPage();
+    mockEnvironment = setupEnvironment();
+    mockEnvironment.httpRequestsMockHelpers.setLoadDeprecationLoggingResponse(


What do you think of destructing httpRequestsMockHelpers from mockEnvironment? I think that will reduce the noise of the diff and also make things easier to follow since the changes will be scoped to the setup of the test rather the implementation of each.

I think this change can be applied throughout most of the test files in this PR.

Yep, good point, let me do that!

sabarasaba · 2022-03-11T10:30:45Z

x-pack/plugins/upgrade_assistant/__jest__/client_integration/helpers/http_requests.ts

-      { 'Content-Type': 'application/json' },
-      JSON.stringify(body),
-    ]);
+  const mockResponse = (method: HttpMethod, path: string, response?: unknown, error?: unknown) => {


Love it! But was wondering if we should perhaps move all of this to a shared place (perhaps kbn-test) from which we could re-use this in other apps also? I think would also be cool if we could optionally specify headers too!

I see your point, but the approach you use in UA and that was copied to few other plugins (additional abstraction over existing Core's mocks) is more like an exception and not something we (Platform) recommend. It's fine for a simple use case like you have, but if you need to customize response behavior (e.g. reply with different mock response based on parameters etc.) it becomes harder than using the Core's mock directly and also it's a bit harder to follow for people outside of the team.

So I'd rather localize this approach only inside plugins that opted to use it for now, until we have enough evidence that it can be useful for the majority of the plugins.

What do you think?

Makes sense 👍🏼 , I think most of the platform-management apps use this pattern but let's keep an eye open for the other kibana apps and see if this is something more teams could benefit from.

Yep, sounds good 👍

sabarasaba · 2022-03-11T10:40:05Z

...gins/upgrade_assistant/__jest__/client_integration/es_deprecations/deprecations_list.test.ts

-    expect(server.requests.length).toBe(totalRequests + 4);
-    expect(server.requests[server.requests.length - 4].url).toBe(
-      `${API_BASE_PATH}/es_deprecations`
+    expect(mockEnvironment.httpSetup.get).toHaveBeenCalledWith(


If we are to refactor out mockResponse into kbn-test I think would be nice to have some sort of mechanism to access an array of requests made like what sinnon provides, given that you can choose what to match against rather than having to have an ordered list of expect get toHaveBeenCalledWith

kibana-ci · 2022-03-14T13:59:02Z

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

💚 Build #28305 succeeded 9a7a426

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

sabarasaba

latest lgtm, thanks @azasypkin!

azasypkin · 2022-03-15T09:41:26Z

Thanks for the review @sabarasaba !

kibanamachine · 2022-03-15T09:45:09Z

💔 All backports failed

Status	Branch	Result
❌	7.17	Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 127122

Questions ?

Please refer to the Backport tool documentation

…127122) (#127716) * Get rid of `axios` dependency in the Upgrade Assistant tests. (#127122) (cherry picked from commit 53420d8) # Conflicts: # x-pack/plugins/upgrade_assistant/__jest__/client_integration/es_deprecation_logs/es_deprecation_logs.helpers.ts # x-pack/plugins/upgrade_assistant/__jest__/client_integration/es_deprecations/es_deprecations.helpers.ts # x-pack/plugins/upgrade_assistant/__jest__/client_integration/helpers/setup_environment.tsx # x-pack/plugins/upgrade_assistant/__jest__/client_integration/kibana_deprecations/kibana_deprecations.helpers.ts # x-pack/plugins/upgrade_assistant/__jest__/client_integration/overview/overview.helpers.ts * Fix formatting.

…#127122)" This reverts commit 53420d8.

jbudz · 2022-03-15T14:55:17Z

@azasypkin FYI I reverted this at 7b86f00 due to a conflict with #126693 in main. It looks like a fix is in progress at #127779

azasypkin · 2022-03-15T14:58:40Z

@azasypkin FYI I reverted this at 7b86f00 due to a conflict with #126693 in main. It looks like a fix is in progress at #127779

Ugh, sorry about that. The fix won't work without the main PR - I'll re-create it including the changes from #127779

sabarasaba · 2022-03-15T15:03:15Z

Thanks @azasypkin! I'll go ahead and close my pr then, ping me when you have this other PR open and I can have a look 👍🏼

azasypkin · 2022-03-15T16:05:03Z

Thanks @azasypkin! I'll go ahead and close my pr then, ping me when you have this other PR open and I can have a look 👍🏼

Thanks, will do!

…c#127122)

…elastic#127122)" This reverts commit 53420d8.

Get rid of axios dependency in the Upgrade Assistant tests.

9a7a426

azasypkin mentioned this pull request Mar 8, 2022

Upgrade axios dependency (0.21.1 → 0.27.2). #111655

Merged

azasypkin marked this pull request as ready for review March 8, 2022 11:29

azasypkin requested a review from a team as a code owner March 8, 2022 11:29

azasypkin added auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes v7.17.2 v8.2.0 labels Mar 8, 2022

sabarasaba reviewed Mar 11, 2022

View reviewed changes

Merge branch 'main' into issue-xxx-axios-upgrade-assistant

aa5feb7

sabarasaba mentioned this pull request Mar 14, 2022

Jest axe a11y testing #127185

Merged

Review#1: destructure setupEnvironment result.

264bd15

azasypkin requested a review from sabarasaba March 14, 2022 16:40

sabarasaba approved these changes Mar 15, 2022

View reviewed changes

azasypkin merged commit 53420d8 into elastic:main Mar 15, 2022

azasypkin deleted the issue-xxx-axios-upgrade-assistant branch March 15, 2022 09:41

azasypkin mentioned this pull request Mar 15, 2022

[7.17] Get rid of axios dependency in the Upgrade Assistant tests. (#127122) #127716

Merged

jbudz added a commit that referenced this pull request Mar 15, 2022

Revert "Get rid of axios dependency in the Upgrade Assistant tests. (…

7b86f00

…#127122)" This reverts commit 53420d8.

sabarasaba mentioned this pull request Mar 15, 2022

[Upgrade Assistant] Fix broken test that is missing HttpSetup instance in setup method #127779

Closed

jbudz added the reverted label Mar 15, 2022

azasypkin mentioned this pull request Mar 15, 2022

Get rid of axios dependency in the Upgrade Assistant tests. #127784

Merged

azasypkin mentioned this pull request Mar 17, 2022

Get rid of axios dependency in the data_view_field_editor tests. #127965

Closed

azasypkin mentioned this pull request Mar 17, 2022

Get rid of axios dependency in the platform deployment management tests. #127966

Closed

7 tasks

maksimkovalev pushed a commit to maksimkovalev/kibana that referenced this pull request Mar 18, 2022

Get rid of axios dependency in the Upgrade Assistant tests. (elasti…

42148bd

…c#127122)

maksimkovalev pushed a commit to maksimkovalev/kibana that referenced this pull request Mar 18, 2022

Revert "Get rid of axios dependency in the Upgrade Assistant tests. (…

8511f3b

…elastic#127122)" This reverts commit 53420d8.

sabarasaba mentioned this pull request Mar 24, 2022

[IM] Remove axios dependency in tests #128171

Merged

Conversation

azasypkin commented Mar 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

sabarasaba left a comment

Choose a reason for hiding this comment

Uh oh!

sabarasaba Mar 11, 2022

Choose a reason for hiding this comment

Uh oh!

azasypkin Mar 14, 2022

Choose a reason for hiding this comment

Uh oh!

sabarasaba Mar 11, 2022

Choose a reason for hiding this comment

Uh oh!

azasypkin Mar 14, 2022

Choose a reason for hiding this comment

Uh oh!

sabarasaba Mar 15, 2022

Choose a reason for hiding this comment

Uh oh!

azasypkin Mar 15, 2022

Choose a reason for hiding this comment

Uh oh!

sabarasaba Mar 11, 2022

Choose a reason for hiding this comment

Uh oh!

kibana-ci commented Mar 14, 2022

💚 Build Succeeded

Metrics [docs]

History

Uh oh!

sabarasaba left a comment

Choose a reason for hiding this comment

Uh oh!

azasypkin commented Mar 15, 2022

Uh oh!

kibanamachine commented Mar 15, 2022

💔 All backports failed

Manual backport

Questions ?

Uh oh!

jbudz commented Mar 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

azasypkin commented Mar 15, 2022

Uh oh!

sabarasaba commented Mar 15, 2022

Uh oh!

azasypkin commented Mar 15, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

azasypkin commented Mar 8, 2022 •

edited

Loading

jbudz commented Mar 15, 2022 •

edited

Loading