Skip to content

Refactored the tests on fileUpoadItem.spec.js to Vue Testing Library … #6010

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
harrisonoakess wants to merge 5 commits into
base: unstable
Choose a base branch
from
+148 −57
Open

Refactored the tests on fileUpoadItem.spec.js to Vue Testing Library … #6010

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cea1678
Refactored the tests on fileUpoadItem.spec.js to Vue Testing Library …
harrisonoakess Jun 29, 2026
8626be2
[pre-commit.ci lite] apply automatic fixes
pre-commit-ci-lite[bot] Jun 30, 2026
4520e3f
Created a translator with createTranslator for tests instead of using…
harrisonoakess Jul 17, 2026
5cf419a
fixed the file so it does not use any stubs
harrisonoakess Aug 11, 2026
901cac8
Restore mocks in afterEach instread of at the end of the test per rti…
harrisonoakess Aug 11, 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
213 changes: 155 additions & 58 deletions ...uration/contentcuration/frontend/channelEdit/views/files/__tests__/fileUploadItem.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,18 @@
import { mount } from '@vue/test-utils';
import { render, screen, configure } from '@testing-library/vue';
import userEvent from '@testing-library/user-event';
import FileUploadItem from '../FileUploadItem';
import { factory } from '../../../store';
import Uploader from 'shared/views/files/Uploader';
import { fileErrors } from 'shared/constants';

