Ui tests

[JasonStoltz]

Pushing up some more unit tests.

Instead of starting at the top and working down like last time, I'm starting at the bottom and working up.

There's more to come, but just wanted to get some out there.

[JasonStoltz]

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.

[cee-chen]

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?

[JasonStoltz]

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>
}

[JasonStoltz]

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

[richkuz]

None of my feedback is blocking, just adding to the discussion.

[richkuz]

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

[JasonStoltz]

Updated here: a7b0acd

[cee-chen]

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?

[cee-chen]

Final form here: https://github.com/constancecchen/kibana/pull/4/files#r415998493

[JasonStoltz]

Feels good a7b0acd

[cee-chen]

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

[JasonStoltz]

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.

[cee-chen]

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 👍

[JasonStoltz]

Dope. 5f28efe

[cee-chen]

This should be renamed index.test.ts

[cee-chen]

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

[JasonStoltz]

a7b0acd

[cee-chen]

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.

[JasonStoltz]

I like it.

[JasonStoltz]

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

[JasonStoltz]

@constancecchen Makes sense, updated here: 8a678a0.

[cee-chen]

🎉