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

Ui tests #4

Merged
merged 10 commits into from
Apr 28, 2020
20 changes: 20 additions & 0 deletions x-pack/plugins/enterprise_search/public/application.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
/*
Copy link
Owner

Choose a reason for hiding this comment

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

This should be renamed index.test.ts

Copy link
Owner

Choose a reason for hiding this comment

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

Ah, and should also be moved to public/applications/index.test.ts

Copy link
Author

Choose a reason for hiding this comment

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

* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import { coreMock } from 'src/core/public/mocks';
import { renderApp } from './applications';

describe('renderApp', () => {
it('mounts and unmounts UI', () => {
const params = coreMock.createAppMountParamters();
const core = coreMock.createStart();

const unmount = renderApp(core, params, {});
expect(params.element.querySelector('.setup-guide')).not.toBeNull();
unmount();
expect(params.element.innerHTML).toEqual('');
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import React from 'react';
import { mount } from 'enzyme';
Copy link
Author

Choose a reason for hiding this comment

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

I use mount over shallow in most tests for two reasons...

  1. Since we use EUI components it would require a lot of diving, which starts to make these tests a bit more brittle, because any UI tweak would also require cause us to adjust our dive chain. Using mount I just mount the whole thing and pick out interesting elements to test using data-test-subj.

  2. In order to execute useContext, we need to use mount, afaict.

I this also generally lets us mock less and test bigger unit chunks.

Copy link
Owner

@cee-chen cee-chen Apr 27, 2020

Choose a reason for hiding this comment

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

My wariness of using mount() over shallow() stems primarily from my past experiences writing hundreds of Jest tests for a large UI library. The primary dev impact I recall is one of speed - Jest tests are simply much faster (for obvious reasons) with shallow(). This only comes into play when we have a larger codebase with a lot of tests, which we don't have now - but we eventually will once AS moves fully over.

That being said, I accept that is a pre-optimization we don't have to worry about yet, that can easily wait until post-MVP, and you're right that useContext is far more annoying to do in shallow() vs. mount() (it's possible from my quick research, but it requires some extra setup that we're not doing).

I am slightly more concerned about point 1 - again, this is from past experience with writing tests with 3rd party libs (or even 1st party deps that we'd written ourselves) - but I tend to find that mount() tempts people into the trap of diving into a component that doesn't belong to us and that we shouldn't depend upon for tests. From a high level point of view for a unit test, all we care about is that a component named "EuiButton" rendered, and that the correct props were passed to it - not anything that's inside it (that belongs to the unit test for EuiButton itself, which EUI is in charge of).

For example, if we wrote something like wrapper.find('[data-test-subj="someStringSetByEui"]' - that's now fragile and causes failures if a 3rd party lib changes without warning. We didn't do so in this test, which is great, but I find that shallow() generally tends to discourage / makes it much harder to write code that relies on 3rd party deps by forcing devs to manually dive into components rather than just having everything readily available.

Sorry, all that was a lot of words :) TL;DR: I'm definitely not against moving forward with mount(), but especially with performance considerations down the road, I'd like us to be open to moving to shallow() as a tech debt item at some point. Does that sound good?

Copy link
Author

Choose a reason for hiding this comment

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

I 100% agree with you. I usually default to shallow, and if shallow won't work, I use mount.

One way I've handled the context thing in components is export two versions of a component, the outer component, which wraps an inner component to access context and pass it through as a simple property to the inner component. Then I unit test the InnerComponent.

export default OuterComponent = () => {
  const {prop} = useContext(...)
  return <InnerComponent prop={prop}
}

export const InnerComponent = ({prop}) => {
  <div>Hi, I am a {prop}</div>
}

Copy link
Author

Choose a reason for hiding this comment

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

That was pre-hooks, redux higher-order component days, things are probably different now though.


import { EngineOverviewHeader } from '../engine_overview_header';
import { KibanaContext } from '../../..';

describe('EngineOverviewHeader', () => {
let enterpriseSearchUrl;

afterEach(() => {
enterpriseSearchUrl = undefined;
});

const render = () => {
return mount(
<KibanaContext.Provider
value={{
http: {},
enterpriseSearchUrl,
setBreadcrumbs: jest.fn(),
}}
>
<EngineOverviewHeader />
</KibanaContext.Provider>
);
};
Copy link
Owner

@cee-chen cee-chen Apr 27, 2020

Choose a reason for hiding this comment

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

I'd like to pull this out to a reusable helper function if possible. Something like this:

const mountWithContext = (component, context) => {
  const defaultContext = {
    http: jest.fn(),
    setBreadcrumbs: jest.fn(),
    enterpriseSearchUrl: 'http://localhost:3002',
  };

  return mount(
    <KibanaContext.Provider value={{ ...defaultContext, ...context }}>
      {component}
    </KibanaContext.Provider>
  );
};

example usage:

const wrapper = mountWithContext(<EngineOverviewHeader />, { enterpriseSearchUrl: '' });

I'd strongly prefer mountWithContext() over render() - render() is already an existing OOTB function within Enzyme and one that most people find confusing vs shallow/mount and that I personally prefer to avoid.

My thought is that this could get exported from index.test.ts, but maybe helpers.test.js is better? 🤷 I'd normally put in a Jest setupFile but unfortunately since we're inheriting Jest settings from Kibana, I don't think we can make this a global helper without an import.

Copy link
Author

Choose a reason for hiding this comment

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

I like it.

Copy link
Author

Choose a reason for hiding this comment

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

I created a test_utils/helpers.tsx file, which seemed consistent with what Kibana does with other test helpers like their enzyme helper. a7b0acd


describe('when enterpriseSearchUrl is set', () => {
let wrapper;

beforeEach(() => {
enterpriseSearchUrl = 'http://localhost:3002';
wrapper = render();
});
Copy link
Owner

@cee-chen cee-chen Apr 27, 2020

Choose a reason for hiding this comment

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

I'd like to see us refactor everything within context to be a passed param rather than let strings.

As an example:

  const render = context => {
    return mount(
      <KibanaContext.Provider
        value={{
          http: {},
          enterpriseSearchUrl: '',
          setBreadcrumbs: jest.fn(),
          ...context,
        }}
      >
        <EngineOverviewHeader />
      </KibanaContext.Provider>
    );
  };


  describe('when enterpriseSearchUrl is set', () => {
    beforeEach(() => {
      wrapper = render({ enterpriseSearchUrl: 'http://localhost:3002' });
    });
  });

  describe('when enterpriseSearchUrl is not set', () => {
    beforeEach(() => {
      wrapper = render({ enterpriseSearchUrl: undefined });
    });
  });

Let me know how that feels to you?

Copy link
Owner

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

Feels good a7b0acd


describe('the Launch App Search button', () => {
const subject = () => wrapper.find('EuiButton[data-test-subj="launch_button"]');
Copy link
Owner

Choose a reason for hiding this comment

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

This is a very minor thing, but I would actually slightly prefer that we remain as library-agnostic as possible and instead just do wrapper.find('[data-test-subj="launch_button"]'). That way in the very unlikely scenario where we move away from EUI (or EUI changes their names?), we'll have less static strings to change, but primarily it's a best practice thing and one we also use in our E2E/Cypress tests.

As an alternative note - snake_casing is primarily for file/folder names in Kibana I believe, I believe we should be using camelCase for data attrs

Copy link
Author

Choose a reason for hiding this comment

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

wrapper.find('[data-test-subj="launch_button"]') actually finds two elements for some reason, I haven't worked out exactly why, because there is only one rendered element in the .html() output. 'EuiButton[data-test-subj="launch_button"]' was the only way I could properly select the element I was looking for. So this was basically a shortcut because I didn't feel like going any deeper into this problem right now. Happy to take a second look though.

Copy link
Owner

Choose a reason for hiding this comment

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

Interesting, I'm seeing the same thing. I think it's because EuiButton is passing data-test-subj down to as a prop to the raw <button> component, but the way Enzyme works, the prop is available on both the parent EuiButton component and the button child.

I'm fairly certain this wouldn't be an issue with shallow, so I'm good with leaving it for now and come back to it later when we've investigated using shallow more thoroughly.

If we could change to camelCasing though that would be dope 👍

Copy link
Author

Choose a reason for hiding this comment

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

Dope. 5f28efe


it('should not be disabled', () => {
expect(subject().props().isDisabled).toBeFalsy();
});

it('should use the enterpriseSearchUrl as the base path for its href', () => {
expect(subject().props().href).toBe('http://localhost:3002/as');
});
});
});

describe('when enterpriseSearchUrl is not set', () => {
JasonStoltz marked this conversation as resolved.
Show resolved Hide resolved
let wrapper;

beforeEach(() => {
enterpriseSearchUrl = undefined;
wrapper = render();
});

describe('the Launch App Search button', () => {
const subject = () => wrapper.find('EuiButton[data-test-subj="launch_button"]');

it('should be disabled', () => {
expect(subject().props().isDisabled).toBe(true);
});

it('should not have an href', () => {
expect(subject().props().href).toBeUndefined();
});
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ export const EngineOverviewHeader: React.FC<> = () => {
const buttonProps = {
fill: true,
iconType: 'popout',
['data-test-subj']: 'launch_button',
};
if (enterpriseSearchUrl) {
buttonProps.href = `${enterpriseSearchUrl}/as`;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,151 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import { appSearchBreadcrumbs, enterpriseSearchBreadcrumbs } from '../kibana_breadcrumbs';

jest.mock('../react_router_helpers', () => ({
letBrowserHandleEvent: () => false,
}));

describe('appSearchBreadcrumbs', () => {
const historyMock = {
createHref: jest.fn().mockImplementation(path => path.pathname),
push: jest.fn(),
};

const breadCrumbs = [
{
text: 'Page 1',
path: '/page1',
},
{
text: 'Page 2',
path: '/page2',
},
];

afterEach(() => {
jest.clearAllMocks();
});

const subject = () => appSearchBreadcrumbs(historyMock)(breadCrumbs);

it('Builds a chain of breadcrumbs with Enterprise Search and App Search at the root', () => {
JasonStoltz marked this conversation as resolved.
Show resolved Hide resolved
expect(subject()).toEqual([
{
href: '/',
onClick: expect.any(Function),
text: 'Enterprise Search',
},
{
href: '/app_search',
onClick: expect.any(Function),
text: 'App Search',
},
{
href: '/page1',
onClick: expect.any(Function),
text: 'Page 1',
},
{
href: '/page2',
onClick: expect.any(Function),
text: 'Page 2',
},
]);
});

describe('links', () => {
const eventMock = {
preventDefault: jest.fn(),
};

it('has a link to Enterprise Search Home page first', () => {
subject()[0].onClick(eventMock);
expect(historyMock.push).toHaveBeenCalledWith('/');
});

it('has a link to App Search second', () => {
subject()[1].onClick(eventMock);
expect(historyMock.push).toHaveBeenCalledWith('/app_search');
});

it('has a link to our custom page third', () => {
subject()[2].onClick(eventMock);
expect(historyMock.push).toHaveBeenCalledWith('/page1');
});

it('has a link to our second custom page last', () => {
subject()[3].onClick(eventMock);
expect(historyMock.push).toHaveBeenCalledWith('/page2');
});
});
});

describe('enterpriseSearchBreadcrumbs', () => {
const historyMock = {
createHref: jest.fn(),
push: jest.fn(),
};

const breadCrumbs = [
{
text: 'Page 1',
path: '/page1',
},
{
text: 'Page 2',
path: '/page2',
},
];

afterEach(() => {
jest.clearAllMocks();
});

const subject = () => enterpriseSearchBreadcrumbs(historyMock)(breadCrumbs);

it('Builds a chain of breadcrumbs with Enterprise Search at the root', () => {
expect(subject()).toEqual([
{
href: undefined,
onClick: expect.any(Function),
text: 'Enterprise Search',
},
{
href: undefined,
onClick: expect.any(Function),
text: 'Page 1',
},
{
href: undefined,
onClick: expect.any(Function),
text: 'Page 2',
},
]);
});

describe('links', () => {
const eventMock = {
preventDefault: jest.fn(),
};

it('has a link to Enterprise Search Home page first', () => {
subject()[0].onClick(eventMock);
expect(historyMock.push).toHaveBeenCalledWith('/');
});

it('has a link to our custom page third', () => {
Copy link

Choose a reason for hiding this comment

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

replace with: it('has a link to page 1 second')

Copy link
Author

Choose a reason for hiding this comment

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

Updated here: a7b0acd

subject()[1].onClick(eventMock);
expect(historyMock.push).toHaveBeenCalledWith('/page1');
});

it('has a link to our second custom page last', () => {
subject()[2].onClick(eventMock);
expect(historyMock.push).toHaveBeenCalledWith('/page2');
});
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import React from 'react';
import { SetAppSearchBreadcrumbs } from '../kibana_breadcrumbs';
import { mount } from 'enzyme';

jest.mock('./generate_breadcrumbs', () => ({
appSearchBreadcrumbs: jest.fn(),
}));
import { appSearchBreadcrumbs } from './generate_breadcrumbs';

jest.mock('react-router-dom', () => ({
useHistory: () => ({
createHref: jest.fn(),
push: jest.fn(),
location: {
pathname: '/current-path',
},
}),
}));

import { KibanaContext } from '../..';
JasonStoltz marked this conversation as resolved.
Show resolved Hide resolved

describe('SetAppSearchBreadcrumbs', () => {
const setBreadcrumbs = jest.fn();
const builtBreadcrumbs = [];
const appSearchBreadCrumbsInnerCall = jest.fn().mockReturnValue(builtBreadcrumbs);
const appSearchBreadCrumbsOuterCall = jest.fn().mockReturnValue(appSearchBreadCrumbsInnerCall);
appSearchBreadcrumbs.mockImplementation(appSearchBreadCrumbsOuterCall);

afterEach(() => {
jest.clearAllMocks();
});

const render = props => {
return mount(
<KibanaContext.Provider
value={{
http: {},
enterpriseSearchUrl: 'http://localhost:3002',
setBreadcrumbs,
}}
>
<SetAppSearchBreadcrumbs {...props} />
</KibanaContext.Provider>
);
};

describe('when isRoot is false', () => {
const subject = () => render({ text: 'Page 1', isRoot: false });

it('calls appSearchBreadcrumbs to build breadcrumbs, then registers them with Kibana', () => {
subject();

// calls appSearchBreadcrumbs to build breadcrumbs with the target page and current location
expect(appSearchBreadCrumbsInnerCall).toHaveBeenCalledWith([
{ text: 'Page 1', path: '/current-path' },
]);

// then registers them with Kibana
expect(setBreadcrumbs).toHaveBeenCalledWith(builtBreadcrumbs);
});
});

describe('when isRoot is true', () => {
JasonStoltz marked this conversation as resolved.
Show resolved Hide resolved
const subject = () => render({ text: 'Page 1', isRoot: true });

it('calls appSearchBreadcrumbs to build breadcrumbs with an empty breadcrumb, then registers them with Kibana', () => {
subject();

// uses an empty bredcrumb
expect(appSearchBreadCrumbsInnerCall).toHaveBeenCalledWith([]);

// then registers them with Kibana
expect(setBreadcrumbs).toHaveBeenCalledWith(builtBreadcrumbs);
});
});
});
Loading