codeforpdx / PASS

PASS project - with HMIS module integration
MIT License
30 stars 26 forks source link

Enhancement: Use `getByRole` or `testId` in tests over `getByLabelText` #449

Closed milofultz closed 10 months ago

milofultz commented 1 year ago

Describe the Current Behavior/Feature:

We currently have tests that use getByLabelText in the tests when other selector methods would suffice.

Proposed Behavior/Feature:

We should convert getByLabelText tests to something less fragile like getByRole/TestId.

Rationale:

Relying on LabelText for anything other than rare cases where it is necessary make tests fragile and possibly can imply that adding labels is the way to select elements.

Using getByRole in particular can also reinforce testing of functionality. For instance, in testing that a button maintains the expected accessible name, selecting it via Role of button helps reinforce that we are indeed selecting for a button, whereas selecting by LabelText can result in a false positive if the element loses the button role. This could help stop significant accessibility regressions.

Because label texts can change but functionality should change much more seldom, this also makes maintenance of the tests much easier.

Proposed Implementation (if applicable):

For instances where selecting by role is applicable, do so.

In instances where that doesn't work, giving elements a data-testid will give some of the same benefits and not rely so much on implementation details.

Example of existing test

it('renders NavbarLoggedOut when user is not logged in', () => {
-    const { getByLabelText } = render(
+    const { getByRole } = render(
       <SessionContext.Provider value={{ session: { info: { isLoggedIn: false } } }}>
         <BrowserRouter>
           <NavBar />
         </BrowserRouter>
       </SessionContext.Provider>
     );
-    const loginButton = getByLabelText('Login Button');
+    const loginButton = getByRole('button');

     expect(loginButton).not.toBeNull();
   });

See https://testing-library.com/docs/queries/byrole/ for more info on more specific selection using the name option.

leekahung commented 11 months ago

I believe these has been resolved in recent merges to Development @milofultz? If so, I think we can close this.

milofultz commented 11 months ago

A few have been resolved, but there's a bit more. I'll work on this a bit later