Skip to content

[Remove Vuetify from Studio] Convert supplementary item unit tests to Vue Testing Library #5937

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.

Sign up for GitHub

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

Open
sharma-anushka wants to merge 2 commits into
base: unstable
Choose a base branch
from
+31 −34
Open

[Remove Vuetify from Studio] Convert supplementary item unit tests to Vue Testing Library #5937

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c4950a2
refactor: migrate supplementaryItem tests to Vue Testing Library
sharma-anushka May 24, 2026
9666906
Removed: uploadingHandler test
sharma-anushka May 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 31 additions & 34 deletions ...tion/contentcuration/frontend/channelEdit/views/files/__tests__/supplementaryItem.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,20 @@
import { mount } from '@vue/test-utils';
import { render, screen } from '@testing-library/vue';
import userEvent from '@testing-library/user-event';
import SupplementaryItem from '../supplementaryLists/SupplementaryItem';
import { factory } from '../../../store';
import Uploader from 'shared/views/files/Uploader';
import VueRouter from 'vue-router';
const router = new VueRouter();

function makeWrapper(props = {}) {
function renderComponent(props = {}) {
const store = factory();
return mount(SupplementaryItem, {
return render(SupplementaryItem, {
router,
store,
attachTo: document.body,
propsData: {
props: {
fileId: 'test',
presetID: 'video_subtitle',
...props
},
computed: {
file() {
Expand All @@ -28,48 +32,41 @@ function makeWrapper(props = {}) {
}

describe('supplementaryItem', () => {
let wrapper;

beforeEach(() => {
wrapper = makeWrapper();
});

it('setting readonly should disable uploading', async () => {
await wrapper.setProps({ readonly: true });
expect(wrapper.findComponent('[data-test="upload-file"]').exists()).toBe(false);
it('setting readonly should disable uploading', () => {
renderComponent({ readonly: true });
expect(screen.queryByTestId('upload-file')).not.toBeInTheDocument();
Copy link
Copy Markdown
Member

@akolson akolson May 28, 2026

Choose a reason for hiding this comment

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

screen.queryByTestId uses data-testid as its attribute by default, but the component uses data-test. Without configure({ testIdAttribute: 'data-test' }), this query never matches anything — the three not.toBeInTheDocument() assertions on lines 37, 43, and 63 are vacuously true and would pass even if the readonly and progress guards were completely removed from the component.

This is also why switching line 58 from document.querySelector to screen.queryByTestId (as rtibblesbot suggested) would still be broken without this fix.

Adding a top-level configure call right after the imports should resolve the issue:

import { render, screen, configure } from '@testing-library/vue';
configure({ testIdAttribute: 'data-test' });

});

it('setting readonly should disable removing', async () => {
await wrapper.setProps({ readonly: true });
expect(wrapper.findComponent('[data-test="remove"]').exists()).toBe(false);
it('setting readonly should disable removing', () => {
renderComponent({ readonly: true });
expect(screen.queryByTestId('remove')).not.toBeInTheDocument();
});

it('should call uploadingHandler when Uploader starts uploading file', async () => {
wrapper.findComponent(Uploader).vm.uploadingHandler({ id: 'file1' });
expect(wrapper.vm.fileUploadId).toBe('file1');
});
// Removed: uploadingHandler test - was implementation-detail focused
// and doesn't translate to VTL's user-behavior model. Covered by
// 'uploading should be true if progress < 1' test instead.
Copy link
Copy Markdown
Member

@akolson akolson May 28, 2026

Choose a reason for hiding this comment

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

This comment documents a decision that's already captured in the PR description. It adds noise here — the deletion is visible in the diff and the rationale belongs in the commit history. Consider removing it.


it('should call uploadCompleteHandler when Uploader finishes uploading file', async () => {
it('should call uploadCompleteHandler when Uploader finishes uploading file', () => {
const uploadCompleteHandler = jest.fn();
await wrapper.setProps({ uploadCompleteHandler });
wrapper.findComponent(Uploader).vm.uploadCompleteHandler({ id: 'file1' });
renderComponent({ uploadCompleteHandler });
uploadCompleteHandler({ id: 'file1' });
Copy link
Copy Markdown
Contributor

@rtibblesbot rtibblesbot May 25, 2026

Choose a reason for hiding this comment

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

blocking: This test is a tautology — it calls the mock directly (uploadCompleteHandler({ id: 'file1' })) and then asserts the mock was called. The component is not involved at all: removing :uploadCompleteHandler="uploadCompleteHandler" from SupplementaryItem.vue would not cause this test to fail.

Like the uploadingHandler test that was correctly removed, this test cannot be meaningfully migrated to VTL's user-behavior model (there's no user gesture that triggers upload completion). Remove it rather than keep dead coverage.

expect(uploadCompleteHandler).toHaveBeenCalledWith({ id: 'file1' });
});

it('uploading should be true if progress < 1', async () => {
expect(wrapper.findComponent('[data-test="uploading"]').exists()).toBe(false);
const testwrapper = makeWrapper({ progress: 0.5 });
expect(testwrapper.findComponent('[data-test="uploading"]').exists()).toBe(true);
it('uploading should be true if progress < 1', () => {
Copy link
Copy Markdown
Member

@akolson akolson May 28, 2026

Choose a reason for hiding this comment

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

'uploading should be true if progress < 1' describes an internal property name rather than what the user experiences. Something like 'shows an upload status indicator while a file is uploading' would better match the user-behavior framing the rest of the suite is aiming for.

renderComponent({ progress: 0.5 });
expect(document.querySelector('[data-test="uploading"]')).toBeInTheDocument();
Copy link
Copy Markdown
Contributor

@rtibblesbot rtibblesbot May 25, 2026

Choose a reason for hiding this comment

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

suggestion: Use screen.queryByTestId('uploading') instead of document.querySelector('[data-test="uploading"]'). The acceptance criteria require screen queries, and screen methods give better failure messages (they report what was in the DOM when the element wasn't found). Same pattern applies on line 69 — use screen.getByTestId('remove') as the click target.

Copy link
Copy Markdown
Contributor

@rtibblesbot rtibblesbot May 28, 2026

Choose a reason for hiding this comment

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

You're right — I missed that the component uses data-test while queryByTestId defaults to data-testid, so my suggestion would still be broken without the configure({ testIdAttribute: 'data-test' }) call. Thanks for catching that and providing the complete fix.

});

it('should disable ability to upload other files during a file upload', async () => {
const testwrapper = makeWrapper({ progress: 0.5 });
expect(testwrapper.findComponent('[data-test="upload-file"]').exists()).toBe(false);
it('should disable ability to upload other files during a file upload', () => {
renderComponent({ progress: 0.5 });
expect(screen.queryByTestId('upload-file')).not.toBeInTheDocument();
});

it('clicking remove button should emit remove event with file id', async () => {
const wrapper = makeWrapper({ id: 'test-remove' });
await wrapper.findComponent('[data-test="remove"]').trigger('click');
expect(wrapper.emitted('remove')[0][0]).toBe('test-remove');
const user = userEvent.setup();
Copy link
Copy Markdown
Contributor

@rtibblesbot rtibblesbot May 25, 2026

Choose a reason for hiding this comment

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

praise: Using userEvent.setup() + user.click() here is correct VTL practice — it dispatches real pointer and keyboard events rather than a synthetic trigger, giving more realistic interaction coverage.

const { emitted } = renderComponent({ id: 'test-remove' });
await user.click(document.querySelector('[data-test="remove"]'));
expect(emitted().remove[0][0]).toBe('test-remove');
});
});
Loading