From 85e186cf10b0d672be5ee17f906f19efb765889b Mon Sep 17 00:00:00 2001
From: Josh Hunt <joshhunt@users.noreply.github.com>
Date: Tue, 16 Feb 2021 16:19:55 +0000
Subject: [PATCH] Grafana-UI: Add id to Select to make it easier to test
 (#31230)

* Prettier formatting

* Grafana-UI: Add support for id and inputId props to Select

* Grafana-UI: Add aria-label to Select

* Grafana-UI: InlineField and Field get ID from inputId prop

For Select

* Fix tests using TagFilter

* Update Select prop documentation

* Update Field tests to use screen instead

* Fix the last few tests
---
 package.json                                  |  2 +-
 .../src/components/Forms/Field.test.tsx       | 37 +++++++++++++
 .../grafana-ui/src/components/Forms/Field.tsx | 10 +---
 .../src/components/Forms/InlineField.test.tsx | 37 +++++++++++++
 .../src/components/Forms/InlineField.tsx      |  7 +--
 .../src/components/Select/Select.mdx          | 54 ++++++++++++++-----
 .../src/components/Select/SelectBase.test.tsx | 42 ++++++++++-----
 .../src/components/Select/SelectBase.tsx      |  6 +++
 .../grafana-ui/src/components/Select/types.ts |  8 ++-
 packages/grafana-ui/src/utils/children.ts     | 19 +++++++
 .../core/components/TagFilter/TagFilter.tsx   |  4 +-
 .../components/DashboardSearch.test.tsx       |  9 ++--
 .../components/SearchResultsFilter.test.tsx   |  4 +-
 yarn.lock                                     | 21 ++++++++
 14 files changed, 210 insertions(+), 50 deletions(-)
 create mode 100644 packages/grafana-ui/src/components/Forms/Field.test.tsx
 create mode 100644 packages/grafana-ui/src/components/Forms/InlineField.test.tsx
 create mode 100644 packages/grafana-ui/src/utils/children.ts

diff --git a/package.json b/package.json
index 8a3f1ed944f..371f500ab32 100644
--- a/package.json
+++ b/package.json
@@ -175,6 +175,7 @@
     "postcss-reporter": "6.0.1",
     "prettier": "2.2.1",
     "react-hot-loader": "4.8.0",
+    "react-select-event": "^5.1.0",
     "react-test-renderer": "16.12.0",
     "redux-mock-store": "1.5.4",
     "regexp-replace-loader": "1.0.1",
@@ -216,7 +217,6 @@
     "@types/react-virtualized-auto-sizer": "1.0.0",
     "@types/sockjs-client": "^1.1.1",
     "@types/uuid": "8.3.0",
-    "@types/hoist-non-react-statics": "3.3.1",
     "@welldone-software/why-did-you-render": "4.0.6",
     "abortcontroller-polyfill": "1.4.0",
     "angular": "1.8.2",
diff --git a/packages/grafana-ui/src/components/Forms/Field.test.tsx b/packages/grafana-ui/src/components/Forms/Field.test.tsx
new file mode 100644
index 00000000000..970985fa95c
--- /dev/null
+++ b/packages/grafana-ui/src/components/Forms/Field.test.tsx
@@ -0,0 +1,37 @@
+import React from 'react';
+import { render, screen } from '@testing-library/react';
+import { Input } from '../Input/Input';
+import { Field } from './Field';
+import { Select } from '../Select/Select';
+
+describe('Field', () => {
+  it('renders the label', () => {
+    render(
+      <Field label="My label">
+        <Input id="my-text-input" />
+      </Field>
+    );
+
+    expect(screen.getByText('My label')).toBeInTheDocument();
+  });
+
+  it('renders with the id of its children', () => {
+    render(
+      <Field label="My label">
+        <Input id="my-text-input" />
+      </Field>
+    );
+
+    expect(screen.getByLabelText('My label')).toBeInTheDocument();
+  });
+
+  it('renders with the inputId of its children', () => {
+    render(
+      <Field label="My other label">
+        <Select inputId="my-select-input" onChange={() => {}} />
+      </Field>
+    );
+
+    expect(screen.getByLabelText('My other label')).toBeInTheDocument();
+  });
+});
diff --git a/packages/grafana-ui/src/components/Forms/Field.tsx b/packages/grafana-ui/src/components/Forms/Field.tsx
index 8b4fe85b0f7..6602f07f09f 100644
--- a/packages/grafana-ui/src/components/Forms/Field.tsx
+++ b/packages/grafana-ui/src/components/Forms/Field.tsx
@@ -4,6 +4,7 @@ import { stylesFactory, useTheme } from '../../themes';
 import { css, cx } from 'emotion';
 import { GrafanaTheme } from '@grafana/data';
 import { FieldValidationMessage } from './FieldValidationMessage';
+import { getChildId } from '../../utils/children';
 
 export interface FieldProps extends HTMLAttributes<HTMLDivElement> {
   /** Form input element, i.e Input or Switch */
@@ -62,16 +63,9 @@ export const Field: React.FC<FieldProps> = ({
   ...otherProps
 }) => {
   const theme = useTheme();
-  let inputId;
   const styles = getFieldStyles(theme);
+  const inputId = getChildId(children);
 
-  // Get the first, and only, child to retrieve form input's id
-  const child = React.Children.map(children, (c) => c)[0];
-
-  if (child) {
-    // Retrieve input's id to apply on the label for correct click interaction
-    inputId = (child as React.ReactElement<{ id?: string }>).props.id;
-  }
   const labelElement =
     typeof label === 'string' ? (
       <Label htmlFor={inputId} description={description}>
diff --git a/packages/grafana-ui/src/components/Forms/InlineField.test.tsx b/packages/grafana-ui/src/components/Forms/InlineField.test.tsx
new file mode 100644
index 00000000000..9945649883f
--- /dev/null
+++ b/packages/grafana-ui/src/components/Forms/InlineField.test.tsx
@@ -0,0 +1,37 @@
+import React from 'react';
+import { render, screen } from '@testing-library/react';
+import { Input } from '../Input/Input';
+import { InlineField } from './InlineField';
+import { Select } from '../Select/Select';
+
+describe('InlineField', () => {
+  it('renders the label', () => {
+    render(
+      <InlineField label="My label">
+        <Input id="my-text-input" />
+      </InlineField>
+    );
+
+    expect(screen.getByText('My label')).toBeInTheDocument();
+  });
+
+  it('renders with the id of its children', () => {
+    render(
+      <InlineField label="My label">
+        <Input id="my-text-input" />
+      </InlineField>
+    );
+
+    expect(screen.getByLabelText('My label')).toBeInTheDocument();
+  });
+
+  it('renders with the inputId of its children', () => {
+    render(
+      <InlineField label="My other label">
+        <Select inputId="my-select-input" onChange={() => {}} />
+      </InlineField>
+    );
+
+    expect(screen.getByLabelText('My other label')).toBeInTheDocument();
+  });
+});
diff --git a/packages/grafana-ui/src/components/Forms/InlineField.tsx b/packages/grafana-ui/src/components/Forms/InlineField.tsx
index 540d03015c6..3156c69a8eb 100644
--- a/packages/grafana-ui/src/components/Forms/InlineField.tsx
+++ b/packages/grafana-ui/src/components/Forms/InlineField.tsx
@@ -5,6 +5,7 @@ import { useTheme } from '../../themes';
 import { InlineLabel } from './InlineLabel';
 import { PopoverContent } from '../Tooltip/Tooltip';
 import { FieldProps } from './Field';
+import { getChildId } from '../../utils/children';
 
 export interface Props extends Omit<FieldProps, 'css' | 'horizontal' | 'description' | 'error'> {
   /** Content for the label's tooltip */
@@ -32,12 +33,8 @@ export const InlineField: FC<Props> = ({
 }) => {
   const theme = useTheme();
   const styles = getStyles(theme, grow);
-  const child = React.Children.only(children);
-  let inputId;
+  const inputId = getChildId(children);
 
-  if (child) {
-    inputId = (child as React.ReactElement<{ id?: string }>).props.id;
-  }
   const labelElement =
     typeof label === 'string' ? (
       <InlineLabel width={labelWidth} tooltip={tooltip} htmlFor={inputId} transparent={transparent}>
diff --git a/packages/grafana-ui/src/components/Select/Select.mdx b/packages/grafana-ui/src/components/Select/Select.mdx
index 89ab9153680..db9e1fb7eae 100644
--- a/packages/grafana-ui/src/components/Select/Select.mdx
+++ b/packages/grafana-ui/src/components/Select/Select.mdx
@@ -1,6 +1,6 @@
-import { Props, Preview } from "@storybook/addon-docs/blocks";
-import { Select, AsyncSelect, MultiSelect, AsyncMultiSelect } from "./Select";
-import { generateOptions } from "./mockOptions";
+import { Props, Preview } from '@storybook/addon-docs/blocks';
+import { Select, AsyncSelect, MultiSelect, AsyncMultiSelect } from './Select';
+import { generateOptions } from './mockOptions';
 
 # Select variants
 
@@ -27,13 +27,13 @@ There are four properties for each option:
 
 ```jsx
 const options = [
-  { label: "Basic option", value: 0 },
-  { label: "Option with description", value: 1, description: "this is a description" },
+  { label: 'Basic option', value: 0 },
+  { label: 'Option with description', value: 1, description: 'this is a description' },
   {
-    label: "Option with description and image",
+    label: 'Option with description and image',
     value: 2,
-    description: "This is a very elaborate description, describing all the wonders in the world.",
-    imgUrl: "https://placekitten.com/40/40",
+    description: 'This is a very elaborate description, describing all the wonders in the world.',
+    imgUrl: 'https://placekitten.com/40/40',
   },
 ];
 ```
@@ -90,7 +90,7 @@ Where the async function could look like this:
 
 ```tsx
 const loadAsyncOptions = () => {
-  return new Promise<Array<SelectableValue<string>>>(resolve => {
+  return new Promise<Array<SelectableValue<string>>>((resolve) => {
     setTimeout(() => {
       resolve(options);
     }, 2000);
@@ -103,7 +103,7 @@ const loadAsyncOptions = () => {
 Possible to Select multiple values at the same time.
 
 ```tsx
-import { MultiSelect } from "@grafana/ui";
+import { MultiSelect } from '@grafana/ui';
 
 const multiSelect = () => {
   const [value, setValue] = useState<Array<SelectableValue<string>>>([]);
@@ -113,7 +113,7 @@ const multiSelect = () => {
       <MultiSelect
         options={options}
         value={value}
-        onChange={v => {
+        onChange={(v) => {
           setValue(v);
         }}
       />
@@ -126,6 +126,36 @@ const multiSelect = () => {
 
 Like MultiSelect but handles data asynchronously with the `loadOptions` prop.
 
-## Props
+# Testing
+
+Using React Testing Library, you can select the `<Select />` using its matching label, such as the label assigned with the `inputId` prop. Use the `react-select-event` package to select values from the options.
+
+```tsx
+import { render, screen } from '@testing-library/react';
+import selectEvent from 'react-select-event';
+import { Select } from '@grafana/ui';
+
+it('should call onChange', () => {
+  const onChange = jest.fn();
+
+  render(
+    <>
+      <label htmlFor="my-select">My select</label>
+      <Select onChange={onChange} options={options} inputId="my-select" />
+    </>
+  );
+
+  const selectEl = screen.getByLabelText('My select');
+  expect(selectEl).toBeInTheDocument();
+
+  await selectEvent.select(selectEl, 'Option 2');
+  expect(onChange).toHaveBeenCalledWith({
+    label: 'Option 2',
+    value: 2,
+  });
+});
+```
+
+# Props
 
 <Props of={Select} />
diff --git a/packages/grafana-ui/src/components/Select/SelectBase.test.tsx b/packages/grafana-ui/src/components/Select/SelectBase.test.tsx
index 8c1c5cde710..48a358b3e9a 100644
--- a/packages/grafana-ui/src/components/Select/SelectBase.test.tsx
+++ b/packages/grafana-ui/src/components/Select/SelectBase.test.tsx
@@ -1,5 +1,7 @@
 import React from 'react';
 import { mount, ReactWrapper } from 'enzyme';
+import { render, screen } from '@testing-library/react';
+import selectEvent from 'react-select-event';
 import { SelectBase } from './SelectBase';
 import { SelectableValue } from '@grafana/data';
 import { MultiValueContainer } from './MultiValue';
@@ -28,6 +30,19 @@ describe('SelectBase', () => {
     expect(noopt).toHaveLength(1);
   });
 
+  it('is selectable via its label text', async () => {
+    const onChange = jest.fn();
+
+    render(
+      <>
+        <label htmlFor="my-select">My select</label>
+        <SelectBase onChange={onChange} options={options} inputId="my-select" />
+      </>
+    );
+
+    expect(screen.getByLabelText('My select')).toBeInTheDocument();
+  });
+
   describe('when openMenuOnFocus prop', () => {
     describe('is provided', () => {
       it('opens on focus', () => {
@@ -158,22 +173,23 @@ describe('SelectBase', () => {
 
   describe('options', () => {
     it('renders menu with provided options', () => {
-      const container = mount(<SelectBase options={options} onChange={onChangeHandler} isOpen />);
-      const menuOptions = container.find({ 'aria-label': 'Select option' });
+      render(<SelectBase options={options} onChange={onChangeHandler} isOpen />);
+      const menuOptions = screen.getAllByLabelText('Select option');
       expect(menuOptions).toHaveLength(2);
     });
-    it('call onChange handler when option is selected', () => {
-      const spy = jest.fn();
-      const handler = (value: SelectableValue<number>) => spy(value);
-      const container = mount(<SelectBase options={options} onChange={handler} isOpen />);
-      const menuOptions = container.find({ 'aria-label': 'Select option' });
-      expect(menuOptions).toHaveLength(2);
-      const menuOption = menuOptions.first();
-      menuOption.simulate('click');
 
-      expect(spy).toBeCalledWith({
-        label: 'Option 1',
-        value: 1,
+    it('call onChange handler when option is selected', async () => {
+      const spy = jest.fn();
+
+      render(<SelectBase onChange={spy} options={options} aria-label="My select" />);
+
+      const selectEl = screen.getByLabelText('My select');
+      expect(selectEl).toBeInTheDocument();
+
+      await selectEvent.select(selectEl, 'Option 2');
+      expect(spy).toHaveBeenCalledWith({
+        label: 'Option 2',
+        value: 2,
       });
     });
   });
diff --git a/packages/grafana-ui/src/components/Select/SelectBase.tsx b/packages/grafana-ui/src/components/Select/SelectBase.tsx
index 70afab8af75..e46ebafb06b 100644
--- a/packages/grafana-ui/src/components/Select/SelectBase.tsx
+++ b/packages/grafana-ui/src/components/Select/SelectBase.tsx
@@ -90,6 +90,7 @@ const CustomControl = (props: any) => {
 
 export function SelectBase<T>({
   allowCustomValue = false,
+  'aria-label': ariaLabel,
   autoFocus = false,
   backspaceRemovesValue = true,
   cacheOptions,
@@ -106,8 +107,10 @@ export function SelectBase<T>({
   inputValue,
   invalid,
   isClearable = false,
+  id,
   isLoading = false,
   isMulti = false,
+  inputId,
   isOpen,
   isOptionDisabled,
   isSearchable = true,
@@ -168,6 +171,7 @@ export function SelectBase<T>({
   }
 
   const commonSelectProps = {
+    'aria-label': ariaLabel,
     autoFocus,
     backspaceRemovesValue,
     captureMenuScroll: false,
@@ -181,10 +185,12 @@ export function SelectBase<T>({
     inputValue,
     invalid,
     isClearable,
+    id,
     // Passing isDisabled as react-select accepts this prop
     isDisabled: disabled,
     isLoading,
     isMulti,
+    inputId,
     isOptionDisabled,
     isSearchable,
     maxMenuHeight,
diff --git a/packages/grafana-ui/src/components/Select/types.ts b/packages/grafana-ui/src/components/Select/types.ts
index f9dfceac73c..0dee841f29d 100644
--- a/packages/grafana-ui/src/components/Select/types.ts
+++ b/packages/grafana-ui/src/components/Select/types.ts
@@ -7,6 +7,8 @@ export type InputActionMeta = {
 };
 
 export interface SelectCommonProps<T> {
+  /** Aria label applied to the input field */
+  ['aria-label']?: string;
   allowCustomValue?: boolean;
   /** Focus is set to the Select when rendered*/
   autoFocus?: boolean;
@@ -18,15 +20,19 @@ export interface SelectCommonProps<T> {
   defaultValue?: any;
   disabled?: boolean;
   filterOption?: (option: SelectableValue, searchQuery: string) => boolean;
-  /**   Function for formatting the text that is displayed when creating a new value*/
+  /** Function for formatting the text that is displayed when creating a new value*/
   formatCreateLabel?: (input: string) => string;
   getOptionLabel?: (item: SelectableValue<T>) => React.ReactNode;
   getOptionValue?: (item: SelectableValue<T>) => string;
   inputValue?: string;
   invalid?: boolean;
   isClearable?: boolean;
+  /** The id to set on the SelectContainer component. To set the id for a label (with htmlFor), @see inputId instead */
+  id?: string;
   isLoading?: boolean;
   isMulti?: boolean;
+  /** The id of the search input. Use this to set a matching label with htmlFor */
+  inputId?: string;
   isOpen?: boolean;
   /** Disables the possibility to type into the input*/
   isSearchable?: boolean;
diff --git a/packages/grafana-ui/src/utils/children.ts b/packages/grafana-ui/src/utils/children.ts
new file mode 100644
index 00000000000..d2556ea23e1
--- /dev/null
+++ b/packages/grafana-ui/src/utils/children.ts
@@ -0,0 +1,19 @@
+import React, { ReactElement } from 'react';
+
+/** Returns the ID value of the first, and only, child element  */
+export function getChildId(children: ReactElement): string | undefined {
+  let inputId: unknown;
+
+  // Get the first, and only, child to retrieve form input's id
+  const child = React.Children.only(children);
+
+  // Retrieve input's id to apply on the label for correct click interaction
+  // For some components (like Select), we want to get the ID from a different prop
+  if ('id' in child?.props) {
+    inputId = child.props.id;
+  } else if ('inputId' in child.props) {
+    inputId = child?.props.inputId;
+  }
+
+  return typeof inputId === 'string' ? inputId : undefined;
+}
diff --git a/public/app/core/components/TagFilter/TagFilter.tsx b/public/app/core/components/TagFilter/TagFilter.tsx
index 1fc2382b754..68e28036ef6 100644
--- a/public/app/core/components/TagFilter/TagFilter.tsx
+++ b/public/app/core/components/TagFilter/TagFilter.tsx
@@ -93,13 +93,13 @@ export const TagFilter: FC<Props> = ({
   };
 
   return (
-    <div className={styles.tagFilter} aria-label="Tag filter">
+    <div className={styles.tagFilter}>
       {isClearable && tags.length > 0 && (
         <span className={styles.clear} onClick={() => onTagChange([])}>
           Clear tags
         </span>
       )}
-      <AsyncSelect {...selectOptions} prefix={<Icon name="tag-alt" />} />
+      <AsyncSelect {...selectOptions} prefix={<Icon name="tag-alt" />} aria-label="Tag filter" />
     </div>
   );
 };
diff --git a/public/app/features/search/components/DashboardSearch.test.tsx b/public/app/features/search/components/DashboardSearch.test.tsx
index 7319e3c8598..f2a70446e66 100644
--- a/public/app/features/search/components/DashboardSearch.test.tsx
+++ b/public/app/features/search/components/DashboardSearch.test.tsx
@@ -1,6 +1,6 @@
 import React from 'react';
 import { render, fireEvent, screen, waitFor, act } from '@testing-library/react';
-import userEvent from '@testing-library/user-event';
+import selectEvent from 'react-select-event';
 import * as SearchSrv from 'app/core/services/search_srv';
 import * as MockSearchSrv from 'app/core/services/__mocks__/search_srv';
 import { DashboardSearch, Props } from './DashboardSearch';
@@ -94,12 +94,9 @@ describe('DashboardSearch', () => {
     setup();
 
     await waitFor(() => screen.getByLabelText('Tag filter'));
-    // Get the actual element for the underlying Select component, since Select doesn't accept aria- props
-    const tagComponent = screen.getByLabelText('Tag filter').querySelector('div') as Node;
-    fireEvent.keyDown(tagComponent, { keyCode: 40 });
 
-    const firstTag = await screen.findByText('tag1');
-    userEvent.click(firstTag);
+    const tagComponent = screen.getByLabelText('Tag filter');
+    await selectEvent.select(tagComponent, 'tag1');
 
     expect(tagComponent).toBeInTheDocument();
 
diff --git a/public/app/features/search/components/SearchResultsFilter.test.tsx b/public/app/features/search/components/SearchResultsFilter.test.tsx
index 4d5d1805d83..e3ee3440643 100644
--- a/public/app/features/search/components/SearchResultsFilter.test.tsx
+++ b/public/app/features/search/components/SearchResultsFilter.test.tsx
@@ -1,5 +1,6 @@
 import React from 'react';
 import { fireEvent, render, screen } from '@testing-library/react';
+import selectEvent from 'react-select-event';
 import { Props, SearchResultsFilter } from './SearchResultsFilter';
 import { SearchLayout } from '../types';
 
@@ -80,9 +81,8 @@ describe('SearchResultsFilter', () => {
       query: { ...searchQuery, tag: [] },
     });
     const tagComponent = await screen.findByLabelText('Tag filter');
+    await selectEvent.select(tagComponent, 'tag1');
 
-    fireEvent.keyDown(tagComponent.querySelector('div') as Node, { keyCode: 40 });
-    fireEvent.click(await screen.findByText('tag1'));
     expect(mockFilterByTags).toHaveBeenCalledTimes(1);
     expect(mockFilterByTags).toHaveBeenCalledWith(['tag1']);
   });
diff --git a/yarn.lock b/yarn.lock
index 49bb770ab58..deaf95b8ffd 100644
--- a/yarn.lock
+++ b/yarn.lock
@@ -5725,6 +5725,20 @@
     resolve-from "^5.0.0"
     store2 "^2.7.1"
 
+"@testing-library/dom@>=7":
+  version "7.29.4"
+  resolved "https://registry.yarnpkg.com/@testing-library/dom/-/dom-7.29.4.tgz#1647c2b478789621ead7a50614ad81ab5ae5b86c"
+  integrity sha512-CtrJRiSYEfbtNGtEsd78mk1n1v2TUbeABlNIcOCJdDfkN5/JTOwQEbbQpoSRxGqzcWPgStMvJ4mNolSuBRv1NA==
+  dependencies:
+    "@babel/code-frame" "^7.10.4"
+    "@babel/runtime" "^7.12.5"
+    "@types/aria-query" "^4.2.0"
+    aria-query "^4.2.2"
+    chalk "^4.1.0"
+    dom-accessibility-api "^0.5.4"
+    lz-string "^1.4.4"
+    pretty-format "^26.6.2"
+
 "@testing-library/dom@^7.26.6":
   version "7.26.6"
   resolved "https://registry.yarnpkg.com/@testing-library/dom/-/dom-7.26.6.tgz#d558db63070a3acea5bea7e2497e631cd12541cc"
@@ -22310,6 +22324,13 @@ react-reverse-portal@^2.0.1:
   resolved "https://registry.yarnpkg.com/react-reverse-portal/-/react-reverse-portal-2.0.1.tgz#23b18292c531fb7b343d85a614c15a995838ba31"
   integrity sha512-sj/D9nSHspqV8i8hWkTSZ5Ohnrqk2A5fkDKw4Xe/zV4OfF1UYwmbzrxLdmNRdKkWgQwnXIxaa2E3FC7QYdZAeA==
 
+react-select-event@^5.1.0:
+  version "5.1.0"
+  resolved "https://registry.yarnpkg.com/react-select-event/-/react-select-event-5.1.0.tgz#d45ef68f2a9c872903e8c9725f3ae6e7576f7be0"
+  integrity sha512-D5DzJlYCdZsGbDVFMQFynrG0OLalJM3ZzDT7KQADNVWE604JCeQF9bIuvPZqVD7IzhnPsFzOUCsilzDA6w6WRQ==
+  dependencies:
+    "@testing-library/dom" ">=7"
+
 react-select@^3.0.8:
   version "3.2.0"
   resolved "https://registry.yarnpkg.com/react-select/-/react-select-3.2.0.tgz#de9284700196f5f9b5277c5d850a9ce85f5c72fe"