Add e2e test for sorting

[hashicc]

Description

This pr adds a e2e test for the sorting capability in our tables. The roles resource was chosen because it has support for custom sorting and regular attribute sorting.

🎟️ Jira ticket

Checklist

- [ ] I have added before and after screenshots for UI changes
- [ ] I have added JSON response output for API changes
- [ ] I have added steps to reproduce and test for bug fixes in the description

• I have commented on my code, particularly in hard-to-understand areas
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
boundary-ui	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jun 18, 2025 2:20am
boundary-ui-desktop	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jun 18, 2025 2:20am

[lisbet-alvarez]

When running the tests locally, it got stuck at this line. Is the function goToRolesPage supposed to be defined in pages/roles.js?

[hashicc]

Yes, sorry about that. This time it should really work

[hashicc]

I think everyone else understood this. Hopefully this helps future me

[moduli]

Is it possible to update some of these locators to use the recommended locator methods (https://playwright.dev/docs/locators)?

Suggested change

	const table = page.locator('table');
	const table = page.getByRole('table');

[hashicc]

Updated the locators, the only ones I kept as selectors were for tbody since aria doesn't have a role for the rowgroup belonging to the body separately from the rowgroup belonging to thead. I felt like tbody was better than getByRole('rowgroup').nth(2) since knowing the 2nd rowgroup is the thead is less obvious.

It seems like the w3c is aware of this limitation in the current spec:

This role does not differentiate between types of row groups (e.g., thead vs. tbody), but an issue has been raised for WAI-ARIA 2.0.

And there's an open issue around the usefulness of rowgroup in practice

[moduli]

Would something like this work?

const rowCount = await page
            .getByRole('table')
            .getByRole('row')
            .filter({ hasNot: page.getByRole('columnheader') })
            .count();

[ZedLi]

I also generally do it this way in desktop when counting table rows

[hashicc]

That works great

[moduli]

In some other tests, I had the test find the index of the column based on the name. I don't really have a strong opinion and not sure which approach is preferred, but I wanted to just share.

     let headersCount = await page
        .getByRole('table')
        .getByRole('columnheader')
        .count();
      let fullNameIndex;
      let emailIndex;
      for (let i = 0; i < headersCount; i++) {
        const header = await page
          .getByRole('table')
          .getByRole('columnheader')
          .nth(i)
          .innerText();
        if (header == 'Full Name') {
          fullNameIndex = i;
        } else if (header == 'Email') {
          emailIndex = i;
        }
      }

[calcaide]

None blocking comment: for learning purposes, I found interesting how to select columns for e2e tests.
I checked both ways, and I do not have a strong opinion either. But I will aim for consistency within.

I do think the suggested way by moduli, seems more semantic, since we pass the column name and the code does the rest. Chad's we pass the index, therefore the user needs to know the index beforehand (not a big deal), but "less computation cost" if that makes any sense.

I think the semantic way seems more resilient, in case we add a column or change the table, the test will need no changes.

[hashicc]

This is definitely more flexible if the column headers ever change. I kept what I had, but after seeing this I did add an assertion to check that the column header labels matched

[hashicc]

I commented before reading your comment @calcaide. Check my most recent push. I think what I was missing was asserting the label which then brings these two implementations much closer together with the main difference being a static assertion vs dynamic references

[calcaide]

Yeah I saw it, I think it was like seconds of difference lol all good 😉

[calcaide]

I left a none blocking comment, mostly for learning purposes.

Run the sorting.spec isolated test successfully!! looks awesome, full coverage of the sorting together with the unit tests.

Thank you for the work 💪 🙌 🚀

[ZedLi]

I haven't ran these yet so I might just be missing something but is there a reason we can't just do something like getByRole('columnheader', { name }).getByRole('button') instead of using indices?

[hashicc]

Yup, that works! Updated

[ZedLi]

Same here, any reason we couldn't have just grabbed by name directly?

Suggested change

	expect(
	tableLocator.locator(`thead`).getByRole('columnheader').nth(nth),
	).toHaveAttribute('aria-sort', 'ascending');
	expect(
	getByRole('columnheader', { name })).toHaveAttribute('aria-sort', 'ascending');

[ZedLi]

I also generally do it this way in desktop when counting table rows