[Uptime] Tests/uptime testing utils #87650

dominiqueclarke · 2021-01-07T15:11:58Z

Summary

Adds testing additional testing utilities

Mock Provider Components

MockKibanaProvider - a component that wraps children in KibanaContextProvider and 118nProvider
MockRouter - a component that wraps children in Router, in addition to KibanaContextProvider and 118nProvider

React testing library custom renders
renderTLWithKibana - a render function that takes a React Element and renders it using react testing library, wrapped in KibanaContextProvider and 118nProvider
renderTLWithRouter - a render function that takes a React Element and renders it using react testing library, wrapped in Router, in addition to KibanaContextProvider and 118nProvider

The idea is that if you don't need a router, you can use one of the Kibana only mock components or custom renders. The assumption is made that if you need a router, you probably need the KibanaContextProvider as well.

Some tests were changed as examples.

elasticmachine · 2021-01-07T15:12:03Z

Pinging @elastic/uptime (Team:uptime)

shahzad31 · 2021-01-07T15:31:15Z

x-pack/plugins/uptime/public/lib/helper/component_test_helpers.tsx

+  );
+};
+
+export const renderTLWithRouter = (


my initial though is that, imo, i don't think we should name it like this renderWithRouter or renderWithKibana stuff.

I wish we can find a better naming? maybe just have it as simple render, and by default we should add kibana, router and redux for all components, since most components in uptime are dependent those.

I don't think we will have a use case where adding those by default will cause problems.

The one reason I didn't name it renderWithRouter is because we already have a renderWithRouter in this same file that we are using for enzyme.

I will consider consolidating into one render function.

dominiqueclarke · 2021-01-07T15:14:11Z

x-pack/plugins/uptime/public/components/monitor/ml/ml_job_link.test.tsx

-        <MLJobLink dateRange={{ to: '', from: '' }} basePath="" monitorId="testMonitor" />
-      </KibanaContextProvider>
+    const { asFragment } = renderTLWithRouter(
+      <MLJobLink dateRange={{ to: '', from: '' }} basePath="" monitorId="testMonitor" />,


This is an example where we needed to extend the core options. You can do so by passing customCoreOptions. If you need to overwrite the entire core and make your own mock, you can do so by passing { kibanaProps: { services: { ...myCustomCore } } }.

I'm unable to figure out why this component needed to call getEditAlertFlyout. The MLJobLink component sets up an href and a few other fields and renders a link.

dominiqueclarke · 2021-01-07T15:15:29Z

...ins/uptime/public/components/monitor/synthetics/step_detail/use_monitor_breadcrumbs.test.tsx

+          <Component />
+        </Route>
+      </MountWithReduxProvider>,
+      { kibanaProps: { services: { ...core } } }


This is an example of overwriting the entire Kibana context core. This is useful in this context to get access to a function for getting the breadcrumbs out of core.

dominiqueclarke · 2021-01-07T15:16:07Z

x-pack/plugins/uptime/public/lib/helper/component_test_helpers.tsx

+  };
+
+  return core;
+};


I wanted to create a mock core that met most of our use cases without having to add to it, but I don't have the context to know what that looks like. I would love feedback here.

TBH this might be something we need to take a couple of attempts at and incrementally improve it as we run into different issues. I think we can try to review the fields we use to a certain extent but it's ok if we need to revisit the default mock.

dominiqueclarke · 2021-01-07T15:17:04Z

x-pack/plugins/uptime/public/lib/helper/component_test_helpers.tsx

+  }: {
+    customHistory?: History;
+    customCoreOptions?: any;
+    kibanaProps?: { services: any };


I'm not sure what the types should be for some of these pieces. I welcome feedback here.

let's remove the custom prefix, i think that's unnecessary

Regarding the types, I believe Kibana Core team exports types for core that we can use here. See https://github.com/elastic/kibana/blob/master/x-pack/plugins/uptime/public/apps/plugin.ts#L7

cc @shahzad31 does that sound right?

i think there might be a kibana core mock as well exported by team, may be use that directly?

i have seen that somewhere

@dominiqueclarke might be worth asking in #kibana-developer-experience channel

dominiqueclarke · 2021-01-07T15:46:56Z

x-pack/plugins/uptime/public/components/monitor/ml/ml_job_link.test.tsx

    );
