From 35d2eb5a9bda3ab204303271fd1da74b7a96112a Mon Sep 17 00:00:00 2001
From: Sebastian Silbermann <silbermann.sebastian@gmail.com>
Date: Mon, 18 Nov 2019 16:33:34 +0100
Subject: [PATCH] [test] Check a11y tree inclusion in CI only

---
 CONTRIBUTING.md                                  | 16 ++++++++++++++--
 .../src/Breadcrumbs/Breadcrumbs.test.js          | 10 +++++-----
 packages/material-ui/src/Select/Select.test.js   |  2 +-
 .../material-ui/test/integration/Menu.test.js    |  9 ++++-----
 test/utils/init.js                               |  9 ++++++++-
 5 files changed, 32 insertions(+), 14 deletions(-)

diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
index 88fbf9323ea7be..3eaeb0c63cff6c 100644
--- a/CONTRIBUTING.md
+++ b/CONTRIBUTING.md
@@ -113,13 +113,13 @@ how to fix the issues.
 ##### ci/circleci: test_unit-1
 
 Runs the unit tests in a `jsdom` environment. If this fails then `yarn test:unit`
-should fail locally as well. You can narrow the scope of tests run with `yarn test:unit --grep ComponentName`.
+should<sup>[1](#accessiblity-tree-exclusion)</sup> fail locally as well. You can narrow the scope of tests run with `yarn test:unit --grep ComponentName`.
 
 ##### ci/circleci: test_browser-1
 
 Runs the unit tests in multiple browsers (via Browserstack). The log of the failed
 build should list which browsers failed. If Chrome failed then `yarn test:karma`
-should fail locally as well. If other browsers failed debugging might be trickier.
+should<sup>[1](#accessiblity-tree-exclusion)</sup> fail locally as well. If other browsers failed debugging might be trickier.
 
 ##### ci/circleci: test_regression-1
 
@@ -158,6 +158,18 @@ it's always appreciated if it can be improved.
 There are various other checks done by Netlify to check the integrity of our docs. Click
 on _Details_ to find out more about them.
 
+#### Caveats
+
+##### Accessiblity tree exclusion
+
+Our tests also explicitly document which parts of the queried element are included in
+the accessibility (a11y) tree and which are excluded. This check is fairly expensive which
+is why it is disabled when tests are run locally by default. The rationale being
+that in almost all cases including or excluding elements from a query-set depending
+on their a11y-tree membership makes no difference. The queries where this does
+make a difference explicityl include this check e.g. `getByRole('button', { hidden: false })` (see [byRole documentation](https://testing-library.com/docs/dom-testing-library/api-queries#byrole) for more information).
+To see if your test (`test:browser` or `test:unit`) behaves the same between CI and locally set the environment variable `CI` to `'true'`.
+
 ### Testing the documentation site
 
 The documentation site is built with Material-UI and contains examples of all the components.
diff --git a/packages/material-ui/src/Breadcrumbs/Breadcrumbs.test.js b/packages/material-ui/src/Breadcrumbs/Breadcrumbs.test.js
index c3c89af7c6e3f9..cd0f64b5a7f10f 100644
--- a/packages/material-ui/src/Breadcrumbs/Breadcrumbs.test.js
+++ b/packages/material-ui/src/Breadcrumbs/Breadcrumbs.test.js
@@ -37,7 +37,7 @@ describe('<Breadcrumbs />', () => {
       </Breadcrumbs>,
     );
 
-    expect(getAllByRole('listitem')).to.have.length(2);
+    expect(getAllByRole('listitem', { hidden: false })).to.have.length(2);
     expect(getByRole('list')).to.have.text('first/second');
   });
 
@@ -56,7 +56,7 @@ describe('<Breadcrumbs />', () => {
       </Breadcrumbs>,
     );
 
-    const listitems = getAllByRole('listitem');
+    const listitems = getAllByRole('listitem', { hidden: false });
     expect(listitems).to.have.length(3);
     expect(getByRole('list')).to.have.text('first//ninth');
     expect(listitems[1].querySelector('[data-mui-test="MoreHorizIcon"]')).to.be.ok;
@@ -77,9 +77,9 @@ describe('<Breadcrumbs />', () => {
       </Breadcrumbs>,
     );
 
-    getAllByRole('listitem')[2].click();
+    getAllByRole('listitem', { hidden: false })[2].click();
 
-    expect(getAllByRole('listitem')).to.have.length(3);
+    expect(getAllByRole('listitem', { hidden: false })).to.have.length(3);
   });
 
   describe('warnings', () => {
@@ -100,7 +100,7 @@ describe('<Breadcrumbs />', () => {
           <span>fourth</span>
         </Breadcrumbs>,
       );
-      expect(getAllByRole('listitem')).to.have.length(4);
+      expect(getAllByRole('listitem', { hidden: false })).to.have.length(4);
       expect(getByRole('list')).to.have.text('first/second/third/fourth');
       expect(consoleErrorMock.callCount()).to.equal(2); // strict mode renders twice
       expect(consoleErrorMock.args()[0][0]).to.include(
diff --git a/packages/material-ui/src/Select/Select.test.js b/packages/material-ui/src/Select/Select.test.js
index d334fd739116f9..0c6b33cdddc59f 100644
--- a/packages/material-ui/src/Select/Select.test.js
+++ b/packages/material-ui/src/Select/Select.test.js
@@ -125,7 +125,7 @@ describe('<Select />', () => {
     });
 
     expect(handleBlur.callCount).to.equal(0);
-    expect(queryByRole('listbox')).to.be.null;
+    expect(queryByRole('listbox', { hidden: false })).to.be.null;
   });
 
   it('options should have a data-value attribute', () => {
diff --git a/packages/material-ui/test/integration/Menu.test.js b/packages/material-ui/test/integration/Menu.test.js
index b36b1eac715f9e..c4c40669893e3c 100644
--- a/packages/material-ui/test/integration/Menu.test.js
+++ b/packages/material-ui/test/integration/Menu.test.js
@@ -315,7 +315,7 @@ describe('<Menu /> integration', () => {
   });
 
   it('closes the menu when Tabbing while the list is active', () => {
-    const { getByRole, queryByRole } = render(<ButtonMenu />);
+    const { getByRole } = render(<ButtonMenu />);
 
     getByRole('button').focus();
     getByRole('button').click();
@@ -324,11 +324,11 @@ describe('<Menu /> integration', () => {
     // react-transition-group uses one commit per state transition so we need to wait a bit
     clock.tick(0);
 
-    expect(queryByRole('menu')).to.be.null;
+    expect(getByRole('menu', { hidden: true })).to.be.inaccessible;
   });
 
   it('closes the menu when the backdrop is clicked', () => {
-    const { getByRole, queryByRole } = render(<ButtonMenu />);
+    const { getByRole } = render(<ButtonMenu />);
     const button = getByRole('button');
 
     button.focus();
@@ -337,7 +337,6 @@ describe('<Menu /> integration', () => {
     document.querySelector('[data-mui-test="Backdrop"]').click();
     clock.tick(0);
 
-    // TODO use getByRole with hidden and match that it's inaccessible
-    expect(queryByRole('menu')).to.be.null;
+    expect(getByRole('menu', { hidden: true })).to.be.inaccessible;
   });
 });
diff --git a/test/utils/init.js b/test/utils/init.js
index 42ca8ac7e2716f..9c244342638fde 100644
--- a/test/utils/init.js
+++ b/test/utils/init.js
@@ -1,8 +1,15 @@
-import enzyme from 'enzyme/build/index';
+import enzyme from 'enzyme';
 import Adapter from 'enzyme-adapter-react-16';
+import * as testingLibrary from '@testing-library/react/pure';
 import consoleError from './consoleError';
 import './initMatchers';
 
 consoleError();
 
 enzyme.configure({ adapter: new Adapter() });
+
+// checking if an element is hidden is quite expensive
+// this is only done in CI as a fail safe. It can still explicitly be checked
+// in the test files which helps documenting what is part of the DOM but hidden
+// from assistive technology
+testingLibrary.configure({ defaultHidden: !process.env.CI });