Reduce duplication in ui framework for jest tests

[stacey-gammon]

A lot of our ui framework react components have nearly the same exact basic jest test, where the only thing that changes is a single word - the name of the component.

For example, we have about 50 files of the format:

import React from 'react';
import { render } from 'enzyme';
import { requiredProps } from '../../test/required_props';

import {
  KuiReactComponent,
} from './react_component';

test('renders KuiReactComponent', () => {
  const component = <KuiReactComponent { ...requiredProps }>children</KuiReactComponent>;
  expect(render(component)).toMatchSnapshot();
});

You can replace KuiReactComponent with about 50 different react components and each has a single test file with this single test in it.

If we ever want to change this test ever so slightly, we're looking at updating many files.

I explored a refactoring awhile back that didn't garner full support - #10821 (comment)

But perhaps we can come up with a way to reduce the duplication that is supported by all interested react parties.

cc @kjbekkelund @weltenwort @cjcenizal

[cjcenizal]

At my last job, we had tests to ensure every component we created was exported from the root index file, which is a similar requirement as our "required props" requirement because it applied to every component we had. Here's the test we used.

We could do something similar where we create a specific test file which imports every component, stores each one in an array, and then loops through the array and applies the "required props" assertions.

This file could be components/index.test.js or it could be a special required_props.test.js file (perhaps inside of a components/common directory or something to move it out of the root).

[kimjoar]

++ when the file contains nothing else than that single test I agree — it would be nice to just create an array or something and loop over and do the snapshot tests. When there are more tests I think it should stay in the test file.

[cjcenizal]

@kjbekkelund If you had to choose one or other, which would you prefer? Whichever we choose, I'd like to be consistent. I want to avoid a scenario in which someone needs to add a new test, except the only test that currently exists is the "required props" one in required_props.test.js. Then s/he has to create the new test file, extract the "required props" test, and add the new test. What should be a task that involves a single file ends up involving two.

[kimjoar]

I prefer doing what we already do. It's just a couple lines and we're consistent across React components. If we need to change 50 of them later I'd just jscodeshift it, if it's not otherwise simple to search/replace the change.

[weltenwort]

Maybe we can achieve both (keeping the test in the file and reducing boilerplate) by creating reusable test factories such as

export const testRequiredProps(ComponentUnderTest, extraProps = {}) => {
  test('renders required properties', () => {
    const component = (
      <ComponentUnderTest
        { ...extraProps }
        { ...requiredProps }
      >
        children
      </ComponentUnderTest>
    );
    expect(render(component)).toMatchSnapshot();
  });
};

This could then be used in the individual test files like this:

import { testRequiredProps } from '../../test/required_props';

import { KuiReactComponent } from './react_component';


describe('component KuiReactComponent', () => {
  testRequiredProps(KuiReactComponent);
  
  test('does something fancy', () => {
    // ...
  });
});

[weltenwort]

One caveat comes to mind though: I don't know where jest will put the snapshots when the test creation is factored out like this. But that should be easy to find out.

[kimjoar]

Not "ideal", but I'm okey with it. I'd prefer if there was no extraProps or things like that (I think it decreases readability). I prefer just purely wrapping these 4 lines into a helper (and extract KuiReactComponent of course).

test('renders KuiReactComponent', () => {
  const component = <KuiReactComponent { ...requiredProps }>children</KuiReactComponent>;
  expect(render(component)).toMatchSnapshot();
});

[weltenwort]

I added the extraProps because there might be required props defined on the component.

[weltenwort]

Too many similar terms, sorry. I meant to say that there might be props that are marked as isRequired.

[cjcenizal]

Closing this after talking with @stacey-gammon.

[adamchenwei]

@cjcenizal Closing this after talking with @stacey-gammon. what does it mean...

[cjcenizal]

@adamchenwei I don't recall the specifics of our conversation, but from the comment and context I have to assume that I discussed this with the original author of the issue (Stacey Gammon) and we decided to not to act on this issue and instead focus on other work.

[samking314]

One caveat comes to mind though: I don't know where jest will put the snapshots when the test creation is factored out like this. But that should be easy to find out.

@weltenwort It's actually still located in the same directory as the test file that's calling it