refactor TaskPanel and PinAuthenticationModal to use vue test library#14393
refactor TaskPanel and PinAuthenticationModal to use vue test library#14393curiouscoder-cmd wants to merge 2 commits intolearningequality:developfrom
Conversation
github-actions
bot
added
APP: Device
|
👋 Thanks for contributing! We will assign a reviewer within the next two weeks. In the meantime, please ensure that:
We'll be in touch! 😊 |
Build Artifacts
Smoke test screenshot |
|
I notice that old tests were using I don't know if it's the preffered way , or let me know I will do as preferred. Thanks! |
|
📢✨ Before we assign a reviewer, we'll turn on |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Clean migration from @vue/test-utils snapshots to @testing-library/vue behavioral assertions. CI passing.
- praise: Snapshot removal in TaskPanel replaced with explicit, readable assertions that document actual component behavior
- praise: PinAuthenticationModal tests use
userEvent.type+screen.getByLabelTextfor realistic user interaction - suggestion: Minor — see inline comment about unused
percentagefield in TaskPanel fixture
@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
| type: TaskTypes.DISKCONTENTEXPORT, | ||
| status: 'CANCELED', | ||
| clearable: true, | ||
| percentage: 1.0, |
There was a problem hiding this comment.
suggestion: percentage: 1.0 was added to the fixture but isn't exercised by any assertion — the progress bar only renders when taskIsRunning (status RUNNING), so this field has no effect on the canceled-task output. Consider removing it to keep the fixture minimal, or if it's intentional, add a brief comment explaining why it's included.
| expect(wrapper.emitted().submit).toEqual([[]]); | ||
| assertTextbox(wrapper, '', false); | ||
| }); | ||
| it('emits submit when the user enters a valid PIN and submits', async () => { |
There was a problem hiding this comment.
praise: Good pattern — using emitted() from the render return and waitFor for the async PIN validation flow is cleaner and more idiomatic than the old .then() chains with wrapper.vm access.
| expect(screen.getByText(/started by 'tester'/i)).toBeInTheDocument(); | ||
| expect(screen.getByRole('button', { name: /clear/i })).toBeInTheDocument(); | ||
|
|
||
| expect(screen.getByText(/500 resources/i)).toBeInTheDocument(); |
There was a problem hiding this comment.
praise: Asserting that resource totals are shown for canceled exports is more honest than the old snapshot, which had a misleading comment ("File size/resource numbers should not be shown for canceled exports") contradicted by its own snapshot output.
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Clean test migration — prior suggestion resolved, no new issues.
Prior findings
Resolved:
- Unused
percentagefield in TaskPanel fixture (suggestion) — removed in9ed00bc16a
1/1 prior findings resolved.
CI passing. Test-only changes — Phase 3 skipped.
- praise: See inline comment on TaskPanel assertions
@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
| expect(screen.getByRole('button', { name: /clear/i })).toBeInTheDocument(); | ||
|
|
||
| expect(screen.getByText(/500 resources/i)).toBeInTheDocument(); | ||
| expect(screen.getByText(/\(5 KB\)/i)).toBeInTheDocument(); |
There was a problem hiding this comment.
praise: Good verification that sizeText renders resource totals and human-readable file size for canceled exports — this correctly exercises the !taskIsCompleted template branch and catches the actual user-visible output.
3 participants
Summary
@vue/test-utilsto@testing-library/vue.Manual verification:
pnpm run test-jest -- kolibri/plugins/device/frontend/views/__tests__/PinAuthenticationModal.spec.jspnpm run test-jest -- kolibri/plugins/device/frontend/views/ManageTasksPage/__tests__/TaskPanel.spec.jspnpm run test-jest -- --testPathPattern devicepre-commit run --files kolibri/plugins/device/frontend/views/__tests__/PinAuthenticationModal.spec.js kolibri/plugins/device/frontend/views/ManageTasksPage/__tests__/TaskPanel.spec.jsReferences
Reviewer guidance
wrapper.vm, child component props, or@vue/test-utils, and instead assert through visible text, buttons, and emitted events.AI usage
I used an AI tool minimally for small syntax and code-block fixes, and to help structure the PR description. I reviewed and edited the final code and wording myself, adjusted the queries to match the actual rendered DOM and verified the relevant tests locally.