-    expect(wrapper).toMatchSnapshot();
+    expect(asFragment()).toMatchSnapshot();


I only changes this to useasFragment because I don't have further context about this component to know how to test it without a snapshot. I'm using it as an example for now.

I think we can improve this even more and remove the need for asFragment.

What do you think of 9728d3a? We're essentially testing the user-facing features of the component; specifically we ensure our text is displayed and the href we build is what we expect. As a bonus we can eliminate the snapshot file altogether.

I think that's perfect. I wasn't sure how to format that full url lol.

…tions

justinkambic · 2021-01-07T15:50:22Z

x-pack/plugins/uptime/public/lib/helper/component_test_helpers.tsx

+  customCoreOptions,
+  kibanaProps,
+}: {
+  children: ReactElement;


Are you planning to define an interface rather than specifying the types inline like this? These Mock functions have a lot of parameter overlap.

justinkambic · 2021-01-07T15:52:22Z

x-pack/plugins/uptime/public/lib/helper/component_test_helpers.tsx

+  };
+
+  return core;
+};


TBH this might be something we need to take a couple of attempts at and incrementally improve it as we run into different issues. I think we can try to review the fields we use to a certain extent but it's ok if we need to revisit the default mock.

justinkambic · 2021-01-07T15:53:34Z

x-pack/plugins/uptime/public/components/monitor/synthetics/executed_step.test.tsx

+      <ExecutedStep index={3} step={step} checkGroup={'fake-group'} />
+    );
+
+    expect(getByText(`${step?.synthetics?.step?.index}. ${step?.synthetics?.step?.name}`));


This is exactly what I was hoping to achieve when we talked about this test. It tests the same thing in a much cleaner format.

justinkambic · 2021-01-07T15:54:40Z

x-pack/plugins/uptime/public/components/monitor/synthetics/executed_step.test.tsx


 // FLAKY: https://github.com/elastic/kibana/issues/85899
