Comments

You ran pre-commit locally
All issue requirements are satisfied
The contribution is aligned with our Contributing guidelines. Pay extra attention to Using generative AI. Pull requests that don't follow the guidelines will be closed.

We'll be in touch! 😊

vtushar06

vtushar06 reviewed

Feb 10, 2026

View reviewed changes

...tion/contentcuration/frontend/channelList/views/ChannelSet/__tests__/channelSetModal.spec.js

expect(wrapper.vm.$route.name).toBe(RouteNames.CHANNEL_SETS);

});

expect(screen.getByText('Field is required')).toBeInTheDocument();

Copy link

Contributor

@vtushar06 vtushar06 Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assertion depends on the exact validation copy. If the message changes but the validation behavior stays the same, this test will fail unnecessarily.

My suggestion is to assert the validation state instead (for example via a semantic role).
expect(screen.getByRole('alert')).toBeInTheDocument();

vtushar06

vtushar06 reviewed

Feb 10, 2026

View reviewed changes

...tion/contentcuration/frontend/channelList/views/ChannelSet/__tests__/channelSetModal.spec.js

});

expect(await screen.findByRole('heading', { name: 'Unsaved changes' })).toBeInTheDocument();

expect(

screen.getByText('You will lose any unsaved changes. Are you sure you want to exit?'),

Copy link

Contributor

@vtushar06 vtushar06 Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same I wanted to apply here .
expect(screen.getByRole('dialog')).toBeInTheDocument();

@rtibbles rtibbles assigned AllanOXDi

Feb 10, 2026

AllanOXDi

AllanOXDi reviewed

Feb 13, 2026

View reviewed changes

Copy link

Member

@AllanOXDi AllanOXDi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @SheikhZaeem, I have left some comments.

...tion/contentcuration/frontend/channelList/views/ChannelSet/__tests__/channelSetModal.spec.js

it('should prompt user if there are unsaved changes', async () => {

expect(getUnsavedDialog(wrapper).exists()).toBeFalsy();

it('saves valid changes and returns the user to collections', async () => {

Copy link

Member

@AllanOXDi AllanOXDi Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this only checks that the route changes, it never asserts that updateChannelSet or add/removeChannels is invoked with the updated name. Think about where we navigate away without persisting would still pass. Consider asserting the action call, maybe?

...tion/contentcuration/frontend/channelList/views/ChannelSet/__tests__/channelSetModal.spec.js

expect(channelItems.at(1).html()).toContain('Channel 2');

expect(channelItems.at(1).html()).toContain('Second channel description');

expect(channelItems.at(1).find('button').text()).toBe('Remove');

it('shows a validation message when user tries to create without a name', async () => {

Copy link

Member

@AllanOXDi AllanOXDi Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only create-related check is the "required name" validation, so regressions in commitChannelSet/$router.replace could slip by unnoticed. Adding a success-case test would be ideal as well.

...tion/contentcuration/frontend/channelList/views/ChannelSet/__tests__/channelSetModal.spec.js

it("should render channels' names, descriptions and remove buttons", () => {

const channelItems = wrapper.findAllComponents({ name: 'ChannelItem' });

expect(await screen.findByRole('heading', { name: 'Select channels' })).toBeInTheDocument();

Copy link

Member

@AllanOXDi AllanOXDi Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also test for for channel items

@SheikhZaeem

Copy link

Author

SheikhZaeem commented Feb 16, 2026

Thanks for the reviews, I will make the necessary changes soon

SheikhZaeem added 2 commits

February 17, 2026 11:44

@SheikhZaeem


 convert collection modal tests to Vue Testing Library

21ca55b

@SheikhZaeem


 test: address review feedback in ChannelSetModal VTL tests

5abe5b7

@SheikhZaeem SheikhZaeem force-pushed the modal-vtl-test branch from b8adaee to 5abe5b7 Compare

February 17, 2026 22:16

@SheikhZaeem

Copy link

Author

SheikhZaeem commented Feb 17, 2026

@AllanOXDi I made the relevant changes according to your review, can you check and let me know.

Labels

None yet

3 participants

@SheikhZaeem @AllanOXDi @vtushar06

Comments

Conversation

@SheikhZaeem SheikhZaeem commented Feb 8, 2026

Summary

Ran

Uh oh!

learning-equality-bot bot commented Feb 8, 2026

Uh oh!

@vtushar06 vtushar06 Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

@vtushar06 vtushar06 Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

@AllanOXDi AllanOXDi left a comment

Choose a reason for hiding this comment

Uh oh!

@AllanOXDi AllanOXDi Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

@AllanOXDi AllanOXDi Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

@AllanOXDi AllanOXDi Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

SheikhZaeem commented Feb 16, 2026

Uh oh!

SheikhZaeem commented Feb 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants