Convert trash modal unit tests to Vue Testing Library by Prashant-thakur77 · Pull Request #5816 · learningequality/studio

Prashant-thakur77 · 2026-04-07T11:19:52Z

Summary

Migrates trashModal.spec.js from @vue/test-utils to @testing-library/vue, as part of the broader effort to make Studio's frontend tests more user-centric and resilient to implementation changes.
Manual verification:

Logged in as a@a.com, opened a published channel, removed resources, and verified the trash modal behavior .
All the test cases passed.

Screencast.From.2026-04-07.16-36-39.webm

References

Closes #5790
See #5789

Reviewer guidance

1)Only trashModal.spec.js was changed.
2) Reviewer need to check if all the needed testcases are present and testing-library is properly used.

AI usage

I used Claude to help plan and implement the migration.I checked other Testing-library implementations and verified the tests written and updated wherever needed.I followed the guidance given in the issue description and its parent issue description.I again checked if all the test cases are passing properly.

learning-equality-bot · 2026-04-07T11:20:27Z

👋 Hi @Prashant-thakur77, thanks for contributing!

For the review process to begin, please verify that the following is satisfied:

Contribution is aligned with our contributing guidelines
Pull request description has correctly filled AI usage section & follows our AI guidance:
AI guidance

State explicitly whether you didn't use or used AI & how.

If you used it, ensure that the PR is aligned with Using AI as well as our DEEP framework. DEEP asks you:
- Disclose — Be open about when you've used AI for support.
- Engage critically — Question what is generated. Review code for correctness and unnecessary complexity.
- Edit — Review and refine AI output. Remove unnecessary code and verify it still works after your edits.
- Process sharing — Explain how you used the AI so others can learn.
Examples of good disclosures:

"I used Claude Code to implement the component, prompting it to follow the pattern in ComponentX. I reviewed the generated code, removed unnecessary error handling, and verified the tests pass."

"I brainstormed the approach with Gemini, then had it write failing tests for the feature. After reviewing the tests, I used Claude Code to generate the implementation. I refactored the output to reduce verbosity and ran the full test suite."

Also check that issue requirements are satisfied & you ran pre-commit locally.

Pull requests that don't follow the guidelines will be closed.

Reviewer assignment can take up to 2 weeks.

Prashant-thakur77 · 2026-04-07T11:20:39Z

Hlo @MisRob ,I have resolved the issue,do review when you have time.

learning-equality-bot · 2026-04-11T05:35:33Z

📢✨ Before we assign a reviewer, we'll turn on @rtibblesbot to pre-review. Its comments are generated by an LLM, and should be evaluated accordingly.

learning-equality-bot · 2026-04-11T05:35:55Z

📢✨ Before we assign a reviewer, we'll invite community pre-review. See the community review guidance for both authors and reviewers.

AllanOXDi

Hi @Prashant-thakur77, thanks for working on this. I left a few clean up

AllanOXDi · 2026-04-15T15:39:00Z

+    .mockResolvedValue({});
+  jest.spyOn(TrashModal.methods, 'loadAncestors').mockResolvedValue();
+  jest.spyOn(TrashModal.methods, 'removeContentNodes').mockResolvedValue();
+  const loadNodesSpy = jest.spyOn(TrashModal.methods, 'loadNodes');


This works but is non-obvious; add a comment explaining that real execution is intentional.

AllanOXDi · 2026-04-15T15:41:55Z

