Migrate Device transfer modal tests to Vue Testing Library#14563
Migrate Device transfer modal tests to Vue Testing Library#14563soaebhasan12 wants to merge 22 commits into
Conversation
|
👋 Hi @soaebhasan12, 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. |
|
|
closed by mistake... |
|
📢✨ Before we assign a reviewer, we'll turn on |
rtibblesbot
left a comment
There was a problem hiding this comment.
Clean migration — both files are free of @vue/test-utils, tests use semantic VTL queries, and the pattern matches CoreMenuOption.spec.js. CI is mostly pending but no failures.
- suggestion: count-only radio assertion in
SelectDriveModal(see inline) - suggestion: two removed tests leave a coverage gap for the primary selection interaction
- praise:
getByRole('button', { name: 'Continue' })+toBeDisabled()is good; stubs inSelectTransferSourceModalare well-reasoned
@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
Build Artifacts
Smoke test screenshot |
rtibblesbot
left a comment
There was a problem hiding this comment.
No new code changes since the prior review — only a develop merge commit that doesn't touch the test files. Re-raising two unaddressed suggestions below.
CI passing.
Prior findings
Unaddressed (re-raised below):
- Radio count doesn't verify which drives are shown (suggestion) — no reply, no code change
- Two coverage gaps: "Continue" enabled on selection, "Cancel" event (suggestion) — no reply, no code change
0/2 prior suggestions resolved. 2 re-raised below.
@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
- For UI changes: inspected screenshots for layout, visual completeness, and consistency
rtibblesbot
left a comment
There was a problem hiding this comment.
Prior suggestions addressed; one nitpick on the new commit.
CI passing. Phase 3 skipped — no UI files changed.
Prior findings
N prior findings unchanged (2 from initial review, still resolved).
Newly resolved:
- Radio count doesn't verify which drives are shown (suggestion) ← was UNADDRESSED —
getByTextassertion added atSelectDriveModal.spec.js:77 - Missing test for drive-selected enables Continue (suggestion) ← was UNADDRESSED — test added at
SelectDriveModal.spec.js:131
2/2 prior findings resolved. 0 re-raised below.
Findings:
- 1 nitpick (inline)
- 1 praise (inline)
@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
|
Hi @soaebhasan12! Looks like we have some conflicts! Could you please rebase and resolve them? |
rtibblesbot
left a comment
There was a problem hiding this comment.
No new commits since the prior review. Re-raising one unaddressed nitpick.
CI passing. Phase 3 skipped — no UI files changed.
Prior findings
3 prior findings unchanged (2 suggestions resolved, 1 nitpick re-raised).
Still unaddressed (re-raised below):
asyncon"Continue" button is disabledtest (nitpick) — no reply, no code change
@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
| cancelButton().trigger('click'); | ||
| await wrapper.vm.$nextTick(); | ||
| expect(wrapper.findComponent({ name: 'KModal' }).emitted().cancel).toHaveLength(1); | ||
| it('when no drive is selected, "Continue" button is disabled', async () => { |
There was a problem hiding this comment.
nitpick: async is still unnecessary — no await inside this test body. Remove it to match the sync pattern of the other simple assertions in this suite (e.g., lines 64, 96, 105, 114).
Hi @rtibbles, I have rebased and resolved the conflicts. The branch is now up to date with develop. |
rtibblesbot
left a comment
There was a problem hiding this comment.
All prior findings now resolved.
CI passing. Phase 3 skipped — no UI files changed.
Prior findings
2 prior findings unchanged (both suggestions resolved since review 3).
Newly resolved:
asyncon "Continue button disabled" test (nitpick) ← was UNADDRESSED — removed in commit75f1b32a7
3/3 prior findings resolved. None re-raised.
@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
|
Hi @soaebhasan12. we've just merged few new conventions & related linting rules to |
Hi @MisRob, I have run the linter and found two types of errors:
|
There was a problem hiding this comment.
HI @soaebhasan12! Great work on this pr this far! I've left a few comments for you to look into prior to being merged.
The main issue is the createTranslator exports added to source files — see inline comments.
Two other things:
- The
'clicking "Continue" triggers a "go forward" action'test was deleted. It tested thatgoForwardFromSelectDriveModalis called with the right parameters — that's real behavior worth keeping. - In
SelectTransferSourceModal, theSELECT_DRIVEtest only checks that the import source UI isn't shown. It doesn't confirm the drive modal actually rendered.
| import { TransferTypes } from 'kolibri-common/utils/syncTaskUtils'; | ||
| import DriveList from './DriveList'; | ||
|
|
||
| export const strings = createTranslator('SelectDriveModal', { |
There was a problem hiding this comment.
Same — no changes needed here. Use createTranslator(SelectDriveModal.name, SelectDriveModal.$trs) in the test.
There was a problem hiding this comment.
Fixed — removed the export const strings from SelectDriveModal.vue. Now using createTranslator(SelectDriveModal.name, SelectDriveModal.$trs) in the test.
| import { createTranslator } from 'kolibri/utils/i18n'; | ||
| import { ContentSources } from '../../../constants'; | ||
|
|
||
| export const strings = createTranslator('SelectImportSourceModal', { |
There was a problem hiding this comment.
Same — use createTranslator(SelectImportSourceModal.name, SelectImportSourceModal.$trs) in the test.
There was a problem hiding this comment.
Fixed — removed the export const strings from SelectImportSourceModal.vue. Now using createTranslator(SelectImportSourceModal.name, SelectImportSourceModal.$trs) in the test.
rtibblesbot
left a comment
There was a problem hiding this comment.
New commits after review 5's approval introduced a DRY violation in three source files, and two @akolson review concerns (May 20) remain unaddressed.
CI blocked on maintainer approval (action_required) — workflows haven't run on these commits yet. Phase 3 skipped — source file changes are script-only, no template or style changes.
Prior findings
All 3 prior findings unchanged (all resolved since review 5 APPROVE).
New findings:
- 1 blocking (inline, DriveList.vue)
- 2 suggestions (inline)
- 1 praise (inline)
@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
Hi @akolson, thanks for the review!
All 466 device tests pass. |
…s and add positive assertion
rtibblesbot
left a comment
There was a problem hiding this comment.
All three prior findings resolved in the new commits. Two new issues appeared in the same commits — the blocking fix was incomplete.
CI blocked on maintainer approval (action_required — workflows haven't run yet). Phase 3 skipped — no UI files changed.
Prior findings
Newly resolved:
- Exported translator strings duplicating
$trsin source files (blocking) ← exports removed in commitsfa53569,33132f3,07393e5 - Misleading drive ID
'WRITABLE_IMPORTABLE_drive'for unwritable drive (suggestion) ← fixed to'unwritable_drive'in2f567b7 SELECT_DRIVEtest missing positive assertion (suggestion) ←getByText(selectDrive$())added in3721cc2
3/3 prior findings resolved.
Findings:
- 2 blocking (inline — SelectDriveModal.vue:52, SelectImportSourceModal.vue:43)
- 1 nitpick (inline — SelectDriveModal.spec.js:152)
- 1 praise (inline — SelectDriveModal.spec.js:162)
@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
left a comment
There was a problem hiding this comment.
All 3 findings from review 9 resolved in commit 2c5b767.
CI blocked on maintainer approval (action_required) — not test failures. Phase 3 skipped — no UI files changed.
Praise: The commit that addressed review 9 made exactly the three changes requested and nothing else — no scope creep, no unrelated cleanups. Surgical fixes build reviewer trust.
Prior findings
Newly resolved:
- Unused
createTranslatorimport inSelectDriveModal.vue(blocking) ← removed in2c5b767 - Unused
createTranslatorimport inSelectImportSourceModal.vue(blocking) ← removed in2c5b767 - Non-English comment in
SelectDriveModal.spec.js(nitpick) ← removed in2c5b767
3/3 prior findings resolved. No new findings.
@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
|
📢✨ Before we assign a reviewer, we'll turn on |
rtibblesbot
left a comment
There was a problem hiding this comment.
Re-confirming the prior approval — the only change since review 10 (approved 2026-05-23 on commit 2c5b767) is a formatting-only pre-commit.ci commit (bb8ab9e8): trailing whitespace removal, newlines at file ends, and line-length reformatting of two createTranslator calls in the spec files. No logic changes.
CI shows action_required on all checks — workflows are pending maintainer approval to run on the fork branch, unchanged from the previous review cycle. Phase 3 skipped — no UI files changed.
praise: The final test suite consistently uses createTranslator(Component.name, Component.$trs) to derive translated strings from the component under test, rather than hard-coding English strings. This means any string change will break tests at the source, not silently.
Prior findings
All findings from prior rounds remain resolved — status unchanged from review 10.
0/0 new findings on the formatting commit.
@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
| cancelButton().trigger('click'); | ||
| await wrapper.vm.$nextTick(); | ||
| expect(wrapper.findComponent({ name: 'KModal' }).emitted().cancel).toHaveLength(1); | ||
| }); |
There was a problem hiding this comment.
What was the motivation for omitting this test? Based on the the cancel behavior, we could test that;
- a cancel event is emitted if in
manageMode. This is the current test behaviour resetContentWizardStateis called if not inmanageMode. An addition to consider
There was a problem hiding this comment.
Added two cancel tests:
- clicking "Cancel" emits a cancel event when in manageMode
- clicking "Cancel" calls resetContentWizardState when not in manageMode
| forExport: false, | ||
| }); | ||
| goForwardSpy.mockRestore(); | ||
| }); |
There was a problem hiding this comment.
Could we add another test that verifies that a submit event is emitted if manageMode is true. The above test only verifies the behavior if manageMode is false
There was a problem hiding this comment.
Added a test that verifies a submit event is emitted with driveId when manageMode is true.
akolson
left a comment
There was a problem hiding this comment.
Hi @soaebhasan12! Great work on this pr and thanks for your persistence on it--It's looking great.
I've left some comments for your considerations after which we should merge this pr.
Thank you!
akolson
left a comment
There was a problem hiding this comment.
Great work @soaebhasan12! LGTM! Thank you!
Thanks, @akolson I appreciate the review. |
|
Hi @soaebhasan12! Could you please rebase your branch onto the latest develop branch and push the updated changes? Thanks! |
Summary
Migrated two test files from
@vue/test-utilsto Vue Testing Libraryas described in issue #14263:
SelectDriveModal.spec.js— 9 tests migratedSelectTransferSourceModal.spec.js— 2 tests migratedChanges made:
mountwithrenderfrom@testing-library/vue(
getByText,queryByText,getAllByRole,getByRole,getByTestId)@vue/test-utilsimportsmakeWrapperasrenderComponentfollowing the patternin
CoreMenuOption.spec.jsVerified all tests pass:
References
Closes #14263
Reviewer guidance
Run the following command to verify all tests pass:
The main files changed are:
kolibri/plugins/device/frontend/views/__tests__/SelectDriveModal.spec.jskolibri/plugins/device/frontend/views/__tests__/SelectTransferSourceModal.spec.jsNo other files were modified.
AI usage
I used Claude (AI assistant) to support this contribution.
Disclose: I used Claude to understand Vue Testing Library concepts
and the difference between
@vue/test-utilsand VTL patterns.Engage critically: I did not use AI output directly. For each test,
I analyzed what the original test was asserting, then chose the
appropriate VTL query based on Testing Library's query priority
guidelines. For example:
getAllByRole('radio')instead of CSS selectors to count drivesgetByRole('button', { name: 'Continue' })instead offind('button[name="submit"]')getByTextfor visible text assertions instead ofcomponent-internal checks
Edit: I removed
makeWrapper,getElements, and unnecessary stubs.I also discovered that passing the Vuex store directly via
store:option worked correctly, while
global: { plugins: [store] }did not— I debugged this by reading error messages and testing different
approaches.
Process sharing: I referenced
CoreMenuOption.spec.jsas thepattern guide mentioned in the issue, and used Claude to clarify
concepts when stuck. All final decisions on test structure and
assertions were made by me after reading the Testing Library docs.
I reviewed every test carefully to make sure it reflects real user
interactions — for example, using
getByTextto find visible textinstead of CSS classes, and
getByRole('button')instead offind('button[name="submit"]').I ran the full test suite to verify nothing was broken.