EES-5558 - got "Data and files" link to close modal and navigate to t… #5411
Conversation
| expect(screen.getByRole('button', { name: 'Close' })).toBeInTheDocument(); | ||
|
|
||
| expect( | ||
| await screen.findByText( | ||
| screen.getByText( | ||
| /No API data sets can be created as there are no candidate data files available/, | ||
| ), | ||
| ).toBeInTheDocument(); | ||
|
|
||
| expect(screen.getByRole('button', { name: 'Close' })).toBeInTheDocument(); | ||
| expect( | ||
| await screen.findByRole('link', { name: 'Data and files' }), | ||
| ).toBeInTheDocument(); | ||
|
|
||
| await user.click(screen.getByRole('link', { name: 'Data and files' })); | ||
|
|
||
| expect( | ||
| screen.queryByRole('link', { name: 'Data and files' }), | ||
| ).not.toBeInTheDocument(); | ||
|
|
||
| expect(history.location.pathname).toEqual( | ||
| '/publication/publication-id/release/release-id/data', | ||
| ); | ||
| expect(history.location.hash).toEqual('#data-uploads'); |
There was a problem hiding this comment.
No need to click the link and navigate - just assert that the link has the correct href. Additionally, we can tighten up the checks to be within the modal.
We can replace these lines with the following:
const modal = within(screen.getByRole('dialog'));
expect(
modal.getByText(
/No API data sets can be created as there are no candidate data files available/,
),
).toBeInTheDocument();
expect(modal.getByRole('link', { name: 'Data and files' })).toHaveAttribute(
'href',
'/publication/publication-id/release/release-id/data#data-uploads',
);There was a problem hiding this comment.
Hey @ntsim thanks for that. I'd seen that approach elsewhere as well.
The only thing that the clicking and navigation does show in and above this is that the modal is successfully closed with the navigation:
expect(
screen.queryByRole('link', { name: 'Data and files' }),
).not.toBeInTheDocument();
Without the navigation step, we don't know that the modal has been automatically closed. This being the case, would you still prefer the changes you requested, or is there maybe some other way to check for the modal closing without needing to do a full navigation perhaps?
There was a problem hiding this comment.
Not sure what the benefit is of checking that the modal has closed?
Routing is well understood to work correctly, so if a user clicks a link, the current page unmounts and is replaced by the next route's page component.
The direct implication of this is that the modal is unmounted as well because it's only mounted via the current page (which is unmounted after navigating). Clicking the link would just redundantly test that routing works, so I'm not sure why we'd want to do this given we don't typically test that links can navigate.
Additionally, to actually make an 'end-to-end' navigation, you'd need to set additional stuff up for this test, which is just out of scope tbh.
Would suggest we proceed with the changes I've outlined above!
| @@ -29,6 +29,8 @@ describe('ApiDataSetCreateModal', () => { | |||
| ]; | |||
|
|
|||
| test('renders warning message in modal when no candidates', async () => { | |||
| const history = createMemoryHistory(); | |||
There was a problem hiding this comment.
If we don't navigate into the link, we don't need to check the history and can remove this (and it's usage below)
There was a problem hiding this comment.
See my previous reply above, ta!
…he Data Uploads tab
…tests as it is not necessary to test.
duncan-at-hiveit
force-pushed
the
EES-5558-data-and-file-link-in-api-data-set-modal
branch
from
5ee45b4 to
029fc49
Compare
duncan-at-hiveit
deleted the
EES-5558-data-and-file-link-in-api-data-set-modal
branch
This PR: