Refactor index management client integration tests for scalability

[cjcenizal]

The biggest change I made in this PR was to extract the index templates tab tests into their own file. I think this increases scalability because we can test additional tabs by adding additional files.

I also think the general pattern we have for unit tests is to colocate them with tested code, so I updated the flatten_object tests to follow that pattern.

Another small change was to remove unused mocks.

[elasticmachine]

Pinging @elastic/es-ui (Team:Elasticsearch UI)

[alisonelizabeth]

Thanks for working on this @cjcenizal! I think this extraction makes sense.

What do you think about taking it a step further? For example, I could see only testing the IndexTemplates tab component, rather than mounting the home component within the home_index_templates_tab.test.ts. That could also eliminate some of the setup code (for example, here and here). Similarly, I could also see having an "indices tab" test that could further extract some of the tests in home.test.ts.

This might be outside the scope of what you wanted to do here though 😄

[cjcenizal]

@alisonelizabeth Great suggestion! Can you take another look? I extracted out these tests into their own files. Note that I couldn't consume the TemplateList component because we depend upon react router to tell us which template to open in the detail panel, and the Home component contains the enclosing Route component. Maybe in a future iteration we can solve this. I left a code comment along these lines.

I also decided to extract and colocate the helpers with the tests that consume them. This significantly deflated the shared helpers directory. I think this makes it easier to understand which tests use which helpers. Let me know what you think.

[cjcenizal]

Also, I was able to reproduce a flaky test at one point. Of course, it was flaky so it happened once and I wasn't able to reproduce it again.

[sebelga]

LGTM! I left a few comments in the code for info. Thanks for doing this 👍 !

[sebelga]

Something around those lines should work

const CompToTest = () => {
  return (
    <Switch>
      <Route exact path="templates/:templateName*" component={TemplateList} />
     </Switch>
  );
};

const testBedConfig: TestBedConfig = {
  memoryRouter: {
    initialEntries: ["templates"],
    componentRoutePath: "templates", // or maybe an empty ""?
  },
  doMountAsync: true,
};

const setup = registerTestBed(WithAppDependencies(CompToTest), testBedConfig);

[cjcenizal]

Thanks, Seb! I took a stab at this and every test started failing. I'm going to merge this in its current state to enable us to add tests for data streams etc. using this new organization, with the expectation that we'll address your comments separately.

[sebelga]

info: This block is exactly what we want to remove everywhere. It creates latency in the tests and could be the cause of flakiness.

It should be written

act(() => {
  // this is causing states updates. There can only be **one** in each act() call
  // It should be synchronous if possible.
  // Sometimes hook update require async, so if there is an error in console --> use async act()
  actions.selectHomeTab('templatesTab');
});

// If I need a second action straightaway, we use another act()
act(() => {
  actions.doSomethingElse();
});

// And we only need to call update() once after all actions
component.update();

[sebelga]

As an alternative, we could wrap the act() inside our helpers' actions and keep the test file cleaner.

[alisonelizabeth]

Latest changes LGTM. Thanks @cjcenizal!

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 23f6e0e

History

• 💔 Build #51561 failed 4bf9f62
• 💚 Build #51253 succeeded 7945f89
• 💚 Build #50981 succeeded 4c78128
• 💔 Build #50964 failed 710090c

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