Migrate epub viewer search tests to Vue Testing Library by sahibsiddiqui · Pull Request #14321 · learningequality/kolibri

sahibsiddiqui · 2026-03-03T20:53:06Z

Summary

This PR migrates the epub viewer search unit tests from @vue/test-utils to @testing-library/vue.

The following files were updated:

SearchButton.spec.js
SearchSideBar.spec.js

Changes include:

Removed @vue/test-utils usage
Rewrote tests to follow Testing Library principles
Updated queries to prioritize user-facing interactions (e.g., getByRole)
Removed direct access to component internals (wrapper, vm, etc.)

Manual verification

Ran pnpm run test-jest -- --testPathPattern epub_viewer
Confirmed all epub_viewer tests pass locally

References

Closes #14187

Reviewer guidance

Reviewers can verify by running: pnpm run test-jest -- --testPathPattern epub_viewer
Only two files were modified, no other files were changed.

learning-equality-bot · 2026-03-03T20:53:42Z

👋 Thanks for contributing!

We will assign a reviewer within the next two weeks. In the meantime, please ensure that:

You ran pre-commit locally
All issue requirements are satisfied
The contribution is aligned with our Contributing guidelines. Pay extra attention to Using generative AI. Pull requests that don't follow the guidelines will be closed.

We'll be in touch! 😊

rtibbles · 2026-03-03T20:54:41Z

Requesting rtibblesbot review for first pass, we will do a full review when it is satisfied!

rtibblesbot

Clean migration of epub viewer search tests to Vue Testing Library. CI passing (admin checks only — no build/test CI ran).

blocking: Focus test in SearchSideBar no longer tests the component's focusOnInput() method (see inline comment)
praise: Good use of getByRole queries in both files

@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-03-03T20:56:25Z

+
+    const input = screen.getByRole('searchbox');
+
+    input.focus();


blocking: This test no longer verifies the component's focusOnInput() public method — it just calls input.focus() directly on the DOM element, which tests native browser behavior rather than component behavior.

The original test called wrapper.vm.focusOnInput(), which verified that the component's ref wiring (this.$refs.searchInput.focus() at line 181 of SearchSideBar.vue) works correctly. The new test bypasses that entirely.

Vue Testing Library's render returns the component instance on the container property's parent. One option that preserves the behavioral test while staying within VTL patterns:

