Refactored the tests on fileUpoadItem.spec.js to Vue Testing Library …#6010
Refactored the tests on fileUpoadItem.spec.js to Vue Testing Library …#6010harrisonoakess wants to merge 2 commits into
Conversation
|
👋 Hi @harrisonoakess, thanks for contributing! For the review process to begin, please verify that the following is satisfied:
Also check that issue requirements are satisfied & you ran Pull requests that don't follow the guidelines will be closed. Reviewer assignment can take up to 2 weeks. |
|
📢✨ Before we assign a reviewer, we'll turn on |
rtibblesbot
left a comment
There was a problem hiding this comment.
Good VTL migration — correct imports, screen queries, jest-dom matchers, and userEvent throughout. One acceptance criterion is unmet (blocking), and the $trs pattern used elsewhere in the test suite isn't followed here.
Note: a pre-commit.ci lite auto-fix commit (8626be27) was pushed after the review passes ran and was not reviewed.
CI was pending at review time.
Findings:
- blocking: PR description is missing a screenshot or screen recording (acceptance criterion #2 from #5811). The note "Ran the docker instance and verified the upload process" describes intent but doesn't satisfy the criterion — add an image or recording of the Files section in action.
- important: Hard-coded English strings in
getByText(…)calls — useFileUploadItem.$trs.keyinstead (see inline comments). - suggestion:
getByRole('button')at line 105 has no name constraint — see inline comment. - suggestion:
getByTestId('list-item')at line 195 — prefer an accessible query. - praise: The
UploaderStubapproach (lines 111–152) correctly exercises theuploadCompleteHandlerguard via slot props rather than calling the handler directly — this catches a bug path the old test missed.
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran a phased review pipeline over the pull request diff:
- Classified the diff to select review passes (core, frontend, backend) and whether manual QA was required
- Core review pass checked correctness, design, architecture, testing, completeness, and DRY/SRP/Rule-of-Three principles
- Specialized frontend/backend review passes applied framework-specific lenses where those files changed
- For UI changes: manual QA and an accessibility audit against a live dev server, when available
- Checked CI status and linked issue acceptance criteria
- Synthesized one review from those passes and chose the verdict from the findings, CI status, and QA evidence
| original_filename: 'file', | ||
| }, | ||
| }); | ||
| expect(screen.getByText('Unknown filename')).toBeInTheDocument(); |
There was a problem hiding this comment.
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.
| url: 'file-url', | ||
| }, | ||
| }); | ||
| await user.click(screen.getByRole('button')); |
There was a problem hiding this comment.
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 }));| }, | ||
| }); | ||
|
|
||
| await user.click(screen.getByTestId('list-item')); |
There was a problem hiding this comment.
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));
Summary
Refactored channelEdit/views/files/tests/fileUploadItem.spec.js to use Vue Testing Library (VTL). Tests are rewritten to reflect how a user interacts with the file upload feature rather than testing implementation details.
References
Closes #5811
Sub-issue of #5789
Reviewer guidance
Ran the docker instance and verified the upload process.
Ran 'pnpm test fileUploadItem' and passed 1/1 test suites and 9/9 tests in 2.5 s
Code changes should not affect UI, these changes are only tests
The tests make sure the upload files section works correctly. channels --> my channels --> click on the channel card or create a new channel) --> Add --> Upload files
AI usage
I used Claude Code for some code generation and final review. For the code I wrote, I had Claude Code review. For the code that Claude wrote, I reviewed the code, made edits, and removed excessive lines.
I am new to open-source contribution, so let me know if you have any suggestions.