+    jest.spyOn(TrashModal.methods, 'loadChildren').mockReturnValue(new Promise(() => {}));
+  } else {
+    jest.spyOn(TrashModal.methods, 'loadChildren').mockResolvedValue({
+      more: items === testChildren ? null : { parent: TRASH_ID, page: 2 },


using (===) to decide pagination is not quite right "show more unless the exact same array object was passed." Use an explicit hasMore parameter instead.

AllanOXDi · 2026-04-15T17:13:28Z

-      expect(wrapper.vm.selected).toEqual([]);
-      expect(wrapper.vm.previewNodeId).toBe(null);
+      await waitFor(() => {
+        expect(loadContentNodesSpy).toHaveBeenCalledWith({ parent: TRASH_ID, page: 2 });


The assertion uses toHaveBeenCalledWith, which passes if any call matches, so the initial call and the "Show more" call are both included. This passes correctly but could be more precise with toHaveBeenLastCalledWith to explicitly assert the pagination call was the most recent one.

AllanOXDi · 2026-04-15T17:15:27Z

+
+      await user.click(showMoreBtn);

-    it('moveNoves should clear selected and previewNodeId', async () => {


The original had a test for clicking an item, setting previewNodeId (which opens ResourceDrawer). This has been dropped entirely. It should either be added back or explicitly noted in the PR description as an intentional omission.

I’ve restored the missing coverage in line with Testing Library patterns:

Reintroduced the test that verifies clicking an item sets previewNodeId and opens the ResourceDrawer.

Reworked the previous moveNodes test to follow user interactions instead of directly invoking wrapper.vm. The test now selects an item, opens the preview drawer, completes the “Move here” flow, and asserts that selections are cleared and the drawer is closed.
What are your thoughts on this apporach @AllanOXDi .

AllanOXDi · 2026-04-15T17:18:33Z

 import { RouteNames } from '../../../constants';
+import TrashModal from '../TrashModal';

 const store = factory();


Move inside makeWrapper so each test gets a clean store.

AllanOXDi · 2026-04-15T17:25:44Z

+  const utils = render(
+    TrashModal,
+    {
+      store,


Passing the shared module-level store into render. See line 9, the shared store's dispatch spy in the deletion test ( see line 200) may not be fully cleaned up before subsequent tests run.

Fixed as part of moving store into makeWrapper

AllanOXDi · 2026-04-15T17:28:36Z

+      expect(screen.getByTestId('delete')).toBeDisabled();
+      expect(screen.getByTestId('restore')).toBeDisabled();
+
+      await user.click(screen.getAllByRole('checkbox')[1]);


A bit brittle. [1] assumes the second checkbox in DOM order is the first item checkbox (index 0 being select-all). If any component renders an additional checkbox before the list, this clicks the wrong element. Use screen.getAllByTestId('checkbox')[0] instead.

Done. Changed to within(screen.getAllByTestId('checkbox')[0]).getByRole('checkbox').
Note: data-test="checkbox" is on the k-checkbox-container div, not the input, so within() is needed to reach the actual checkbox input for clicking.

AllanOXDi · 2026-04-15T17:36:52Z

+
+      await waitFor(() => {
+        screen
+          .getAllByRole('checkbox')


Relies on the select-all being first in DOM order. Works today but fragile for the same reason as line 117.

AllanOXDi · 2026-04-15T17:38:28Z

+
+    it('successful deletion triggers snackbar and reloads nodes', async () => {
+      jest.spyOn(TrashModal.methods, 'deleteContentNodes').mockResolvedValue();
+      const dispatchSpy = jest.spyOn(store, 'dispatch').mockImplementation(() => Promise.resolve());


Since makeWrapper is called after this line, the spy is active during mount. But because the store is shared (line 9), this mock intercepts dispatch calls in other tests if cleanup races. The fix is again to move store into makeWrapper.

Prashant-thakur77 · 2026-04-16T11:02:06Z

Hlo @AllanOXDi I have done the requested changes,do look at them when you have time .Please do tell if any other changes are required.

learning-equality-bot · 2026-05-18T15:40:53Z

📢✨ Before we assign a reviewer, we'll turn on @rtibblesbot to pre-review. Its comments are generated by an LLM, and should be evaluated accordingly.

rtibblesbot

Solid migration — the tests correctly verify observable behavior throughout, and CI is passing.

No UI files changed — Phase 3 skipped.

suggestion: getByTestId used for buttons with accessible names — inline comment at the first occurrence
praise: two inline comments

@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly

How was this generated?

Reviewed the pull request diff checking for:

Correctness: bugs, edge cases, undocumented behavior, resource leaks, hardcoded values
Design: unnecessary complexity, naming, readability, comment accuracy, redundant state
Architecture: duplicated concerns, minimal interfaces, composition over inheritance
Testing: behavior-based assertions, mocks only at hard boundaries, accurate coverage
Completeness: missing dependencies, unupdated usages, i18n, accessibility, security
Principles: DRY (same reason to change), SRP, Rule of Three (no premature abstraction)
Checked CI status and linked issue acceptance criteria
For UI changes: inspected screenshots for layout, visual completeness, and consistency

rtibblesbot · 2026-05-18T15:47:20Z

+    it('checking an item enables the Delete and Restore buttons', async () => {
+      const { user } = await makeWrapper();
+
+      expect(screen.getByTestId('delete')).toBeDisabled();


suggestion: The acceptance criteria ask to follow the Testing Library query priority, which puts getByRole above getByTestId. The Delete and Restore buttons have accessible text labels, so prefer:

expect(screen.getByRole('button', { name: /Delete/i })).toBeDisabled(); expect(screen.getByRole('button', { name: /Restore/i })).toBeDisabled();

The same applies wherever getByTestId('delete'), getByTestId('restore'), getByTestId('selectall'), and findByTestId('close') appear throughout the file. getByTestId remains appropriate for structural/state containers (loading, empty, list) where no accessible role exists.

rtibblesbot

Prior findings

Resolved:

getByTestId('delete') / getByTestId('restore') for labeled buttons (suggestion) — replaced with getByRole('button', { name: /Delete/i }) and getByRole('button', { name: /Restore/i }) throughout; getByTestId('selectall') direct clicks also updated to within(getByTestId('selectall')).getByRole('checkbox') across all test flows.

1/1 prior findings resolved. 0 re-raised.

CI passing. No UI files changed — Phase 3 skipped.

praise: inline comment on the getByRole changes

@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly

How was this generated?

Compared the current PR state against findings from a prior review:

Retrieved prior bot reviews deterministically via the GitHub API
Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
Only raised NEW findings for newly introduced code
Reviewed the pull request diff checking for correctness, design, architecture, testing, completeness, and adherence to DRY/SRP principles
Checked CI status and linked issue acceptance criteria

rtibblesbot · 2026-05-19T16:54:53Z

+    it('checking an item enables the Delete and Restore buttons', async () => {
+      const { user } = await makeWrapper();
+
+      expect(screen.getByRole('button', { name: /Delete/i })).toBeDisabled();


praise: The getByRole migration is thorough and consistent — every getByTestId('delete'), getByTestId('restore'), and direct getByTestId('selectall') click across all six affected tests has been updated. The remaining getByTestId usages (loading, empty, list, checkbox container wrappers, item links, close icon button) are all cases where no accessible role or label is available, so testId is the appropriate fallback per Testing Library's query priority.

rtibbles assigned AllanOXDi Apr 7, 2026

Prashant-thakur77 force-pushed the trashmodalunit-test branch from 08ea025 to 90f477b Compare April 7, 2026 18:56

AllanOXDi assigned rtibblesbot Apr 8, 2026

MisRob unassigned rtibblesbot Apr 11, 2026

MisRob requested review from AllanOXDi and rtibblesbot April 11, 2026 05:34

MisRob added the community-review label Apr 11, 2026

AllanOXDi suggested changes Apr 15, 2026

View reviewed changes

Prashant-thakur77 added 9 commits April 16, 2026 15:48

test: Made changes to trashModal test file to use vue testing library

a95ff09

Updated the test file

cfb8bf8

Updated test file again

19ab7bc

Added necessary stubs

95e1bdf

Finalizing

b46e073

Updated test file

378faf9

Updated test file

fa90400

Refined testcases

03c1662

Updated the test based on reviwer feedback

812d4fc

Prashant-thakur77 force-pushed the trashmodalunit-test branch from 90f477b to 812d4fc Compare April 16, 2026 11:00

MisRob changed the title ~~Trashmodalunit test~~ Convert trash modal unit tests to Vue Testing Library Apr 17, 2026

Prashant-thakur77 requested a review from AllanOXDi April 19, 2026 13:07

rtibbles assigned rtibbles and unassigned AllanOXDi May 7, 2026

akolson requested review from rtibblesbot and removed request for AllanOXDi and rtibblesbot May 18, 2026 15:39

rtibblesbot reviewed May 18, 2026

View reviewed changes

replaced getbytestid wiht getbyrole

3670d7e

rtibblesbot approved these changes May 19, 2026

View reviewed changes


		await user.click(showMoreBtn);

		it('moveNoves should clear selected and previewNodeId', async () => {

Uh oh!

Conversation

Prashant-thakur77 commented Apr 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

References

Reviewer guidance

AI usage

Uh oh!

learning-equality-bot Bot commented Apr 7, 2026

Uh oh!

Prashant-thakur77 commented Apr 7, 2026

Uh oh!

learning-equality-bot Bot commented Apr 11, 2026

Uh oh!

learning-equality-bot Bot commented Apr 11, 2026

Uh oh!

AllanOXDi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AllanOXDi Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Prashant-thakur77 Apr 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Prashant-thakur77 Apr 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Prashant-thakur77 commented Apr 16, 2026

Uh oh!

learning-equality-bot Bot commented May 18, 2026

Uh oh!

rtibblesbot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rtibblesbot left a comment

Choose a reason for hiding this comment

Prior findings

Uh oh!

Choose a reason for hiding this comment

Prashant-thakur77 commented Apr 7, 2026 •

edited

Loading

AllanOXDi Apr 15, 2026 •

edited

Loading

Prashant-thakur77 Apr 16, 2026 •

edited

Loading

Prashant-thakur77 Apr 16, 2026 •

edited

Loading