-describe.skip('ExecutedStep', () => {


We might want to cherry-pick this change to a separate branch and revert it here, since we're going to need to get approval to close the issue before we can remove the skip. I'm fine with keeping the test revision below since they're all skipped in master right now anyway.

justinkambic · 2021-01-07T16:08:41Z

x-pack/plugins/uptime/public/lib/helper/component_test_helpers.tsx

+  }: {
+    customHistory?: History;
+    customCoreOptions?: any;
+    kibanaProps?: { services: any };


Regarding the types, I believe Kibana Core team exports types for core that we can use here. See https://github.com/elastic/kibana/blob/master/x-pack/plugins/uptime/public/apps/plugin.ts#L7

cc @shahzad31 does that sound right?

dominiqueclarke · 2021-01-11T00:34:34Z

@elasticmachine merge upstream

justinkambic

This is looking great, and I've already merged these commits into some other testing that I'm doing and everything went well. I had a few recommendations on typings etc, and some possible improvements to one of the example tests we're touching here.

I checked out this code and made two commits, 9728d3a and 8e35b6b. They summarize most of my revision comments below. Ping with any follow-up.

Thanks for taking this on, it's going to be a big help going forward!

justinkambic · 2021-01-11T12:16:14Z

x-pack/plugins/uptime/public/lib/helper/component_test_helpers.tsx

+}
+
+export function withRouter<T>(WrappedComponent: React.ComponentType<T>, customHistory: History) {
+  const history = customHistory || createMemoryHistory();


It seems like customHistory should be optional since we have a default if it is falsey.

justinkambic · 2021-01-11T15:52:43Z

x-pack/plugins/uptime/public/components/monitor/ml/ml_job_link.test.tsx

    );
-    expect(wrapper).toMatchSnapshot();
+    expect(asFragment()).toMatchSnapshot();


I think we can improve this even more and remove the need for asFragment.

What do you think of 9728d3a? We're essentially testing the user-facing features of the component; specifically we ensure our text is displayed and the href we build is what we expect. As a bonus we can eliminate the snapshot file altogether.

justinkambic · 2021-01-11T15:56:28Z

x-pack/plugins/uptime/public/components/monitor/ml/ml_job_link.test.tsx

@@ -5,28 +5,17 @@
 */

 import React from 'react';
-import { coreMock } from 'src/core/public/mocks';
-import { renderWithRouter, shallowWithRouter } from '../../../lib';
+import { render } from '../../../lib';


One thing I noticed in practice when testing this out was my editor had some issue loading the typings for this function. Is this happening for you? When I edited the path to do a direct import it worked. I can't figure out why this would be a code-related problem, but I'm curious if it's happening with others.

Specifically, I didn't get any intellisense in VS Code for the asFragment function returned by render.

We've had some issues with these types of import chains in the past. Not saying we should mandate always doing direct imports for this helper, but it might be helpful for some.

justinkambic · 2021-01-11T16:03:25Z

x-pack/plugins/uptime/public/lib/helper/component_test_helpers.tsx

+  );
+}
+
+export function withRouter<T>(WrappedComponent: React.ComponentType<T>, customHistory: History) {


It might be good to seek out a test where this HoC can be used and provide an example. I think right now it returns JSX.Element, if we're intending for it to be passed to RTL render directly we might want to make sure the inferred return type is ReactElement. If someone finds this HoC it would be ideal for them to Find all references and see it in action somewhere in the codebase, IMO.

In any case, I think it's rare that we'll need to call this specifically, as the custom render you've created below should cover the vast majority of use cases and is simpler to interface with.

My original idea of using HOCs was a bit misguided, but I left them here in case they are helpful for anyone in any other contexts.

I used to work with RTL using HOCs. I would first create the wrapped component (JSX.Element), and then pass the ReactElement to RTL like so

const WrappedComponent = withRouter(Component, history); render(<WrappedComponent {...props} />);

But that just creates one more unnecessary step.

All in all I think these can be removed.

Ah I see; I'm inclined to agree if we don't have a need for them today. If we run into an instance where using HOCs is preferable, it seems like it will be a small implementation effort to re-add them.

justinkambic · 2021-01-11T16:07:29Z

x-pack/plugins/uptime/public/lib/helper/component_test_helpers.tsx

+  ui: ReactElement,
+  { history, coreOptions, kibanaProps, renderOptions, state }: RenderRouterOptions = {}
+) => {
+  return reactRender(


Maybe we should alias this as reactTestRender to denote it's calling render imported from RTL and not react-dom.

I think if the reader understands the context of this file it's probably not necessary, but some more descriptive name like that (or a better suggestion) might improve readability.

Maybe we should name it something other than reactTestRender since react test renderer is a thing that it may be confused with. How about reactTestLibRender

That sounds fine to me. This way it will disambiguate the source of the function for the person reading the code.

justinkambic · 2021-01-11T16:20:36Z

x-pack/plugins/uptime/public/lib/helper/component_test_helpers.tsx

+}
+
+interface KibanaProviderOptions {
+  coreOptions?: any;


Are we able to reference CoreStart here? As I understand it we use this field to let people supply their own coreOptions, which we spread overtop of the default mock we create. So could we type this as Partial<CoreStart>, so the caller could supply just one or two fields if needed, or a full mock if they wanted?

Suggested change

coreOptions?: any;

coreOptions?: Partial<CoreStart>;

justinkambic · 2021-01-11T16:24:44Z

x-pack/plugins/uptime/public/lib/helper/component_test_helpers.tsx

+interface RenderRouterOptions extends KibanaProviderOptions {
+  history?: History;
+  renderOptions?: any;
+  state?: any;


Suggested change

state?: any;

state?: AppState;

The one thing I don't love about requiring the full app state, is that you have to provide pieces of state that are unnecessary to the particular component you are testing in order to satisfy the interface.

Maybe we could use generics for this and specify the smaller slice of state you need.

Good point; can we use Partial<AppState> instead, and when we

<MountWithReduxProvider store={store}>

change it to:

<MountWithReduxProvider store={...mockStore, ...userStore}>

justinkambic · 2021-01-11T16:25:55Z

x-pack/plugins/uptime/public/lib/helper/component_test_helpers.tsx

+
+interface RenderRouterOptions extends KibanaProviderOptions {
+  history?: History;
+  renderOptions?: any;


I think we should require a type of Omit<RenderOptions, 'queries'>, mimicking the render function that RTL exports.

Suggested change

renderOptions?: any;

renderOptions?: Omit<RenderOptions, 'queries'>;

justinkambic · 2021-01-11T16:47:57Z

x-pack/plugins/uptime/public/components/monitor/ml/ml_job_link.test.tsx

+    const { asFragment } = render(
+      <MLJobLink dateRange={{ to: '', from: '' }} basePath="" monitorId="testMonitor" />,
+      {
+        coreOptions: { triggersActionsUi: { getEditAlertFlyout: jest.fn() } },


Suggested change

coreOptions: { triggersActionsUi: { getEditAlertFlyout: jest.fn() } },

I think the test will succeed without this when we migrate to RTL. See revision in 8e35b6b.

justinkambic · 2021-01-11T16:54:42Z

x-pack/plugins/uptime/public/components/monitor/ml/ml_job_link.test.tsx

-        <MLJobLink dateRange={{ to: '', from: '' }} basePath="" monitorId="testMonitor" />
-      </KibanaContextProvider>
+    const { asFragment } = renderTLWithRouter(
+      <MLJobLink dateRange={{ to: '', from: '' }} basePath="" monitorId="testMonitor" />,


I'm unable to figure out why this component needed to call getEditAlertFlyout. The MLJobLink component sets up an href and a few other fields and renders a link.

dominiqueclarke

Thanks for the feedback! One note about AppState, I'd love to get around requiring the full AppState for redux tests if possible.

dominiqueclarke · 2021-01-11T17:05:49Z

x-pack/plugins/uptime/public/components/monitor/ml/ml_job_link.test.tsx

    );
-    expect(wrapper).toMatchSnapshot();
+    expect(asFragment()).toMatchSnapshot();


I think that's perfect. I wasn't sure how to format that full url lol.

dominiqueclarke · 2021-01-11T17:07:22Z

x-pack/plugins/uptime/public/lib/helper/component_test_helpers.tsx

+interface RenderRouterOptions extends KibanaProviderOptions {
+  history?: History;
+  renderOptions?: any;
+  state?: any;


The one thing I don't love about requiring the full app state, is that you have to provide pieces of state that are unnecessary to the particular component you are testing in order to satisfy the interface.

dominiqueclarke · 2021-01-11T17:10:21Z

x-pack/plugins/uptime/public/lib/helper/component_test_helpers.tsx

+  );
+}
+
+export function withRouter<T>(WrappedComponent: React.ComponentType<T>, customHistory: History) {


My original idea of using HOCs was a bit misguided, but I left them here in case they are helpful for anyone in any other contexts.

I used to work with RTL using HOCs. I would first create the wrapped component (JSX.Element), and then pass the ReactElement to RTL like so

const WrappedComponent = withRouter(Component, history); render(<WrappedComponent {...props} />);

But that just creates one more unnecessary step.

All in all I think these can be removed.

dominiqueclarke · 2021-01-11T17:11:02Z

x-pack/plugins/uptime/public/lib/helper/component_test_helpers.tsx

+  );
+};
+
+/* Enzyme helpers */


They are copied.

…types

…iqueclarke/kibana into tests/uptime-testing-utils

dominiqueclarke · 2021-01-11T19:34:31Z

x-pack/plugins/uptime/public/lib/index.ts

@@ -5,4 +5,4 @@
 */

 export { MountWithReduxProvider } from './helper';
-export * from './helper/helper_with_router';
+export * from './helper/enzyme_helpers';


Exported the enzyme helpers from here in order to maintain compatibility with the existing tests without having to change the imports.

justinkambic

LGTM! WFG

kibanamachine · 2021-01-11T23:36:43Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 1866c29

Metrics [docs]

✅ unchanged

History

💚 Build #98039 succeeded abf0e25
💔 Build #98028 failed ae7406b
💚 Build #97785 succeeded 5ade268
💚 Build #97703 succeeded 48f1536
💔 Build #97604 failed 05b0b6b

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

* add additional component test helpers * add test examples * uptime testing utils remove custom prefix from props and parameter options * skip executed step tests * adjust MlJobLink test * add testing util interfaces * update mock core * combine wrappers into one custom render function * split enzyme helpers and rtl helpers into different files and adjust types * adjust types * spread core on render function * remove unnecessary items from MLJobLink test * update use_monitor_breadcrumbs test Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

* add additional component test helpers * add test examples * uptime testing utils remove custom prefix from props and parameter options * skip executed step tests * adjust MlJobLink test * add testing util interfaces * update mock core * combine wrappers into one custom render function * split enzyme helpers and rtl helpers into different files and adjust types * adjust types * spread core on render function * remove unnecessary items from MLJobLink test * update use_monitor_breadcrumbs test Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

add additional component test helpers

69628a5

dominiqueclarke requested a review from a team as a code owner January 7, 2021 15:11

botelastic bot added the Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability label Jan 7, 2021

dominiqueclarke added v7.11.0 v8.0.0 enhancement New value added to drive a business result release_note:skip Skip the PR/issue when compiling release notes labels Jan 7, 2021

dominiqueclarke marked this pull request as draft January 7, 2021 15:12

add test examples

afea632

dominiqueclarke force-pushed the tests/uptime-testing-utils branch from 40be0cd to afea632 Compare January 7, 2021 15:21

shahzad31 reviewed Jan 7, 2021

View reviewed changes

dominiqueclarke commented Jan 7, 2021

View reviewed changes

uptime testing utils remove custom prefix from props and parameter op…

0ec06b0

…tions

justinkambic reviewed Jan 7, 2021

View reviewed changes

dominiqueclarke added 5 commits January 7, 2021 11:15

skip executed step tests

97d57a6

adjust MlJobLink test

3294340

add testing util interfaces

05b0b6b

update mock core

959c45e

combine wrappers into one custom render function

48f1536

dominiqueclarke force-pushed the tests/uptime-testing-utils branch from 6182468 to 48f1536 Compare January 7, 2021 20:38

dominiqueclarke marked this pull request as ready for review January 7, 2021 20:39

Merge branch 'master' into tests/uptime-testing-utils

5ade268

justinkambic reviewed Jan 11, 2021

View reviewed changes

dominiqueclarke commented Jan 11, 2021

View reviewed changes

dominiqueclarke added 2 commits January 11, 2021 14:23

split enzyme helpers and rtl helpers into different files and adjust …

8f8af46

…types

Merge branch 'tests/uptime-testing-utils' of https://github.com/domin…

ae7406b

…iqueclarke/kibana into tests/uptime-testing-utils

dominiqueclarke force-pushed the tests/uptime-testing-utils branch from fe5c729 to ae7406b Compare January 11, 2021 19:28

dominiqueclarke commented Jan 11, 2021

View reviewed changes

dominiqueclarke added 3 commits January 11, 2021 14:52

adjust types

abf0e25

spread core on render function

edc7423

remove unnecessary items from MLJobLink test

8cf3403

justinkambic approved these changes Jan 11, 2021

View reviewed changes

update use_monitor_breadcrumbs test

1866c29

dominiqueclarke changed the title ~~Tests/uptime testing utils~~ [Uptime] Tests/uptime testing utils Jan 11, 2021

dominiqueclarke merged commit ca31bd8 into elastic:master Jan 12, 2021

dominiqueclarke deleted the tests/uptime-testing-utils branch January 12, 2021 01:02

dominiqueclarke mentioned this pull request Jan 12, 2021

[7.x] [Uptime] Tests/uptime testing utils (#87650) #87958

Merged

dominiqueclarke mentioned this pull request Jan 12, 2021

[7.11] [Uptime] Tests/uptime testing utils (#87650) #87959

Merged

justinkambic added the v7.12.0 label Jan 13, 2021


		// FLAKY: https://github.com/elastic/kibana/issues/85899
		describe.skip('ExecutedStep', () => {

	renderOptions?: any;
	renderOptions?: Omit<RenderOptions, 'queries'>;

[Uptime] Tests/uptime testing utils #87650

[Uptime] Tests/uptime testing utils #87650

Conversation

dominiqueclarke commented Jan 7, 2021 • edited Loading

Summary

elasticmachine commented Jan 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dominiqueclarke commented Jan 11, 2021

justinkambic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dominiqueclarke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinkambic left a comment

Choose a reason for hiding this comment

kibanamachine commented Jan 11, 2021

💚 Build Succeeded

Metrics [docs]

History

dominiqueclarke commented Jan 7, 2021 •

edited

Loading