DG download "remove all" button disables once pressed #1177 #1184
Conversation
Once pressed, the remove all button is disabled and displays an indeterminate loading spinner until the request completes.
sam-glendenning
added
bug
| setTimeout(() => { | ||
| wrapper.update(); | ||
| expect(wrapper.exists('[data-testid="no-selections-message"]')).toBe( | ||
| true | ||
| ); | ||
| }, 2001); |
There was a problem hiding this comment.
This is what I'm not a fan of. It manually adds a non-mocked timeout to the test to force the wrapper to update and display the correct thing on the DOM. We want to use fake timers to adhere to good testing practice. Any help on this one would be appreciated!
There was a problem hiding this comment.
Jest's fake timers and promises inherently don't work well with one another - see https://github.com/ral-facilities/scigateway/blob/169b596d761fc7426136faca946db692849f3f19/src/App.test.tsx#L32 for an example of me working around it. See this issue for info: jestjs/jest#10221. One option is to try using the legacy Jest fake timers, as for example there are a few tests in SG which work with old timers and not with the new ones (like the one I linked). However, the only way I could get things working was to manually wait for real timers. Perhaps using a dedicated sleep function or even just a sleep line of code like await new Promise((r) => setTimeout(r, 2000)); is more semantic, so you use that line then you wrapper.update/assert to your heart's content rather than within the timeout itself?
There was a problem hiding this comment.
Thanks for that, I'll use that example as a reference for another attempt. I had a go with the legacy timers, to no avail. Perhaps this will help. I'll ping you again when I have something ready for review :)
Codecov Report
@@ Coverage Diff @@
## master #1184 +/- ##
=======================================
Coverage 97.79% 97.80%
=======================================
Files 131 131
Lines 6677 6683 +6
Branches 1967 1968 +1
=======================================
+ Hits 6530 6536 +6
Misses 134 134
Partials 13 13
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Having to use real timers to simulate a hanging request still but the code is a little cleaner
packages/datagateway-download/src/downloadCart/downloadCartTable.component.test.tsx
Outdated
Show resolved
Hide resolved
|
As discussed - I'll likely be refactoring this again in #1185 so I'll take a look at the unit test again there |
Description
Once pressed, the remove all button is disabled and displays an indeterminate loading spinner until the request completes. This prevents excessive requests as the user was previously able to click this button several times, which could cause an error.
I found it difficult to get Jest's fake timers to work properly, which resulted in a unit test that isn't the best but works for now. I think this is due to multiple levels of promises trying to resolve and becoming conflicted. So anytime I try to use
jest.useFakeTimers()I get an exceeded timeout error. See my comment below which points out the test in question. Any feedback on how to properly test this would be appreciated!Testing instructions
This behaviour usually only presents itself when there are a large number of items in the cart. Try adding a lot of individual datafiles to the cart. This will hopefully allow for the new behaviour to display for long enough to see in action.
Agile board tracking
Closes #1177