const testFile = { id: 'test' };
function makeWrapper(props = {}, file = {}, computed = {}) {
const store = factory();
return mount(FileUploadItem, {
configure({ testIdAttribute: 'data-test' });

function renderComponent({ props = {}, file = {}, store = factory(), stubs = {} } = {}) {
return render(FileUploadItem, {
routes: [],
store,
attachTo: document.body,
propsData: {
stubs,
props: {
file:
file === null
? null
Expand All @@ -24,81 +27,175 @@ function makeWrapper(props = {}, file = {}, computed = {}) {
},
...props,
},
computed,
});
}

describe('fileUploadItem', () => {
describe('render', () => {

@rtibblesbot rtibblesbot Jul 17, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Resolved — addressed in the current code.

nitpick: The migration folds the interaction cases (opening the options menu, clicking the file row) under describe('render'), whereas the old file split them into a separate describe('methods') block. Harmless and arguably in keeping with VTL's behavior-first grouping — leave it, or rename this block to something like describe('file upload item').

it("'Unknown filename' should be displayed if original_filename is 'file'", () => {
const file = {
original_filename: 'file',
};
const wrapper = makeWrapper({}, file);
expect(wrapper.findComponent('[data-test="file-name"]').text()).toBe('Unknown filename');
it('shows "Unknown filename" when the uploaded file has a generic name', () => {
renderComponent({
file: {
original_filename: 'file',
},
});
expect(screen.getByText('Unknown filename')).toBeInTheDocument();

@rtibblesbot rtibblesbot Jun 30, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Resolved — addressed in the current code.

important: The established repo pattern (see HintsEditor.spec.js) is to reference ComponentName.$trs.key in text queries rather than hard-coding English strings. FileUploadItem exposes these keys in its $trs object:

// instead of:
expect(screen.getByText('Unknown filename')).toBeInTheDocument();

// use:
expect(screen.getByText(FileUploadItem.$trs.unknownFile)).toBeInTheDocument();

Same applies to 'Upload failed' (line 82), 'Select file' (line 89), 'Replace file' / 'Download' / 'Remove' (lines 106–108). This way tests survive wording changes without breaking.

@akolson akolson Jul 1, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please see https://kolibri-dev.readthedocs.io/en/develop/frontend_architecture/unit_testing.html for testing guidelines.

@rtibblesbot rtibblesbot Jul 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @akolson — those guidelines line up with the intent here: avoid hard-coding translated strings so tests survive wording changes. One refinement from the docs worth noting for @harrisonoakess: for the in-component $trs pattern the docs suggest building a translator with createTranslator in the test rather than reaching into Component.$trs.key directly. Either way, the key point stands — query by the translation key, not the literal English.

});

it("'Unknown filename' should be displayed if original_filename is ''", () => {
const file = {
original_filename: '',
};
const wrapper = makeWrapper({}, file);
expect(wrapper.findComponent('[data-test="file-name"]').text()).toBe('Unknown filename');
it("shows 'Unknown filename' when the uploaded filename is ''", () => {
renderComponent({
file: {
original_filename: '',
},
});
expect(screen.getByText('Unknown filename')).toBeInTheDocument();
});

it("original_filename should be displayed if its value is not 'file'", () => {
const file = {
it('shows the uploaded file name when it is available', () => {
renderComponent({
file: {
original_filename: 'SomeFileName',
},
});
expect(screen.getByText('SomeFileName')).toBeInTheDocument();
});

it('shows an upload error when the file upload failed', () => {
const store = factory();
store.commit('file/ADD_FILE', {
id: 'file-1',
original_filename: 'SomeFileName',
};
const wrapper = makeWrapper({}, file);
expect(wrapper.findComponent('[data-test="file-name"]').text()).toBe('SomeFileName');
preset: 'document',
checksum: 'checksum',
file_format: 'pdf',
loaded: 0,
total: 100,
error: fileErrors.UPLOAD_FAILED,
});
renderComponent({
store,
file: {
id: 'file-1',
original_filename: 'SomeFileName',
error: fileErrors.UPLOAD_FAILED,
},
});
expect(screen.getByText('Upload failed')).toBeInTheDocument();
});

it('should show a status error if the file has an error', () => {
const wrapper = makeWrapper({}, { error: true });
expect(wrapper.findComponent('[data-test="status"]').exists()).toBe(true);
it('shows a Select file action when no file has been uploaded', () => {
renderComponent({
file: null,
});
expect(screen.getByText('Select file')).toBeInTheDocument();
});

it('should show an upload button if file is null', () => {
const wrapper = makeWrapper({}, null);
expect(wrapper.findComponent('[data-test="upload-link"]').exists()).toBe(true);
expect(wrapper.findComponent('[data-test="radio"]').exists()).toBe(false);
it('shows file actions when the user opens the options menu', async () => {
const user = userEvent.setup();
renderComponent({
props: {
allowFileRemove: true,
},
file: {
id: 'file-1',
original_filename: 'SomeFileName',
file_size: 100,
url: 'file-url',
},
});
await user.click(screen.getByRole('button'));

@rtibblesbot rtibblesbot Jun 30, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Resolved — addressed in the current code.

suggestion: getByRole('button') without a { name: … } constraint will throw if the component or any child ever adds a second button in this state. Add an accessible name (or aria-label on the icon button in the component source):

await user.click(screen.getByRole('button', { name: /options/i }));

expect(screen.getByText('Replace file')).toBeInTheDocument();
expect(screen.getByText('Download')).toBeInTheDocument();
expect(screen.getByText('Remove')).toBeInTheDocument();
});
it('should show dropdown on click preview file options', async () => {
const wrapper = makeWrapper({ allowFileRemove: true });
await wrapper.findComponent('[data-test="show-file-options"]').trigger('click');
expect(wrapper.find('[data-test="file-options"]').isVisible()).toBe(true);

it('calls the upload complete handler when the replacement upload finishes', async () => {
const user = userEvent.setup();
const uploadCompleteHandler = jest.fn();

const UploaderStub = {
name: 'Uploader',
props: ['uploadingHandler', 'uploadCompleteHandler'],
methods: {
openFileDialog() {},
handleFiles() {},
},
template: `
<div>
<button type="button" @click="uploadingHandler({ id: 'file-1' })">
Start upload
</button>
<button type="button" @click="uploadCompleteHandler({ id: 'file-1' })">
Finish upload
</button>
<slot :openFileDialog="openFileDialog" :handleFiles="handleFiles" />
</div>
`,
};

renderComponent({
props: {
uploadCompleteHandler,
},
stubs: {
Uploader: UploaderStub,

@akolson akolson Jul 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Use of stubs generally discouraged, as it could mask bugs as detailed here

},
});

await user.click(screen.getByRole('button', { name: 'Start upload' }));
await user.click(screen.getByRole('button', { name: 'Finish upload' }));

expect(uploadCompleteHandler).toHaveBeenCalledWith(
expect.objectContaining({
id: 'file-1',
}),
);
});
});

describe('methods', () => {
let wrapper;
it('selects the existing file when the user clicks the file row', async () => {
const user = userEvent.setup();
const { emitted } = renderComponent({
file: {
id: 'file-1',
original_filename: 'SomeFileName',
file_size: 100,
},
});

beforeEach(() => {
wrapper = makeWrapper();
await user.click(screen.getByText('SomeFileName'));

expect(emitted().selected).toHaveLength(1);
});

it('Uploader uploadCompleteHandler should call uploadCompleteHandler with file', async () => {
const file = {
id: 'file-1',
it('opens the file chooser when the user clicks an empty file row', async () => {
const user = userEvent.setup();
const openFileDialog = jest.fn();

const UploaderStub = {
name: 'Uploader',
methods: {
openFileDialog() {
openFileDialog();
},
handleFiles() {},
},
template: `
<div>
<slot :openFileDialog="openFileDialog" :handleFiles="handleFiles" />
</div>
`,
};
const uploadCompleteHandler = jest.fn();
await wrapper.setProps({ uploadCompleteHandler });
await wrapper.setData({ fileUploadId: file.id });
wrapper.findComponent(Uploader).vm.uploadCompleteHandler(file);
expect(uploadCompleteHandler).toHaveBeenCalledWith(file);
});

it('clicking a list item should emit a selected event if a file is available', async () => {
await wrapper.find('[data-test="list-item"]').trigger('click');
expect(wrapper.emitted('selected')).not.toBeUndefined();
});
const { emitted } = renderComponent({
file: null,
stubs: {
Uploader: UploaderStub,

@akolson akolson Jul 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

https://github.com/learningequality/studio/pull/6010/changes#r3656938168

},
});

await user.click(screen.getByTestId('list-item'));

@rtibblesbot rtibblesbot Jun 30, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Resolved — addressed in the current code.

suggestion: getByTestId is a last resort in VTL. In this render state the component shows a 'Select file' action (line 89 uses the same text) — prefer the visible element:

await user.click(screen.getByText(FileUploadItem.$trs.uploadButton));


it('clicking a list item should open the file dialog if file is not available', async () => {
wrapper = makeWrapper({}, null);
await wrapper.find('[data-test="list-item"]').trigger('click');
expect(wrapper.emitted('selected')).toBeUndefined();
expect(openFileDialog).toHaveBeenCalled();
expect(emitted()).not.toHaveProperty('selected');
});
});
});
Loading