Add unit tests for request manager actions#7166
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
speaker-ender
left a comment
There was a problem hiding this comment.
I have a few questions and concerns about what we are testing in this file.
If we want to make sure we have basic coverage I can approve but I worry about the long term efficacy of some of these tests.
| /** | ||
| * Action visibility matrix for all privacy request statuses. | ||
| * This table drives the parameterized tests below. | ||
| */ | ||
| const actionVisibilityByStatus = [ |
There was a problem hiding this comment.
I think this is good. Having a hard coded list of possible states for testing
| expectButtonVisibility(buttons.approve, approve); | ||
| expectButtonVisibility(buttons.deny, deny); | ||
| expectButtonVisibility(buttons.finalize, finalize); | ||
| expectButtonVisibility(buttons.delete, deleteBtn); |
There was a problem hiding this comment.
Is there a reason the logic/components being tested here were not extracted?
Having to render the parent component to test this logic seems excessive.
There was a problem hiding this comment.
Good suggestion-- I moved the logic for button visibility into a separate helper function and we now test that directly without rendering the component.
| const modal = screen.getByTestId("confirmation-modal"); | ||
| expect(modal).toBeInTheDocument(); | ||
|
|
||
| const confirmBtn = screen.getByTestId("confirmation-modal-confirm"); | ||
| await user.click(confirmBtn); | ||
|
|
||
| expect(mockHandleDeleteRequest).toHaveBeenCalledTimes(1); | ||
| expect( | ||
| screen.queryByTestId("confirmation-modal"), | ||
| ).not.toBeInTheDocument(); |
There was a problem hiding this comment.
I'm struggling to understand what code is being tested here that isn't just being mocked above.
The modal is a mock, the confirm button is being mocked, the onClick action is being mocked, and the request hook and data is being mocked.
There was a problem hiding this comment.
Yeah, I've just gone ahead and removed these-- if there's something here that should be getting tested with real components etc. that I should add back in let me know.
| render(<RequestTableActions subjectRequest={baseRequest} />); | ||
|
|
||
| const approveBtn = screen.getByTestId("privacy-request-approve-btn"); | ||
| expect(approveBtn).toHaveAttribute("aria-busy", "true"); |
There was a problem hiding this comment.
Why are we testing a mocked attribute?
There was a problem hiding this comment.
Yep, went ahead and removed this one too.
| get isLoading() { | ||
| return mockIsLoading; | ||
| }, |
There was a problem hiding this comment.
Is it not possible to mock this via a jest mock fn?
Using a mutable variable for this seems error prone.
Ticket ENG-1626
Description Of Changes
Adds unit tests for the RequestTableActions.tsx component. Tests both which action is shown as well as the interactions with each action.
Test Suites (21 tests)
Key Features Tested
Code Changes
Steps to Confirm
Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works