it('should allow parent to focus on input box', async () => { const { container } = renderComponent(); const vm = container.__vue_app__ ? ... // Vue 3 // For Vue 2 with VTL, you can access the instance via: // const wrapper = renderComponent(); // wrapper.vm is not exposed by VTL by design.

Since VTL intentionally discourages vm access, consider testing this indirectly — e.g., by simulating what a parent component would do. Alternatively, if the team accepts that focusOnInput is an internal detail not worth testing through VTL, the test description should at minimum be updated to reflect what it actually tests (e.g., "search input should be focusable") rather than implying it tests the parent-facing API.

rtibblesbot · 2026-03-03T20:56:25Z

+  it('should render', () => {
+    render(SearchButton);
+
+    const button = screen.getByRole('button');


praise: Good use of getByRole('button') — queries by accessible role rather than CSS selectors, which is exactly what Testing Library encourages.

rtibblesbot

Clean migration overall, but the focus test finding from the prior review remains unaddressed.

CI passing.

blocking: Focus test still bypasses the component's focusOnInput() public method (see inline comment)
nitpick: Missing newline at end of SearchSideBar.spec.js

1 prior finding(s) resolved (praise on getByRole usage).

@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-03-03T20:57:35Z

+
+    const input = screen.getByRole('searchbox');
+
+    input.focus();


blocking (UNADDRESSED): This still calls input.focus() directly on the DOM element rather than testing the component's focusOnInput() method (line 180 of SearchSideBar.vue, marked @public). The test now only proves that a native <input> can receive focus, which is a browser default.

Since VTL discourages vm access, the pragmatic options are:

Rename the test to "search input should be focusable" so it describes what it actually tests, accepting that focusOnInput() is no longer unit-tested.

Use a thin wrapper parent component in the test that calls the child's focusOnInput() via a template ref, verifying the public API as a parent would.

Apologies for the duplicate review here!

I do agree though that we still need to test the component method, not the trivial assertion that an input, once focused, has focus.

thanks for the review!
i understand the issue that the current test focuses the input directly instead of verifying the component’s 'focusOnInput()' method.
so i’ll update the test acc, and will push a fix shortly.

sahibsiddiqui · 2026-03-04T21:41:19Z

@rtibbles i've updated the test to verify the component's focusOnInput() behavior using a parent wrapper component. please let me know if further adjustments are needed.

rtibblesbot

Clean migration to Vue Testing Library. The focus test now properly verifies focusOnInput() through a parent wrapper component.

CI passing.

nitpick: Commented-out renderComponent() on line 23 of SearchSideBar.spec.js (see inline)
praise: Parent wrapper pattern for testing focusOnInput() (see inline)

2 prior finding(s) resolved.

@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-03-04T21:55:53Z

-    const elementThatIsFocused = document.activeElement;
-    expect(elementThatIsFocused).toHaveClass('search-input');
+  it('should allow parent to focus on input box', async () => {
+    // renderComponent();


nitpick: Remove the commented-out // renderComponent(); — it's leftover from development and not needed since the parent wrapper handles rendering.

rtibblesbot · 2026-03-04T21:55:53Z

+        const sidebarRef = ref(null);
+
+        function focusInput() {
+          sidebarRef.value.focusOnInput();


praise: Nice use of a parent wrapper to test focusOnInput() through its intended public API — this verifies the real parent-child interaction rather than reaching into component internals.

Yes, this is a very nice way of asserting the overall behaviour!

… as well

sahibsiddiqui · 2026-03-05T04:09:29Z

yeah, even the cleanup's done

rtibblesbot

Clean migration of epub viewer search tests to Vue Testing Library. All prior findings resolved.

CI passing.

3 prior finding(s) resolved.

@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-03-05T04:14:46Z

-    expect(wrapper.emitted().click).toBeTruthy();
+
+  it('should emit an event when the button is clicked', async() => {
+    const { emitted } = render(SearchButton);


praise: Clean use of emitted() destructuring from the render return — keeps the assertion on line 18 concise and readable.

rtibbles

Looks good, should be good to merge assuming the tests pass.

rtibbles · 2026-03-05T17:38:26Z

+        const sidebarRef = ref(null);
+
+        function focusInput() {
+          sidebarRef.value.focusOnInput();


Yes, this is a very nice way of asserting the overall behaviour!

github-actions · 2026-03-05T18:23:58Z

Build Artifacts

Asset type	Download link
PEX file	kolibri-0.20.0.dev0_git.20260307195552.pex
Windows Installer (EXE)	kolibri-0.20.0.dev0+git.20260307195552-windows-setup-unsigned.exe
Debian Package	kolibri_0.20.0.dev0+git.20260307195552-0ubuntu1_all.deb
Mac Installer (DMG)	kolibri-0.20.0.dev0+git.20260307195552.dmg
Android Package (APK)	kolibri-0.20.0.dev0+git.20260307195552-0.1.6-debug.apk
Raspberry Pi Image	kolibri-pi-image-0.20.0.dev0+git.20260307195552.zip
TAR file	kolibri-0.20.0.dev0+git.20260307195552.tar.gz
WHL file	kolibri-0.20.0.dev0+git.20260307195552-py2.py3-none-any.whl

rtibbles · 2026-03-06T22:10:42Z

Linting failures still present:

[ERROR: Kolibri Format] Linting errors for kolibri/plugins/epub_viewer/frontend/views/tests/SearchSideBar.spec.js

/home/runner/work/kolibri/kolibri/kolibri/plugins/epub_viewer/frontend/views/tests/SearchSideBar.spec.js
34:18 error 'sidebarRef' of property returned from setup() found, but never used vue/no-unused-properties
34:30 error 'focusInput' of property returned from setup() found, but never used vue/no-unused-properties

…lity#14321) * Migrate epub viewer search tests to Vue Testing Library * fixed SearchSideBar focus test to verify focusOnInput method * removed leftover comment renderComponent & added a newline at the end as well * [pre-commit.ci lite] apply automatic fixes * fixes lint errors and updated focus assertion * fixing lint errors in SearchSideBar.spec test wrapper --------- Co-authored-by: pre-commit-ci-lite[bot] <117423508+pre-commit-ci-lite[bot]@users.noreply.github.com>

Migrate epub viewer search tests to Vue Testing Library

ffb0b76

github-actions Bot added DEV: renderers HTML5 apps, videos, exercises, etc. DEV: frontend SIZE: small labels Mar 3, 2026

rtibbles requested a review from rtibblesbot March 3, 2026 20:54

rtibbles self-assigned this Mar 3, 2026

sahibsiddiqui mentioned this pull request Mar 3, 2026

Convert epub viewer search unit tests to Vue Testing Library #14187

Closed

7 tasks

rtibblesbot suggested changes Mar 3, 2026

View reviewed changes

fixed SearchSideBar focus test to verify focusOnInput method

4c8238b

rtibblesbot reviewed Mar 4, 2026

View reviewed changes

removed leftover comment renderComponent & added a newline at the end…

9910f56

… as well

rtibblesbot approved these changes Mar 5, 2026

View reviewed changes

rtibbles approved these changes Mar 5, 2026

View reviewed changes

[pre-commit.ci lite] apply automatic fixes

8047c9c

fixes lint errors and updated focus assertion

8ae9683

fixing lint errors in SearchSideBar.spec test wrapper

81ee2ad

rtibbles merged commit ea134e2 into learningequality:develop Mar 9, 2026
51 checks passed

Uh oh!

Conversation

sahibsiddiqui commented Mar 3, 2026

Summary

References

Reviewer guidance

Uh oh!

learning-equality-bot Bot commented Mar 3, 2026

Uh oh!

rtibbles commented Mar 3, 2026

Uh oh!

rtibblesbot 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!

rtibblesbot 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!

Choose a reason for hiding this comment

Uh oh!

sahibsiddiqui commented Mar 4, 2026

Uh oh!

rtibblesbot 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!

Choose a reason for hiding this comment

Uh oh!

sahibsiddiqui commented Mar 5, 2026

Uh oh!

rtibblesbot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rtibbles left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Mar 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rtibbles commented Mar 6, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions Bot commented Mar 5, 2026 •

edited

Loading