-
Notifications
You must be signed in to change notification settings - Fork 280
[Remove Vuetify from Studio] Convert collection channels selection unit tests to Vue Testing Library #5664
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: unstable
Are you sure you want to change the base?
Conversation
…it tests to Vue Testing Library
|
👋 Thanks for contributing! We will assign a reviewer within the next two weeks. In the meantime, please ensure that:
We'll be in touch! 😊 |
|
Configuration: I configured VTL to use data-test (matching the Studio codebase convention) instead of the default data-testid, allowing us to use standard queries like getByTestId and remove document.querySelector. Isolation (Deselect Logic): I tested deselection by initializing the component with the channel already selected via props. This isolates the logic and avoids the complexity of manually simulating parent prop updates in a unit test. Scoping: I used within(row).getByRole('checkbox') to strictly scope the click interaction to the specific channel's container, ensuring the test targets the correct input and not a global match. |
|
Greetings @MisRob, implementation is complete and if any changes are required please let me know. Thanks |
| import { ChannelListTypes } from 'shared/constants'; | ||
|
|
||
| // Configure VTL to use 'data-test' instead of the default 'data-testid' | ||
| configure({ testIdAttribute: 'data-test' }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious about the configure({ testIdAttribute: 'data-test' }) configuration. This feels like a global configuration that should apply to all tests in the codebase, not just this one file.
Would it make more sense to move this to jest_config/setup.js? (@MisRob, I'm probably missing on the details of this refactor -- could you please confirm if this still fall within the scope of this issue? Moving this direction would mean migrating the few usages of data-testid to data-test as well)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @akolson. We'll be actually updating Studio, Kolibri, and KDS to use data-testid everywhere instead of data-test (confirmed in the Monday meet), so it'd be best to remove this configure and just use data-testid.
(and yes, if that wouldn't be the case, this kind of settings would make most sense globally - good catch)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@akolson @MisRob - I had used configure({ testIdAttribute: 'data-test' }) because the component itself uses data-test attributes (e.g., data-test="checkbox-${editChannel.id}" and data-test="channel-item-${editChannel.id}"), not the VTL default of data-testid.
I agree the changes need to be implemented. Thanks .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that's good thinking @abhiraj75 - we appreciate that you tried to follow the codebase conventions.
In any case, you can revert because of the added context :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Edit: This and below was my replies for a comment that was later deleted.
@abhiraj75 it's no problem to just update the component to use data-testid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd directly related to the migration to VTL - so in scope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MisRob, since the component currently uses data-test attributes ,should I keep the configure({ testIdAttribute: 'data-test' }) for now so the tests work with the existing component attributes ? Or proceed to remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abhiraj75 I already responded above to a comment that later disappeared. Remove configure please and update the component with data-testid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure @MisRob
akolson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HI @abhiraj75 Great job -- changes the tests LGTM! I have however left a comment for your action. Thanks
Refactored channelList/views/ChannelSet/tests/channelSelectionList.spec.js to use Vue Testing Library, rewriting tests around user-visible behavior (viewing channels, selecting via checkbox/card, and filtering via search) instead of component internals.
Manual verification
Ran:
pnpm test channelList/views/ChannelSet/tests/channelSelectionList.spec.js (from frontend)
References
Closes #5650
Parent: #5060
Reviewer guidance
Run:
pnpm test channelList/views/ChannelSet/tests/channelSelectionList.spec.js