-
Notifications
You must be signed in to change notification settings - Fork 8k
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
[http] Enhance versioned router mock for easier introspection #159669
[http] Enhance versioned router mock for easier introspection #159669
Conversation
Pinging @elastic/kibana-core (Team:Core) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. API looks fine, some minor nits regarding the implementation
packages/core/http/core-http-router-server-mocks/src/versioned_router.mock.ts
Outdated
Show resolved
Hide resolved
let route: undefined | RegisteredVersionedRoute; | ||
for (let x = 0; x < router[method].mock.calls.length; x++) { | ||
const [routeConfig] = router[method].mock.calls[x]; | ||
if (routeConfig.path === path) { | ||
const mockedAddVersion = router[method].mock.results[x].value as MockedVersionedRoute; | ||
route = { | ||
config: routeConfig, | ||
versions: mockedAddVersion.addVersion.mock.calls.reduce( | ||
(acc, [config, handler]) => ({ | ||
...acc, | ||
[config.version]: { config, handler }, | ||
}), | ||
{} | ||
), | ||
}; | ||
break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: I would have done that within the MockedVersionedRouter
implementation (basically constructing this map / info at registration time) instead of going though the mock's calls in a second time. But as long as it works, it's fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that was an alternative I also considered, but my guess is that this function won't be used a lot so I sacrificed time over space. Might be the wrong assumption, but we should be able to change the implementation without breaking stuff down the line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @jloleysens for these changes. This looks awesome 👍
|
||
export interface RegisteredVersionedRoute { | ||
config: VersionedRouteConfig<any>; | ||
versions: { [version: string]: { config: AddVersionOpts<any, any, any>; handler: any } }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should handler
here be:
RequestHandler<any, any, any, any, any, KibanaResponseFactory>
This will ensure that what we get back matches what was registered - with the exception that Context
is being set to any
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! Good catch, I left any
there while I was concentrating on something else and did not return to it! I'll update per your suggestion.
💚 Build Succeeded
Metrics [docs]Public APIs missing comments
Public APIs missing exports
Unknown metric groupsAPI count
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @jloleysens |
Summary
Adds a helper/util to the Versioned router mock so that tests can more easily be expressed against registered versioned routes.
Usage
See included test. Thanks @paul-tavares for providing some prior art, I adapted slightly to rather return all the versions. Let me know what you think!
CC @pgayvallet @paul-tavares