[Enterprise Search] Automatically mock shared logic files

[cee-chen]

Summary

Part 2 of my Jest cleanup: I've mentioned recently after seeing a ton of jest.mock('../../../shared/kibana') (or http, or flash_messages, etc.) cruft at the top of our logic files that it would be really nice to clean up / DRY out said mocks. We already do this for our React component tests via mocking useValues and useActions, but we didn't have this for our actual Kea logic tests that directly connected/grabbed values from other files (e.g. SomeLogic.value.someValue).

Well, I was yesterday years old when I learned that jest.mock() is relative to the test file, not to the actual file being tested... meaning that we can definitely DRY out mocking our logic. I decided to accomplish this by directly mocking each shared logic file in its __mocks__/*_logic.mock.ts (see 93b7242). This means that importing any mocks from our __mocks__ folder will automatically mock the logic file for you. Check out the following code diff example:

jest.mock('../../../shared/kibana', () => ({
  KibanaLogic: { values: { history: { location: { search: '' } } } },
}));
import { KibanaLogic } from '../../../shared/kibana';

jest.mock('../../../shared/http', () => ({
  HttpLogic: { values: { http: { get: jest.fn() } } },
}));
import { HttpLogic } from '../../../shared/http';

jest.mock('../../../shared/flash_messages', () => ({
  flashAPIErrors: jest.fn(),
}));
import { flashAPIErrors } from '../../../shared/flash_messages';

becomes:

import { mockKibanaValues, mockHttpValues, mockFlashMessageHelpers } from '../../../__mocks__';

const { history } = mockKibanaValues;
const { http } = mockHttpValues;
const { flashAPIErrors } = mockFlashMessageHelpers;

The latter is hopefully much cleaner and nicer to use - we shouldn't have to manually mock XLogic.values or YLogic.actions in the future, we just have to pull the existing mocked actions/values from our mock helpers. 🎉

Following along

I recommend following this PR by-commit since it touches so many different files.

A warning about this approach

It's a fairly "hungry" change. I wasn't super familiar with how imports worked previously (I thought you had to statically import the entire file to get the jest.mock() functionality), but I'm definitely more familiar now. So here's the warning:

⚠️ Any time that you import anything from the __mocks__ index folder, you are now automatically importing all the mocks for every single one of these shared logic files. ⚠️

For the most part, this shouldn't be a huge deal. Mocks are a way of life in unit testing unless we are attempting to unit test the logic file itself, in which case we have a few options:

• Work around importing all mocks by importing only a very specific file within __mocks__, e.g. import specifically __mocks__/http_logic.mock which still retains the Kibana logic file as-is for example.
• Import all the __mocks__, but retain importing the actual file by importing from the *_logic.ts file directly - e.g., import { HttpLogic } from 'shared/http/http_logic instead of shared/http (since our mocks are mocking the index.ts obj, not the actual files).

See b49b877 as an example of the folder most affected by this hungry mocking - because some of these files do require pulling from the __mocks__ folder to mock other logic/kea/etc, I had to change all test files to import the code they were testing directly from their source files to fix the tests.

Again, to reiterate, this should mostly only affect shared logic files, and I don't anticipate us adding a whole lot more of those. I think the tradeoff there (some extra consideration) vs 90% of the rest of the app gaining nicer dev QOL is worth it.

Checklist

• Unit or functional tests were updated or added to match the most common scenarios

[cee-chen]

I decided to reorganize our tests a bit in the light of all these new fancy helpers. Here's the basic architecture I'm thinking:

/**
 * License header
 */

// Mocks go here
import { LogicMounter, mockHttpValues } from '../../__mocks__';

// 3rd party imports go here
import React from 'react';
import { shallow } from 'enzyme';

// 1st party shared/lib/helper imports go here
import { someHelper } from '../../../shared/helper';

// 1st party specific imports go here
import { SomeComponent } from '../';
import { SomeLogic } from './';

describe('SomeLogic', () => {
  // Destructuring from mocks go here - e.g., `mount` from LogicMounter
  const { mount, unmount } = new LogicMounter(SomeLogic);
  const { http } = mockHttpValues;

  // Constants (e.g.) values, actions go here
  const DEFAULT_VALUES = { ... };
  const foo = 'bar';

  // Jest before/after/all blocks
  beforeEach(() => {
    // ...
  });

  // Start the tests
  it('does something', () => {});
});

LMK what you think!

[byronhulcher]

I like this, let's make sure it's in a README file somewhere in this codebase.

[cee-chen]

Hm you think so? Maybe I'm overly optimistic, but I'm hoping that we follow this by osmosis - I figure someone opening the codebase for the first time will be like "Oh all the unit tests are written this way, I'll just follow it/copy this file".

My only hesitation with putting this in our README is if it falls out of date, etc. That being said there's definitely a testing section in our main README, so if other folks are +1 to this I can definitely add it there!

[JasonStoltz]

I think I agree with ya there constance.

[byronhulcher]

This looks good to me. I left a non-blocking comment about the value of allowing the import { mockKibanaValues, mockHttpValues, mockFlashMessageHelpers } from '../../../__mocks__'; style import compared to the cost of debugging its side-effects in the future. Even if we decide to go in that direction we can handle it in a follow-up PR, it does not need to block the work already done here to improve our tests.

[byronhulcher]

I've never really understood the value of creating these kinds of index.ts files for bundling unrelated code in sub-directories. That this introduces side-effects (if you import anything from this file everything gets mocked) seems REALLY weird, and is not something I would expect to encounter, and I imagine would be gnarly to debug. You mention a solution is to import these mocks individually in files, which you do for a couple of tests. Maybe we should handle all mock imports that way? Is there an upside to this I'm not seeing that's worth the side-effect?

[cee-chen]

Ya it's definitely an architectural decision. As with everything it's all about dev tradeoffs, in my personal opinion I do strongly feel that it's nice to not have to dive deeply into folders - when I'm outside that folder but reaching into it from another component, it's very nice to just have everything available from the top-level index.ts file and not to have 3 different lines of imports for 3 different files.

It's also one that Kibana uses everywhere (example file) so I personally feel confident following them in this area. Should they ever change this (which I imagine would primarily be due to performance reasons, tree shaking, etc.), I would definitely be +1 to following suit.

I will also note that the side effects are limited to tests/jest.mock() behavior, whereas the benefits of quicker and combined imports apply outside those and to our source code as well (the main benefit IMO). For me at least the cost is well worth it once imports are understood - they were only unexpected to me because I didn't really understand how they worked. I strongly also think we'll find any side effects do not negatively impact like 95+% of our final code base.

[byronhulcher]

I like this, let's make sure it's in a README file somewhere in this codebase.

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: ecc0148

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
enterpriseSearch	1.8MB	1.8MB	-95.0B

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

[scottybollinger]

This is fantastic! Always great to see my personal todos done in someone else's PR 😂

Thanks Constance!

[JasonStoltz]

So clean and nice. 100 ✋ high-five.

[cee-chen]

Thanks y'all! I'll go ahead and merge this for now without a README update, but if y'all would like one, def feel free to open a PR or bug me about it later as a follow-up PR!