Test DISCOVERY-437 (bonus: smoke test viewScanList!) #518
Conversation
Also add the missing attribute "report_id" to type "scanJob".
The button shall only be enabled when the scan is completed. Also touch showScansModal to use the helper 'canDownloadReport' to decide if the report can be downloaded. Relates to JIRA: DISCOVERY-437
For reasons the author of this commit doesn't understand, ts-jest won't work with '@mturley-latest/react-table-batteries', which is used on all views. But it worked just fine with babel-jest. So now we have this frankenstein config, using both.
Add a sanity check testing viewScanList and the behavior of the kebab button; Properly test DISCOVERY-437; given how tighly coupled everything is, adding a view test was the only way to test it. Relates to JIRA: DISCOVERY-437
bruno-fs
force-pushed
the
testing-disco-437
branch
from
6c5a0af to
bc90e2d
Compare
|
@bruno-fs the primary views were disabled from testing for multiple internal reasons discussed with @nicolearagao around how to architect UI code. Any future "view" code actively needs to be broken apart into subsequent components, helpers, etc... before attempting to shoe-horn a testing solution into place. Some of the "many" future breakouts that need to happen include
...performing these types of updates will subsequently make testing the views easier. I recommend closing this PR down in the short term |
|
That's an interesting point, @cdcabrera. While I agree the views (or at least this one) desperately need a refactor that makes them more decoupled - and at the very minimum easier to test, I don't think testing (at least smoke testing) should be postponed. Waiting to test later is a liability and should be a team decision, not up to just you and @nicolearagao. |
mirekdlugosz
force-pushed
the
disco-437
branch
from
04965c7 to
2ea7278
Compare
|
The changes you are proposing here will
The proper approach is still recommended, understanding the ui architecture. Any issues that arise from merging this work fall outside of my volunteer effort |
|
@mirekdlugosz it seems it won't be possible to test download functionality from scans page on this codebase. I wrote this one because of your previous comment about having download functionality already covered on camayoc, but on the scan jobs modal... Do you have a suggestion on how to test this while the refactor doesn't happen? |
|
@bruno-fs I think we can cover that on Camayoc side. I put it on the list of things to do, but I was constantly busy with other UI work and now we have this rapidast integration on higher priority. If you would want to try to implement it, it would look something like that: Step one: Create a new test similar to It probably shouldn't have In test you wouldn't use Also: right now only new UI gives two ways to download report, so this new test should run conditionally only if Step two: Add a new method to Unless you are feeling adventurous and want to implement generic kebab interaction in
So test / page method could do something like When it comes to download specifically, we want longer longer timeout than default Playwright 30 seconds, and we manually refresh page on each step (I measured that the other day, manually refreshing gives much shorter tests execution time than waiting for UI to refresh automatically). You can copy the loop from existing implementation, although I guess there is an opportunity to abstract and re-use code. |
| it('ensure only the first scan has a downloadable report', async () => { | ||
| // see dummyScanList; only the first scan should have a downloadable report | ||
| // grab the download button for the first scan | ||
| const { downloadBtn } = clickKebabMenu(downloadableScanName); | ||
| // the button should not be disabled | ||
| expect(downloadBtn.hasAttribute('disabled')).toBeFalsy(); | ||
| // and clicking on it should... | ||
| act(() => { | ||
| fireEvent.click(downloadBtn); | ||
| }); | ||
| // ...download the report | ||
| expect(mockedDownloadReport).toHaveBeenCalledWith(downloadableReportId); | ||
| mockedDownloadReport.mockReset(); | ||
| // repeat the process for other scans, which should not be downloadable | ||
| const otherScans = [1, 2, 3, 4]; | ||
| otherScans.forEach(n => { | ||
| const { downloadBtn: button } = clickKebabMenu(`not downloadable ${n}`); | ||
| expect(button.hasAttribute('disabled')).toBeTruthy(); | ||
| act(() => { | ||
| fireEvent.click(downloadBtn); | ||
| }); | ||
| expect(mockedDownloadReport).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
@cdcabrera just to be sure we are on the same page, if I remove all the proposed tests but this one (this was the true goal of testing here), would you still be against this PR?
|
@bruno-fs just FYI, covering both paths to download report (kebab menu and modal) is introduced in quipucords/camayoc#557 |
What's included
Add a sanity check testing viewScanList and the behavior of the kebab
button;
Properly test DISCOVERY-437; given how tighly coupled everything is,
adding a view test was the only way to test it.
Notes
changing jest config and adding babel as transformer was required to be able to test those views - setting up those configs was way more challenging than actually writing those tests 😅
How to test
npm test
Coverage and basic unit test check
$ npm install$ npm test-->
Example
...
Updates issue/story
DISCOVERY-437