Merge pull request #3 from dimaanj/improve-tests

[Discover] Improve tests

src/plugins/discover/public/application/main/components/layout/use_discover_histogram.ts

@@ -25,6 +25,7 @@ import type { AppState, GetStateReturn } from '../../services/discover_state';
 import { FetchStatus } from '../../../types';
 import type { DiscoverSearchSessionManager } from '../../services/discover_search_session';
 import type { InspectorAdapters } from '../../hooks/use_inspector';
+import { sendErrorTo } from '../../utils/fetch_all';
 
 export const CHART_HIDDEN_KEY = 'discover:chartHidden';
 export const HISTOGRAM_HEIGHT_KEY = 'discover:histogramHeight';
@@ -51,7 +52,7 @@ export const useDiscoverHistogram = ({
   inspectorAdapters: InspectorAdapters;
   searchSessionManager: DiscoverSearchSessionManager;
 }) => {
-  const { storage } = useDiscoverServices();
+  const { storage, data } = useDiscoverServices();
 
   /**
    * Visualize
@@ -133,10 +134,20 @@ export const useDiscoverHistogram = ({
    * Total hits
    */
 
+  const sendTotalHitsError = useMemo(
+    () => sendErrorTo(data, savedSearchData$.totalHits$),
+    [data, savedSearchData$.totalHits$]
+  );
+
   const onTotalHitsChange = useCallback(
-    (status: UnifiedHistogramFetchStatus, totalHits?: number) => {
+    (status: UnifiedHistogramFetchStatus, result?: number | Error) => {
       const { fetchStatus, recordRawType } = savedSearchData$.totalHits$.getValue();
 
+      if (result instanceof Error) {
+        sendTotalHitsError(result);
+        return;
+      }
+
       // If we have a partial result already, we don't
       // want to update the total hits back to loading
       if (fetchStatus === FetchStatus.PARTIAL && status === UnifiedHistogramFetchStatus.loading) {
@@ -145,11 +156,11 @@ export const useDiscoverHistogram = ({
 
       savedSearchData$.totalHits$.next({
         fetchStatus: status.toString() as FetchStatus,
-        result: totalHits,
+        result,
         recordRawType,
       });
     },
-    [savedSearchData$.totalHits$]
+    [savedSearchData$.totalHits$, sendTotalHitsError]
   );
 
   const { fetchStatus: hitsFetchStatus, result: hitsTotal } = useDataState(

src/plugins/discover/public/application/main/utils/fetch_all.test.ts

@@ -20,12 +20,12 @@ import {
   DataDocumentsMsg,
   DataMainMsg,
   DataTotalHitsMsg,
+  RecordRawType,
   SavedSearchData,
 } from '../hooks/use_saved_search';
 
 import { fetchDocuments } from './fetch_documents';
 import { fetchSql } from './fetch_sql';
-import { fetchTotalHits } from './fetch_total_hits';
 import { buildDataTableRecord } from '../../../utils/build_data_record';
 import { dataViewMock } from '../../../__mocks__/data_view';
 
@@ -37,12 +37,7 @@ jest.mock('./fetch_sql', () => ({
   fetchSql: jest.fn().mockResolvedValue([]),
 }));
 
-jest.mock('./fetch_total_hits', () => ({
-  fetchTotalHits: jest.fn(),
-}));
-
 const mockFetchDocuments = fetchDocuments as unknown as jest.MockedFunction<typeof fetchDocuments>;
-const mockFetchTotalHits = fetchTotalHits as unknown as jest.MockedFunction<typeof fetchTotalHits>;
 const mockFetchSQL = fetchSql as unknown as jest.MockedFunction<typeof fetchSql>;
 
 function subjectCollector<T>(subject: Subject<T>): () => Promise<T[]> {
@@ -88,7 +83,6 @@ describe('test fetchAll', () => {
 
     mockFetchDocuments.mockReset().mockResolvedValue([]);
     mockFetchSQL.mockReset().mockResolvedValue([]);
-    mockFetchTotalHits.mockReset().mockResolvedValue(42);
   });
 
   test('changes of fetchStatus when starting with FetchStatus.UNINITIALIZED', async () => {
@@ -135,8 +129,12 @@ describe('test fetchAll', () => {
     const documents = hits.map((hit) => buildDataTableRecord(hit, dataViewMock));
     mockFetchDocuments.mockResolvedValue(documents);
 
-    mockFetchTotalHits.mockResolvedValue(42);
     await fetchAll(subjects, searchSource, false, deps);
+    subjects.totalHits$.next({
+      fetchStatus: FetchStatus.COMPLETE,
+      recordRawType: RecordRawType.DOCUMENT,
+      result: 42,
+    });
     expect(await collect()).toEqual([
       { fetchStatus: FetchStatus.UNINITIALIZED },
       { fetchStatus: FetchStatus.LOADING, recordRawType: 'document' },
@@ -149,24 +147,36 @@ describe('test fetchAll', () => {
     const collect = subjectCollector(subjects.totalHits$);
     searchSource.getField('index')!.isTimeBased = () => true;
     await fetchAll(subjects, searchSource, false, deps);
+
+    subjects.totalHits$.next({
+      fetchStatus: FetchStatus.COMPLETE,
+      recordRawType: RecordRawType.DOCUMENT,
+      result: 32,
+    });
+
     expect(await collect()).toEqual([
       { fetchStatus: FetchStatus.UNINITIALIZED },
       { fetchStatus: FetchStatus.LOADING, recordRawType: 'document' },
       { fetchStatus: FetchStatus.PARTIAL, recordRawType: 'document', result: 0 }, // From documents query
       { fetchStatus: FetchStatus.COMPLETE, recordRawType: 'document', result: 32 },
     ]);
-    expect(mockFetchTotalHits).not.toHaveBeenCalled();
   });
 
   test('should only fail totalHits$ query not main$ for error from that query', async () => {
     const collectTotalHits = subjectCollector(subjects.totalHits$);
     const collectMain = subjectCollector(subjects.main$);
     searchSource.getField('index')!.isTimeBased = () => false;
-    mockFetchTotalHits.mockRejectedValue({ msg: 'Oh noes!' });
     const hits = [{ _id: '1', _index: 'logs' }];
     const documents = hits.map((hit) => buildDataTableRecord(hit, dataViewMock));
     mockFetchDocuments.mockResolvedValue(documents);
     await fetchAll(subjects, searchSource, false, deps);
+
+    subjects.totalHits$.next({
+      fetchStatus: FetchStatus.ERROR,
+      recordRawType: RecordRawType.DOCUMENT,
+      error: { msg: 'Oh noes!' } as unknown as Error,
+    });
+
     expect(await collectTotalHits()).toEqual([
       { fetchStatus: FetchStatus.UNINITIALIZED },
       { fetchStatus: FetchStatus.LOADING, recordRawType: 'document' },

src/plugins/discover/public/application/main/utils/fetch_all.ts

@@ -43,6 +43,25 @@ export interface FetchDeps {
   useNewFieldsApi: boolean;
 }
 
+/**
+ * Method to create an error handler that will forward the received error
+ * to the specified subjects. It will ignore AbortErrors and will use the data
+ * plugin to show a toast for the error (e.g. allowing better insights into shard failures).
+ */
+export const sendErrorTo = (
+  data: DataPublicPluginStart,
+  ...errorSubjects: Array<DataMain$ | DataDocuments$>
+) => {
+  return (error: Error) => {
+    if (error instanceof Error && error.name === 'AbortError') {
+      return;
+    }
+
+    data.search.showError(error);
+    errorSubjects.forEach((subject) => sendErrorMsg(subject, error));
+  };
+};
+
 /**
  * This function starts fetching all required queries in Discover. This will be the query to load the individual
  * documents as well as any other requests that might be required to load the main view.
@@ -58,22 +77,6 @@ export function fetchAll(
 ): Promise<void> {
   const { initialFetchStatus, appStateContainer, services, useNewFieldsApi, data } = fetchDeps;
 
-  /**
-   * Method to create an error handler that will forward the received error
-   * to the specified subjects. It will ignore AbortErrors and will use the data
-   * plugin to show a toast for the error (e.g. allowing better insights into shard failures).
-   */
-  const sendErrorTo = (...errorSubjects: Array<DataMain$ | DataDocuments$>) => {
-    return (error: Error) => {
-      if (error instanceof Error && error.name === 'AbortError') {
-        return;
-      }
-
-      data.search.showError(error);
-      errorSubjects.forEach((subject) => sendErrorMsg(subject, error));
-    };
-  };
-
   try {
     const dataView = searchSource.getField('index')!;
     if (reset) {
@@ -145,7 +148,7 @@ export function fetchAll(
       // Only the document query should send its errors to main$, to cause the full Discover app
       // to get into an error state. The other queries will not cause all of Discover to error out
       // but their errors will be shown in-place (e.g. of the chart).
-      .catch(sendErrorTo(dataSubjects.documents$, dataSubjects.main$));
+      .catch(sendErrorTo(data, dataSubjects.documents$, dataSubjects.main$));
 
     // Return a promise that will resolve once all the requests have finished or failed
     return documents.then(() => {

src/plugins/saved_search/public/services/saved_searches/get_saved_searches.test.ts

@@ -100,6 +100,7 @@ describe('getSavedSearch', () => {
     expect(savedObjectsClient.resolve).toHaveBeenCalled();
     expect(savedSearch).toMatchInlineSnapshot(`
       Object {
+        "breakdownField": undefined,
         "columns": Array [
           "_source",
         ],
@@ -197,6 +198,7 @@ describe('getSavedSearch', () => {
     expect(savedObjectsClient.resolve).toHaveBeenCalled();
     expect(savedSearch).toMatchInlineSnapshot(`
       Object {
+        "breakdownField": undefined,
         "columns": Array [
           "_source",
         ],

src/plugins/unified_histogram/public/chart/breakdown_field_selector.tsx

@@ -52,6 +52,7 @@ export const BreakdownFieldSelector = ({
 
   return (
     <EuiComboBox
+      data-test-subj="unifiedHistogramBreakdownFieldSelector"
       prepend={i18n.translate('unifiedHistogram.breakdownFieldSelectorLabel', {
         defaultMessage: 'Break down by',
       })}

src/plugins/unified_histogram/public/chart/chart.tsx

@@ -52,7 +52,7 @@ export interface ChartProps {
   onChartHiddenChange?: (chartHidden: boolean) => void;
   onTimeIntervalChange?: (timeInterval: string) => void;
   onBreakdownFieldChange?: (breakdownField: DataViewField | undefined) => void;
-  onTotalHitsChange?: (status: UnifiedHistogramFetchStatus, totalHits?: number) => void;
+  onTotalHitsChange?: (status: UnifiedHistogramFetchStatus, result?: number | Error) => void;
   onChartLoad?: (event: UnifiedHistogramChartLoadEvent) => void;
 }
 

src/plugins/unified_histogram/public/chart/histogram.tsx

@@ -40,7 +40,7 @@ export interface HistogramProps {
   filters: Filter[];
   query: Query | AggregateQuery;
   timeRange: TimeRange;
-  onTotalHitsChange?: (status: UnifiedHistogramFetchStatus, totalHits?: number) => void;
+  onTotalHitsChange?: (status: UnifiedHistogramFetchStatus, result?: number | Error) => void;
   onChartLoad?: (event: UnifiedHistogramChartLoadEvent) => void;
 }
 

src/plugins/unified_histogram/public/chart/use_total_hits.ts

@@ -12,7 +12,7 @@ import type { AggregateQuery, Filter, Query, TimeRange } from '@kbn/es-query';
 import { i18n } from '@kbn/i18n';
 import { cloneDeep, isEqual } from 'lodash';
 import { MutableRefObject, useEffect, useRef } from 'react';
-import { filter, lastValueFrom, map } from 'rxjs';
+import { catchError, filter, lastValueFrom, map, of } from 'rxjs';
 import {
   UnifiedHistogramFetchStatus,
   UnifiedHistogramHitsContext,
@@ -41,7 +41,7 @@ export const useTotalHits = ({
   filters: Filter[];
   query: Query | AggregateQuery;
   timeRange: TimeRange;
-  onTotalHitsChange?: (status: UnifiedHistogramFetchStatus, totalHits?: number) => void;
+  onTotalHitsChange?: (status: UnifiedHistogramFetchStatus, result?: number | Error) => void;
 }) => {
   const abortController = useRef<AbortController>();
   const totalHitsDeps = useRef<ReturnType<typeof getTotalHitsDeps>>();
@@ -150,7 +150,7 @@ const fetchTotalHits = async ({
   filters: Filter[];
   query: Query | AggregateQuery;
   timeRange: TimeRange;
-  onTotalHitsChange?: (status: UnifiedHistogramFetchStatus, totalHits?: number) => void;
+  onTotalHitsChange?: (status: UnifiedHistogramFetchStatus, result?: number | Error) => void;
 }) => {
   abortController.current?.abort();
   abortController.current = undefined;
@@ -217,10 +217,15 @@ const fetchTotalHits = async ({
     })
     .pipe(
       filter((res) => isCompleteResponse(res)),
-      map((res) => res.rawResponse.hits.total as number)
+      map((res) => res.rawResponse.hits.total as number),
+      catchError((error: Error) => of(error))
     );
 
-  const totalHits = await lastValueFrom(fetch$);
+  const result = await lastValueFrom(fetch$);
 
-  onTotalHitsChange?.(UnifiedHistogramFetchStatus.complete, totalHits);
+  const resultStatus =
+    result instanceof Error
+      ? UnifiedHistogramFetchStatus.error
+      : UnifiedHistogramFetchStatus.complete;
+  onTotalHitsChange?.(resultStatus, result);
 };

src/plugins/unified_histogram/public/layout/layout.tsx

@@ -84,7 +84,7 @@ export interface UnifiedHistogramLayoutProps extends PropsWithChildren<unknown>
    * Callback to update the total hits -- should set {@link UnifiedHistogramHitsContext.status} to status
    * and {@link UnifiedHistogramHitsContext.total} to totalHits
    */
-  onTotalHitsChange?: (status: UnifiedHistogramFetchStatus, totalHits?: number) => void;
+  onTotalHitsChange?: (status: UnifiedHistogramFetchStatus, result?: number | Error) => void;
   /**
    * Called when the histogram loading status changes
    */

test/functional/apps/discover/group1/_discover.ts

@@ -18,7 +18,6 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
   const kibanaServer = getService('kibanaServer');
   const queryBar = getService('queryBar');
   const inspector = getService('inspector');
-  const elasticChart = getService('elasticChart');
   const testSubjects = getService('testSubjects');
   const PageObjects = getPageObjects(['common', 'discover', 'header', 'timePicker']);
 
@@ -106,48 +105,6 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
         );
       });
 
-      it('should modify the time range when the histogram is brushed', async function () {
-        // this is the number of renderings of the histogram needed when new data is fetched
-        // this needs to be improved
-        const renderingCountInc = 1;
-        const prevRenderingCount = await elasticChart.getVisualizationRenderingCount();
-        await PageObjects.timePicker.setDefaultAbsoluteRange();
-        await PageObjects.discover.waitUntilSearchingHasFinished();
-        await retry.waitFor('chart rendering complete', async () => {
-          const actualCount = await elasticChart.getVisualizationRenderingCount();
-          const expectedCount = prevRenderingCount + renderingCountInc;
-          log.debug(
-            `renderings before brushing - actual: ${actualCount} expected: ${expectedCount}`
-          );
-          return actualCount === expectedCount;
-        });
-        let prevRowData = '';
-        // to make sure the table is already rendered
-        await retry.try(async () => {
-          prevRowData = await PageObjects.discover.getDocTableField(1);
-          log.debug(`The first timestamp value in doc table before brushing: ${prevRowData}`);
-        });
-
-        await PageObjects.discover.brushHistogram();
-        await PageObjects.discover.waitUntilSearchingHasFinished();
-        await retry.waitFor('chart rendering complete after being brushed', async () => {
-          const actualCount = await elasticChart.getVisualizationRenderingCount();
-          const expectedCount = prevRenderingCount + renderingCountInc * 2;
-          log.debug(
-            `renderings after brushing - actual: ${actualCount} expected: ${expectedCount}`
-          );
-          return actualCount === expectedCount;
-        });
-        const newDurationHours = await PageObjects.timePicker.getTimeDurationInHours();
-        expect(Math.round(newDurationHours)).to.be(26);
-
-        await retry.waitFor('doc table containing the documents of the brushed range', async () => {
-          const rowData = await PageObjects.discover.getDocTableField(1);
-          log.debug(`The first timestamp value in doc table after brushing: ${rowData}`);
-          return prevRowData !== rowData;
-        });
-      });
-
       it('should show correct initial chart interval of Auto', async function () {
         await PageObjects.timePicker.setDefaultAbsoluteRange();
         await PageObjects.discover.waitUntilSearchingHasFinished();
@@ -265,26 +222,6 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
       });
     });
 
-    describe('empty query', function () {
-      it('should update the histogram timerange when the query is resubmitted', async function () {
-        await kibanaServer.uiSettings.update({
-          'timepicker:timeDefaults': '{  "from": "2015-09-18T19:37:13.000Z",  "to": "now"}',
-        });
-        await PageObjects.common.navigateToApp('discover');
-        await PageObjects.header.awaitKibanaChrome();
-        const initialTimeString = await PageObjects.discover.getChartTimespan();
-        await queryBar.submitQuery();
-
-        await retry.waitFor('chart timespan to have changed', async () => {
-          const refreshedTimeString = await PageObjects.discover.getChartTimespan();
-          log.debug(
-            `Timestamp before: ${initialTimeString}, Timestamp after: ${refreshedTimeString}`
-          );
-          return refreshedTimeString !== initialTimeString;
-        });
-      });
-    });
-
     describe('managing fields', function () {
       it('should add a field, sort by it, remove it and also sorting by it', async function () {
         await PageObjects.timePicker.setDefaultAbsoluteRangeViaUiSettings();