[Watcher] Retain search and pagination values when watch list refreshes

[alisonelizabeth]

Fixes #69606

How to test

1. Create >10 watches with a variety of names.
2. Search for one of the watches. Wait one minute, check that another request was made to refresh the watch list, and verify the search input value along with the filtered results persists.
3. Navigate to page 2 of the results. Wait one minute, check that another request was made to refresh the watch list, and verify you are still on page 2.
4. Change the default page size. Wait one minute, check that another request was made to refresh the watch list, and verify the page size selected persists.

Release note

This fixes a bug in the Watcher UI where the search input cleared after 1 minute of inactivity, causing a user to lose the filtered results. It also fixes a similar issue where if the user changed pages or the default page size and was inactive.

[alisonelizabeth]

The search box in the table doesn't appear to be able to accept a data-test-subj prop, so we have to access it via a CSS selector in the tests. In order to use a CSS selector, you need to first target the closest test subject.

[elasticmachine]

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

[cjcenizal]

Tested locally, code LGTM! Just had a few suggestions.

[cjcenizal]

It looks like each of these tests calls setup on its own, so I think we can remove this entire beforeEach.

[cjcenizal]

Minor DX suggestion. How about abstracting this behind a helper function in watch_list.helpers.ts?

  const searchWatches = (term) => {
    const { find, component } = testBed;
    const searchInput = find('watchesTableContainer').find('.euiFieldSearch');

    // Enter input into the search box
    // @ts-ignore
    searchInput.instance().value = term;
    searchInput.simulate('keyup', { key: 'Enter', keyCode: 13, which: 13 });

    component.update();
  };

  return {
    ...testBed,
    actions: {
      selectWatchAt,
      clickWatchAt,
      clickWatchActionAt,
      searchWatches,
    },
  };

Then here we can call it like this:

        test('should retain the search query', async () => {
          const { find, component, table, actions: { searchWatches } } = testBed;
          searchWatches(watch1.name);

          const { tableCellsValues } = table.getMetaData('watchesTable');

[cjcenizal]

I find it really difficult to understand why we call getExpectedValue in these assertions, and difficult to understand what we expect the value to be. After reading the fixture call site code, the fixture module code, and the getExpectedValue code, I understand that on this line the name will include a random value so we can't hardcode a string here. But why would it ever be undefined? And for the watchStatus properties, it looks like the shape is always { state: 'OK', isActive: true }, so why do we check if those two properties are undefined? Similarly, the other properties are always undefined so I'm confused about why we check this.

It smells a bit like we're testing implementation if getExpectedValue mirrors the implementation of how the table renders undefined values as empty spaces, but without understanding the intention here it's difficult for me to say. My initial reaction based on what I see here would be to suggest we remove the random values from the mocked data and assert against explicit values. If we find it unclear about what's being shown in the table, we can add comments.

          expect(row).toEqual([
            '', // checkbox
            'a-id', // ID
            'a-name', // name
            'OK', // status
            '', // comment
            '', // lastMetCondition
            '', // lastChecked
            '', // actions
          ]);

[alisonelizabeth]

Good point. I think I may have copied this from elsewhere. I agree it's not very clear. I updated the assertion to your suggestion.

[cjcenizal]

Similar DX suggestion here about a helper function. We can add this to watch_list.helpers.ts:

  const advanceTimeToTableRefresh = async () => {
    const { component } = testBed;
    await act(async () => {
      // Advance timers to simulate another request
      jest.advanceTimersByTime(REFRESH_INTERVALS.WATCH_LIST);
    });
    component.update();
  };

  return {
    ...testBed,
    actions: {
      selectWatchAt,
      clickWatchAt,
      clickWatchActionAt,
      advanceTimeToTableRefresh,
    },
  };

And use it here:

await advanceTimeToTableRefresh();

[cjcenizal]

Is this one needed? I was able to remove it and the test passed.

[alisonelizabeth]

Thanks for the review @cjcenizal! I updated the tests with your suggestions.

[kibanamachine]

💚 Build Succeeded

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

Metrics [docs]

async chunks size

id	before	after	diff
watcher	953.4KB	955.2KB	+1.8KB

History

• 💚 Build #85954 succeeded a5bd56e
• 💔 Build #85916 failed ee8410c

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

[sparrowt]

(Belated) thank you for fixing this! Genuinely annoying enough to make me want to upgrade... ;-)