tests: add tests for some components #12108
Conversation
| @@ -1,7 +1,7 @@ | |||
| <template> | |||
|
|
|||
| <KOptionalText | |||
| :text="seconds ? formattedTime : ''" | |||
| :text="seconds !== null ? formattedTime : ''" | |||
There was a problem hiding this comment.
Made this change as earlier passing 0 as seconds led to this expression being evaluated as false, and thus an empty string being displayed instead of 0 seconds. Also added a test case related to the same.
There was a problem hiding this comment.
I'm not very sure if the way I have chosen is the most user-centric way to test this FocusTrap component. Would appreciate any feedback on the same.
Build Artifacts
|
MisRob
requested review from
nucleogenesis,
LianaHarris360,
AlexVelezLl,
ozer550 and
AllanOXDi
kolibri/core/assets/src/views/__tests__/UserTypeDisplay.spec.js
Outdated
Show resolved
Hide resolved
| import VueRouter from 'vue-router'; | ||
| import FocusTrap from '../FocusTrap.vue'; | ||
|
|
||
| const renderComponent = (props = {}) => { |
There was a problem hiding this comment.
The FocusTrap tests look good.
Not a blocker -- the only thing I could think that might be worth adding is a call to the public reset method on FocusTrap to see that it updates isTrapActive and such.
There was a problem hiding this comment.
Got it! That's an important use case. Will add tests for it.
There was a problem hiding this comment.
I have added a test suite to cover this use-case, but I can't get it to work. Like to access the public method reset of the component, we would need to add a wrapper around it so that the user can access the same. I tried writing a test suite with the same, but can't get it to work. Would highly appreciate any help with the same.
There was a problem hiding this comment.
I think this is a case where the 'user' of the component is actually the developer, so I don't think it's necessary to do this via DOM interaction, and the reset method can be invoked programmatically - as it forms part of the public API of the component.
There was a problem hiding this comment.
Thanks @EshaanAgg! I just left two little comments but are non-blocking, just to reflect a little on the functionality we are testing, but everything else looks good to me! 👐.
There was a problem hiding this comment.
As @nucleogenesis didn't flag this as a blocker, I think we should just remove the reset method testing here, both the wrapper component for testing, and the test in the spec file itself.
Doing this will also fix the extant linting issues, so I think it's a win-win.
Signed-off-by: Eshaan Aggarwal <96648934+EshaanAgg@users.noreply.github.com>
|
Got it @rtibbles! Thanks for the review. I have updated to remove the |
|
Thank you @EshaanAgg - you responded so fast I was genuinely surprised! |
|
😂 |
|
Many things have been achieved by students trying to avoid what they are meant to be working on! |
Summary
Introduced a test suite for the following components:
UserTypeDisplay.vueFocusTrap.vueTimeDuration.vueReferences
Reviewer guidance
…
Testing checklist
PR process
Reviewer checklist
yarnandpip)