Address PR feedback from Pierre

- remove unnecessary authz?
- remove unnecessary content-type json headers
- add loggingSystemMock.collect(mockLogger).error assertion
- reconstrcut new MockRouter on beforeEach for better sandboxing
- fix incorrect describe()s -should be it()
- pull out reusable mockDependencies helper (renamed/extended from mockConfig) for tests that don't particularly use config/log but still want to pass type definitions
- Fix comment copy

x-pack/plugins/enterprise_search/server/lib/check_access.ts

@@ -42,7 +42,7 @@ export const checkAccess = async ({
   log,
 }: ICheckAccess): Promise<IAccess> => {
   // If security has been disabled, always show the plugin
-  if (!security?.authz?.mode.useRbacForRequest(request)) {
+  if (!security?.authz.mode.useRbacForRequest(request)) {
     return ALLOW_ALL_PLUGINS;
   }
 

x-pack/plugins/enterprise_search/server/routes/__mocks__/index.ts

@@ -5,4 +5,4 @@
  */
 
 export { MockRouter } from './router.mock';
-export { mockConfig } from './config.mock';
+export { mockConfig, mockLogger, mockDependencies } from './routerDependencies.mock';

x-pack/plugins/enterprise_search/server/routes/__mocks__/routerDependencies.mock.ts

@@ -0,0 +1,27 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License;
+ * you may not use this file except in compliance with the Elastic License.
+ */
+
+import { loggingSystemMock } from 'src/core/server/mocks';
+import { ConfigType } from '../../';
+
+export const mockLogger = loggingSystemMock.createLogger().get();
+
+export const mockConfig = {
+  enabled: true,
+  host: 'http://localhost:3002',
+  accessCheckTimeout: 5000,
+  accessCheckTimeoutWarning: 300,
+} as ConfigType;
+
+/**
+ * This is useful for tests that don't use either config or log,
+ * but should still pass them in to pass Typescript definitions
+ */
+export const mockDependencies = {
+  // Mock router should be handled on a per-test basis
+  config: mockConfig,
+  log: mockLogger,
+};

x-pack/plugins/enterprise_search/server/routes/app_search/engines.test.ts

@@ -4,8 +4,7 @@
  * you may not use this file except in compliance with the Elastic License.
  */
 
-import { loggingSystemMock } from 'src/core/server/mocks';
-import { MockRouter, mockConfig } from '../__mocks__';
+import { MockRouter, mockConfig, mockLogger } from '../__mocks__';
 
 import { registerEnginesRoute } from './engines';
 
@@ -27,12 +26,11 @@ describe('engine routes', () => {
       },
     };
 
-    const mockRouter = new MockRouter({ method: 'get', payload: 'query' });
-    const mockLogger = loggingSystemMock.create().get();
+    let mockRouter: MockRouter;
 
     beforeEach(() => {
       jest.clearAllMocks();
-      mockRouter.createRouter();
+      mockRouter = new MockRouter({ method: 'get', payload: 'query' });
 
       registerEnginesRoute({
         router: mockRouter.router,
@@ -57,7 +55,6 @@ describe('engine routes', () => {
 
         expect(mockRouter.response.ok).toHaveBeenCalledWith({
           body: { results: [{ name: 'engine1' }], meta: { page: { total_results: 1 } } },
-          headers: { 'content-type': 'application/json' },
         });
       });
     });

x-pack/plugins/enterprise_search/server/routes/app_search/engines.ts

@@ -43,10 +43,7 @@ export function registerEnginesRoute({ router, config, log }: IRouteDependencies
           Array.isArray(engines?.results) && typeof engines?.meta?.page?.total_results === 'number';
 
         if (hasValidData) {
-          return response.ok({
-            body: engines,
-            headers: { 'content-type': 'application/json' },
-          });
+          return response.ok({ body: engines });
         } else {
           // Either a completely incorrect Enterprise Search host URL was configured, or App Search is returning bad data
           throw new Error(`Invalid data received from App Search: ${JSON.stringify(engines)}`);

x-pack/plugins/enterprise_search/server/routes/app_search/telemetry.test.ts

@@ -5,7 +5,7 @@
  */
 
 import { loggingSystemMock, savedObjectsServiceMock } from 'src/core/server/mocks';
-import { MockRouter } from '../__mocks__/router.mock';
+import { MockRouter, mockConfig, mockLogger } from '../__mocks__';
 
 import { registerTelemetryRoute } from './telemetry';
 
@@ -20,18 +20,18 @@ import { incrementUICounter } from '../../collectors/app_search/telemetry';
  * is tested more thoroughly in the collectors/telemetry tests.
  */
 describe('App Search Telemetry API', () => {
-  const mockRouter = new MockRouter({ method: 'put', payload: 'body' });
-  const mockLogger = loggingSystemMock.create().get();
+  let mockRouter: MockRouter;
 
   beforeEach(() => {
     jest.clearAllMocks();
-    mockRouter.createRouter();
+    mockRouter = new MockRouter({ method: 'put', payload: 'body' });
 
     registerTelemetryRoute({
       router: mockRouter.router,
-      getSavedObjectsService: () => savedObjectsServiceMock.create(),
+      getSavedObjectsService: () => savedObjectsServiceMock.createStartContract(),
       log: mockLogger,
-    } as any);
+      config: mockConfig,
+    });
   });
 
   describe('PUT /api/app_search/telemetry', () => {
@@ -71,6 +71,11 @@ describe('App Search Telemetry API', () => {
       expect(incrementUICounter).not.toHaveBeenCalled();
       expect(mockLogger.error).toHaveBeenCalled();
       expect(mockRouter.response.internalError).toHaveBeenCalled();
+      expect(loggingSystemMock.collect(mockLogger).error[0][0]).toEqual(
+        expect.stringContaining(
+          'App Search UI telemetry error: Error: Could not find Saved Objects service'
+        )
+      );
     });
 
     describe('validates', () => {
@@ -84,17 +89,17 @@ describe('App Search Telemetry API', () => {
         mockRouter.shouldThrow(request);
       });
 
-      describe('wrong metric type', () => {
+      it('wrong metric type', () => {
         const request = { body: { action: 'clicked', metric: true } };
         mockRouter.shouldThrow(request);
       });
 
-      describe('action is missing', () => {
+      it('action is missing', () => {
         const request = { body: { metric: 'engines_overview' } };
         mockRouter.shouldThrow(request);
       });
 
-      describe('metric is missing', () => {
+      it('metric is missing', () => {
         const request = { body: { action: 'error' } };
         mockRouter.shouldThrow(request);
       });

x-pack/plugins/enterprise_search/server/routes/enterprise_search/public_url.test.ts

@@ -4,7 +4,7 @@
  * you may not use this file except in compliance with the Elastic License.
  */
 
-import { MockRouter } from '../__mocks__/router.mock';
+import { MockRouter, mockDependencies } from '../__mocks__';
 
 jest.mock('../../lib/enterprise_search_config_api', () => ({
   callEnterpriseSearchConfigAPI: jest.fn(),
@@ -14,17 +14,15 @@ import { callEnterpriseSearchConfigAPI } from '../../lib/enterprise_search_confi
 import { registerPublicUrlRoute } from './public_url';
 
 describe('Enterprise Search Public URL API', () => {
-  const mockRouter = new MockRouter({ method: 'get' });
+  let mockRouter: MockRouter;
 
   beforeEach(() => {
-    jest.clearAllMocks();
-    mockRouter.createRouter();
+    mockRouter = new MockRouter({ method: 'get' });
 
     registerPublicUrlRoute({
+      ...mockDependencies,
       router: mockRouter.router,
-      config: {},
-      log: {},
-    } as any);
+    });
   });
 
   describe('GET /api/enterprise_search/public_url', () => {

x-pack/test/ui_capabilities/security_only/tests/catalogue.ts

@@ -35,7 +35,7 @@ export default function catalogueTests({ getService }: FtrProviderContext) {
           case 'dual_privileges_all': {
             expect(uiCapabilities.success).to.be(true);
             expect(uiCapabilities.value).to.have.property('catalogue');
-            // everything except ml and monitoring and enterprise_search is enabled
+            // everything except ml and monitoring is enabled
             const expected = mapValues(
               uiCapabilities.value!.catalogue,
               (enabled, catalogueId) => catalogueId !== 'ml' && catalogueId !== 'monitoring'
@@ -47,7 +47,7 @@ export default function catalogueTests({ getService }: FtrProviderContext) {
           case 'dual_privileges_read': {
             expect(uiCapabilities.success).to.be(true);
             expect(uiCapabilities.value).to.have.property('catalogue');
-            // everything except ml and monitoring and enterprise_search is enabled
+            // everything except ml and monitoring and enterprise search is enabled
             const expected = mapValues(
               uiCapabilities.value!.catalogue,
               (enabled, catalogueId) => !['ml', 'monitoring', 'appSearch'].includes(catalogueId)