[RW-12615][risk=no] Converted admin-user-profile.spec.tsx #8602
Conversation
…institution ADDRESSES'
…institution DOMAINS'
…ey match each other'
…out adding other-text'
evrii
changed the title
| return divs.at(2).text(); | ||
| const getDropdown = (containerTestId: string): HTMLSelectElement => { | ||
| const container = screen.getByTestId(containerTestId); | ||
| return within(container).getByRole('combobox', { hidden: true }); |
There was a problem hiding this comment.
interesting! I think you, me, and Neha all have different ways of accessing dropdowns. We should probably consolidate them when we migrate to Terra-UI's version.
There was a problem hiding this comment.
I agree. I believe that consolidation will be key. I think the problem is compounded by the fact that I believe that there are currently two or three types of drop downs.
|
|
||
| expect(findTextInput(wrapper, 'contactEmail').props().value).toEqual( | ||
| BROAD_ADDR_1 |
There was a problem hiding this comment.
I would like to keep this check, to show that value changed in the course of the test
| screen.getByRole('button', { | ||
| name: /save/i, | ||
| }) | ||
| ); | ||
| }); | ||
|
|
||
| it('should not allow updating both email and institution if they match each other', async () => { |
There was a problem hiding this comment.
please update the name of this test
| expect(tableRows.length).toEqual(orderedAccessModules.length); | ||
| const table = screen.getByTestId('access-module-table'); | ||
| const rows = within(table).getAllByRole('row'); | ||
| rows.shift(); // remove the header row |
There was a problem hiding this comment.
do you know why this was not necessary before?
There was a problem hiding this comment.
The previous version (line 520) is doing a css selector tbody tr[role="row"] so it was only getting the rows in the body of the table. Since css selectors are not the preferred way of selecting within RTL, I am relying on getAllByRole('row'); which gets all the rows in the table (header and body). Since that is the case, we need to remove the first row since it is from the previously unaccounted for header row.
As an aside, there is a long open issue in the testing library that calls for a more intuitive way of testing tables:
testing-library/dom-testing-library#583
| ).exists() | ||
| ).toBeTruthy(); | ||
| const table = screen.getByTestId('access-module-table'); | ||
| within(table).getByText(`requires eRA Commons for ${tiers} access`, { |
There was a problem hiding this comment.
while not strictly necessary, I believe the best practice is to still use an expect() here
There was a problem hiding this comment.
I agree. I wrestled with this for a while, but I think you're right, because it makes our assertions more explicit:
https://kentcdodds.com/blog/common-mistakes-with-react-testing-library#using-get-variants-as-assertions
PR checklist
[risk=no|low|moderate|severe]in the PR title as outlined in CONTRIBUTING.md.