Migrate epub viewer TOC tests from vue test utils to Vue testing library#14542
Migrate epub viewer TOC tests from vue test utils to Vue testing library#14542curiouscoder-cmd wants to merge 2 commits intolearningequality:developfrom
Conversation
github-actions
bot
added
DEV: renderers
|
👋 Hi @curiouscoder-cmd, 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. |
Build Artifacts
|
|
Hello mentors, pls review the PR whenever you get time |
|
📢✨ Before we assign a reviewer, we'll turn on |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Good migration — all three files use VTL correctly and CI is green.
No UI files changed; Phase 3 skipped.
Suggestions (non-blocking):
TocButton.spec.jsline 12:getByRole('button')passes but doesn't verify the accessible name (see inline comment)TableOfContentsSection.spec.jslines 61/66:container.firstElementChildusage (see inline comment)
Praise:
- The
sectionWithEmptyLabeltest cleanly covers thehref-as-fallback edge case - Correct choice of
getByTextforKButtonelements — the source confirmsappearance="basic-link"renders as<a>withouthref, which has no implicit ARIA role, makinggetByRoleinapplicable here
@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
| expect(wrapper.exists()).toBe(true); | ||
| it('renders the table of contents button', () => { | ||
| renderComponent(); | ||
| expect(screen.getByRole('button')).toBeInTheDocument(); |
There was a problem hiding this comment.
suggestion: getByRole('button') finds the button but doesn't verify its accessible name. Since KIconButton receives ariaLabel="Toggle table of contents", adding { name: /toggle table of contents/i } would also assert that the label is wired up correctly — catching regressions if the $tr key or prop binding changes.
| expect(wrapper.findComponent({ name: 'KButton' }).text()).toBe(sectionWithEmptyLabel.href); | ||
| it('applies the appropriate top-level styling class for root level sections', () => { | ||
| const { container } = renderComponent({ section, depth: 0 }); | ||
| expect(container.firstElementChild).toHaveClass('toc-list-item-top-level'); |
There was a problem hiding this comment.
suggestion: container.firstElementChild couples the test to the rendered DOM structure and is generally discouraged by Testing Library. Acceptable here because there's no accessible attribute that reflects the depth === 0 state, but worth noting. If TableOfContentsSection ever gains an aria-level or similar attribute, switching to that would make the test more resilient.
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Both prior suggestions addressed cleanly.
CI passing. No UI files changed — Phase 3 skipped.
Prior findings
Resolved:
getByRole('button')name filter missing (suggestion) —{ name: /toggle table of contents/i }added at both query sites inTocButton.spec.jscontainer.firstElementChildDOM coupling (suggestion) — replaced withgetByText().closest('li')inTableOfContentsSection.spec.js
2/2 prior findings resolved.
New findings: none.
@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(wrapper.exists()).toBe(true); | ||
| it('renders the table of contents button', () => { | ||
| renderComponent(); | ||
| expect(screen.getByRole('button', { name: /toggle table of contents/i })).toBeInTheDocument(); |
There was a problem hiding this comment.
praise: Good fix — both query sites updated consistently, and the name filter now also acts as a regression guard for the $tr binding.
|
I made the suggested changes. All prior findings are resolved now. |
3 participants
Summary
TocButton.spec.js,TableOfContentsSection.spec.js, andTableOfContentsSectionSideBar.spec.js) from@vue/test-utilsto Vue Testing Library (@testing-library/vue). Refactored queries to test purely from a user's perspective, avoiding tight coupling to component internals.pnpm run test-jest -- --testPathPattern epub_viewerto ensure all 47 test cases in theepub_viewersuite passed successfully. Verified that NO residual@vue/test-utilsimports remain in the targeted files.References
Resolves #14189
Reviewer guidance
Reviewers can fetch this branch locally and run
pnpm run test-jest -- --testPathPattern epub_viewerto verify the tests pass organically. All tests follow standard user-experience workflows (e.g.fireEvent.click).There are no risky changes to core UI routing. However, while converting the test suite, I intentionally prioritized
screen.getByTextqueries for the navigation sections overgetByRole('button'). This was done purposefully to gracefully handleKButton(when rendered as a basic link) internally lacking an explicitrole="button". This strategy strictly mirrors the current preferred VTL patterns established in the reference filepackages/kolibri-common/components/userAccounts/__tests__/GenderSelect.spec.js.AI usage
I used Gemini to help structure this PR description, and help to fix the linting error and
brainstormed the
@testing-library/vuerefactoring approach with Gemini,I reviewed the generated test files to verify the queries correctly addressedKButtonaccessibility limitations and ran the full testing suite to ensure backward compatibility.