Refactor HttpService tests #53033

joshdover · 2019-12-13T19:37:47Z

Summary

Follow up from #52434, this PR cleans up the HttpService more with these changes:

Splits out unit tests to separate files, targeted at the correct unit under test
Refactors the structure of the HttpService class to better match Core's conventions
Splits out the loadingCount functionality and tests to a separate sub-service
Removes some duplicate tests from HttpService & BasePath

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.
(https://github.com/elastic/kibana/blob/master/packages/kbn-i18n/README.md)

Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

joshdover · 2019-12-13T21:36:32Z

src/core/types/core_service.ts

-  stop(): Promise<void>;
+  setup(...params: any[]): TSetup | Promise<TSetup>;
+  start(...params: any[]): TStart | Promise<TStart>;
+  stop(): void | Promise<void>;


I made this change to avoid making HttpService async and breaking a test utility that is used in quite a few places. I can also move this to a separate PR if we think that is better.

I think it's alright and that it's actually closer to the various services implementations

elasticmachine · 2019-12-13T21:39:21Z

Pinging @elastic/kibana-platform (Team:Platform)

dgieselaar

APM changes LGTM 👍

lizozom

@elastic/kibana-app-arch team changes include rename of HttpServiceBase to HttpSetup.
Added 1 comment about replacing it with HttpStart

src/plugins/data/public/index_patterns/index_patterns/index_patterns.ts

sorenlouv

APM changes lgtm

pgayvallet

Great cleanup imho. Most of my comments are on existing code and not directly related to the PR changes.

src/core/public/http/anonymous_paths_service.ts

pgayvallet · 2019-12-17T07:59:25Z

src/core/public/http/http_service.ts

+  public start(deps: HttpDeps) {
+    return this.setup(deps);


(Probably a NIT) we are executing twice the setup code by doing this, instead of once by storing as it was done before. The overheat is minimal (for now), but is there any upside on this change?

Not necessary, but it's (very slightly) more consistent with how other services work.

src/core/public/http/loading_count_service.test.ts

src/core/public/http/loading_count_service.ts

pgayvallet · 2019-12-17T08:15:38Z

src/core/public/public.api.md

+    // Warning: (ae-unresolved-link) The @link reference could not be resolved: The package "kibana" does not have an export "LoadingCountSetup"
+    addLoadingCount: LoadingCountSetup['addLoadingCount'];


NIT: this is why I think we should stop using the Internal[XXX] and actually make the 'effort' of adding proper public interfaces for all exported contracts. This is additional boilerplate I agree, but we will need them anyway when we'll cleanup the documentation.

This is a shame our doc generator is not able to properly 'replace' this kind of definition on the fly though.

This is a shame our doc generator is not able to properly 'replace' this kind of definition on the fly though.

I agree. I think part of the complexity of building a tool that does this is knowing when to show the resolved type and when to show a link to a different page.

For now, I'll switch this to put the documented definition on the HttpSetup interface and duplicate it on the LoadingCountSetup interface.

pgayvallet · 2019-12-17T08:19:25Z

src/core/types/core_service.ts

-  stop(): Promise<void>;
+  setup(...params: any[]): TSetup | Promise<TSetup>;
+  start(...params: any[]): TStart | Promise<TStart>;
+  stop(): void | Promise<void>;


I think it's alright and that it's actually closer to the various services implementations

src/plugins/data/public/ui/query_string_input/__snapshots__/query_string_input.test.tsx.snap

elasticmachine · 2019-12-18T20:39:38Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 8c378ca

History

💚 Build #16309 succeeded 4bc4c51f207d76665b9dd814799db9e3b1937ed4
💔 Build #15945 failed 18f00968892a55622fdd225bdd676d754c942b50
💔 Build #15890 failed 0237996d66cd036ab2ecfa43daa7502e0efd3f0b
💔 Build #15541 failed 6945a1adfc2f051755f088022ed0390064dc75af
💔 Build #15526 failed 6bc8be755a9d1e2434c735a2a675e80694bffea4

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

joshdover · 2019-12-19T17:38:05Z

Merging without approval from other teams since it's just a type rename.

joshdover force-pushed the np/http-refactor-tests branch 2 times, most recently from 6bc8be7 to 6945a1a Compare December 13, 2019 21:35

joshdover commented Dec 13, 2019

View reviewed changes

joshdover added Feature:New Platform release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.6.0 labels Dec 13, 2019

joshdover marked this pull request as ready for review December 13, 2019 21:39

joshdover requested a review from a team as a code owner December 13, 2019 21:39

joshdover requested a review from a team December 13, 2019 21:39

joshdover requested review from a team as code owners December 13, 2019 21:39

dgieselaar approved these changes Dec 14, 2019

View reviewed changes

lizozom reviewed Dec 16, 2019

View reviewed changes

src/plugins/data/public/index_patterns/index_patterns/index_patterns.ts Outdated Show resolved Hide resolved

sorenlouv approved these changes Dec 16, 2019

View reviewed changes

joshdover force-pushed the np/http-refactor-tests branch 3 times, most recently from 0237996 to 18f0096 Compare December 16, 2019 23:14

pgayvallet reviewed Dec 17, 2019

View reviewed changes

joshdover force-pushed the np/http-refactor-tests branch from 18f0096 to 4bc4c51 Compare December 17, 2019 23:46

joshdover requested a review from a team as a code owner December 17, 2019 23:46

joshdover requested a review from pgayvallet December 17, 2019 23:46

joshdover added 5 commits December 18, 2019 13:21

Refactor HttpService to match Core conventions

ba160dc

Refactor unit tests to appropriate locations

4a9dde9

Simplify HttpService tests

6d4093e

Fix snapshot test

64be3cd

Fix anonymous paths

eef55b5

joshdover added 2 commits December 18, 2019 13:21

Use HttpSetup

2f30bb2

PR comments

8c378ca

joshdover force-pushed the np/http-refactor-tests branch from 4bc4c51 to 8c378ca Compare December 18, 2019 19:22

pgayvallet approved these changes Dec 19, 2019

View reviewed changes

joshdover merged commit 4980387 into elastic:master Dec 19, 2019

joshdover deleted the np/http-refactor-tests branch December 19, 2019 17:38

joshdover mentioned this pull request Dec 19, 2019

[7.x] Refactor HttpService tests (#53033) #53614

Merged

joshdover added a commit that referenced this pull request Dec 19, 2019

[7.x] Refactor HttpService tests (#53033) (#53614)

68f77d9

jportner mentioned this pull request Feb 7, 2020

Kibana Platform creates two HTTP services #57148

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor HttpService tests #53033

Refactor HttpService tests #53033

joshdover commented Dec 13, 2019 •

edited

Loading

joshdover Dec 13, 2019

pgayvallet Dec 17, 2019

elasticmachine commented Dec 13, 2019

dgieselaar left a comment

lizozom left a comment

sorenlouv left a comment

pgayvallet left a comment

pgayvallet Dec 17, 2019

joshdover Dec 17, 2019

pgayvallet Dec 17, 2019

joshdover Dec 17, 2019 •

edited

Loading

pgayvallet Dec 17, 2019

elasticmachine commented Dec 18, 2019

joshdover commented Dec 19, 2019

		// Warning: (ae-unresolved-link) The @link reference could not be resolved: The package "kibana" does not have an export "LoadingCountSetup"
		addLoadingCount: LoadingCountSetup['addLoadingCount'];

Refactor HttpService tests #53033

Refactor HttpService tests #53033

Conversation

joshdover commented Dec 13, 2019 • edited Loading

Summary

Checklist

For maintainers

joshdover Dec 13, 2019

Choose a reason for hiding this comment

pgayvallet Dec 17, 2019

Choose a reason for hiding this comment

elasticmachine commented Dec 13, 2019

dgieselaar left a comment

Choose a reason for hiding this comment

lizozom left a comment

Choose a reason for hiding this comment

sorenlouv left a comment

Choose a reason for hiding this comment

pgayvallet left a comment

Choose a reason for hiding this comment

pgayvallet Dec 17, 2019

Choose a reason for hiding this comment

joshdover Dec 17, 2019

Choose a reason for hiding this comment

pgayvallet Dec 17, 2019

Choose a reason for hiding this comment

joshdover Dec 17, 2019 • edited Loading

Choose a reason for hiding this comment

pgayvallet Dec 17, 2019

Choose a reason for hiding this comment

elasticmachine commented Dec 18, 2019

💚 Build Succeeded

History

joshdover commented Dec 19, 2019

joshdover commented Dec 13, 2019 •

edited

Loading

joshdover Dec 17, 2019 •

edited

Loading