Refactor device plugin test files to use Vue Testing Library by curiouscoder-cmd · Pull Request #14376 · learningequality/kolibri

curiouscoder-cmd · 2026-03-12T22:25:55Z

Summary

Refactored WelcomeModel.spec.js and DeviceSettingsPage.spec.js within the device plugin to use Vue Testing Library (@testing-library/vue) instead of @vue/test-utils.
Replaced assertions heavily reliant on component implementation details (like .vm.$data and wrapper.find) with user-centric DOM queries utilizing screen.getByRole.
Replaced mount with render handling and updated interactions using fireEvent and jest-dom assertions like toBeChecked().
Manual Verification: Ran the full device test suite (pnpm run test-jest -- --testPathPattern device) to verify that the changes do not break any existing test coverage. The entire suite passes cleanly (19 suites, 464 tests).

References

Fixes #14264

Reviewer guidance

Reviewers can checkout this branch and run pnpm run test-jest -- --testPathPattern device to ensure the device test suites pass.
Please note that within DeviceSettingsPage.spec.js, saveSpy is now configured to spy directly on the imported API saveDeviceSettings call instead of trying to spy on the internal component vm method, heavily adhering to VTL's guiding principles of keeping tests tightly bound to what the user and DOM see instead of Vue internals.

learning-equality-bot · 2026-03-12T22:26:26Z

👋 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! 😊

curiouscoder-cmd · 2026-03-12T22:31:07Z

Sorry for the delay, I was busy with exams. Please review the PR and let me know wherever changes are required.

rtibbles · 2026-03-12T22:34:14Z

Hi @curiouscoder-cmd - just flagging that you have tagged this as fixing the tracking issue for the VTL migration work. Which actual issue is this targeted at? Will help us to review!

curiouscoder-cmd · 2026-03-12T22:40:52Z

Hi @curiouscoder-cmd - just flagging that you have tagged this as fixing the tracking issue for the VTL migration work. Which actual issue is this targeted at? Will help us to review!

Apologies for the confusion there this PR is intended to address issue #14264 (Migrate Device settings tests to Vue Testing Library). I mistakenly referenced the tracking issue in the description. I have updated the references to point to the correct issue. Thanks for pointing it out :)

github-actions · 2026-03-12T23:03:10Z

Build Artifacts

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

Smoke test screenshot

Download screenshot

rtibbles · 2026-03-13T00:16:19Z

No worries - thank you for updating!

learning-equality-bot · 2026-03-13T00:17:22Z

📢✨ 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

Clean migration of both device test files from @vue/test-utils to VTL.

CI passing. No UI files changed — test-only PR.

blocking (1): Reduced coverage in loads the data test
suggestion (1): Helper indirection adds complexity without benefit
nitpick (1): Minor typo in test description
praise (2): Query strategy; API spy approach

@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

kolibri/plugins/device/frontend/views/DeviceSettingsPage/__tests__/DeviceSettingsPage.spec.js

kolibri/plugins/device/frontend/views/__tests__/WelcomeModel.spec.js

rtibblesbot · 2026-03-13T00:19:39Z

kolibri/plugins/device/frontend/views/DeviceSettingsPage/__tests__/DeviceSettingsPage.spec.js

-  const disallowGuestAccess = wrapper.find('[data-test="disallowGuestAccessButton"]');
-  const lockedContent = wrapper.find('[data-test="lockedContentButton"]');
+function getButtons() {
+  const saveButton = screen.getByRole('button', { name: /save changes/i });


praise: Excellent use of screen.getByRole with accessible names instead of data-test attribute selectors. This validates that the component has proper accessibility semantics (radio buttons, button roles) while also making the tests resilient to implementation changes.

kolibri/plugins/device/frontend/views/DeviceSettingsPage/__tests__/DeviceSettingsPage.spec.js

rtibblesbot

Test-only migration from @vue/test-utils to VTL — good direction.

Prior Finding Status

#	Finding	Severity	Status	Evidence
1	Reduced coverage in `loads the data` test	blocking	UNADDRESSED	No reply, no code change — downgraded to suggestion on re-evaluation (see below)
2	Helper indirection (`getButtons`)	suggestion	UNADDRESSED	On reflection, `getButtons()` is reasonable DRY — extracting 7 role queries used across 10+ tests. Withdrawn.
3	Test description typo	nitpick	RESOLVED	The current description `'emits submit event when continue button is clicked'` is already accurate — the prior review appears to have referenced different text.
4	Query strategy praise	praise	N/A
5	API spy praise	praise	N/A

Summary: 1/3 actionable prior findings resolved. 1 re-raised (downgraded). 1 withdrawn.

CI failing — no-unused-vars lint error on Vuex import (see inline). JS tests pass.

blocking (1): Unused Vuex import fails CI lint
suggestion (1): loads the data test could verify the language select reflects loaded data

@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

kolibri/plugins/device/frontend/views/DeviceSettingsPage/__tests__/DeviceSettingsPage.spec.js

rtibblesbot · 2026-03-13T09:18:09Z

kolibri/plugins/device/frontend/views/DeviceSettingsPage/__tests__/DeviceSettingsPage.spec.js

+      await fireEvent.click(saveButton);
      await global.flushPromises();
-      expect(saveSpy).toHaveBeenCalledWith(
+      expect(api.saveDeviceSettings).toHaveBeenCalledWith(


praise: Spying on the imported api.saveDeviceSettings instead of wrapper.vm.saveDeviceSettings is a big improvement — it verifies the actual API boundary rather than an internal method. Clean VTL migration.

rtibblesbot

Clean VTL migration — test-only PR, no source changes.

CI passing (build checks still in progress but all test/smoke checks green).

Prior Finding Status

#	Finding	Severity	Status	Evidence
1	Unused `Vuex` import fails CI lint	blocking	UNADDRESSED	`Vuex` still imported on line 3 but never used — downgraded to suggestion since no lint CI check appears to run
2	`loads the data` test could verify language select	suggestion	UNADDRESSED	Acceptable given VTL philosophy — withdrawn

suggestion (1): Unused Vuex import
praise (1): API boundary testing approach

@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

kolibri/plugins/device/frontend/views/DeviceSettingsPage/__tests__/DeviceSettingsPage.spec.js

rtibblesbot

Clean VTL migration — test-only PR, no source changes.

All CI checks passing. No UI files changed.

Prior Finding Status

#	Finding	Severity	Status
1	Unused `Vuex` import	suggestion	RESOLVED — line 3 now imports only `{ Store }`, not `Vuex`
2	`loads the data` language select	suggestion	Previously withdrawn

All prior findings resolved.

praise (2): Role-based queries; API boundary spying

@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-13T15:07:12Z

kolibri/plugins/device/frontend/views/DeviceSettingsPage/__tests__/DeviceSettingsPage.spec.js

-  const disallowGuestAccess = wrapper.find('[data-test="disallowGuestAccessButton"]');
-  const lockedContent = wrapper.find('[data-test="lockedContentButton"]');
+function getButtons() {
+  const saveButton = screen.getByRole('button', { name: /save changes/i });


praise: Querying by role and accessible name (getByRole('radio', { name: /…/i })) rather than data-test selectors is exactly how VTL is meant to be used — these tests now double as implicit accessibility assertions that the radio buttons have correct labels.

rtibblesbot · 2026-03-13T15:07:12Z

kolibri/plugins/device/frontend/views/DeviceSettingsPage/__tests__/DeviceSettingsPage.spec.js

+      await fireEvent.click(saveButton);
      await global.flushPromises();
-      expect(saveSpy).toHaveBeenCalledWith(
+      expect(api.saveDeviceSettings).toHaveBeenCalledWith(


praise: Spying on api.saveDeviceSettings at the module boundary instead of wrapper.vm.saveDeviceSettings is a solid improvement — tests are now resilient to internal refactors while still verifying the correct API call and payload.

I have also fixed the linting issue

curiouscoder-cmd · 2026-03-13T17:08:47Z

@rtibbles I have addressed the requested changes , It seems the bot may sometimes be checking a previous commit instead of the latest one. Kindly review it once whenever possible.

rtibbles

I think we're still missing some coverage where the bot had previously identified a blocking comment.

We should be able to predict from the mock data the exact state of all the buttons, so we should also know if unlistedChannels exists or not - and it should be an error if it's present when it shouldn't be, or absent when it's not.

rtibbles · 2026-03-18T22:52:22Z

kolibri/plugins/device/frontend/views/DeviceSettingsPage/__tests__/DeviceSettingsPage.spec.js

-  }
+    api.getDeviceSettings.mockResolvedValue(DeviceSettingsData);
+    await makeWrapper();
+    const { signInPage, unlistedChannels } = getButtons();


Feels like we're still only asserting these two settings and the language setting below? Seems like a reduction in coverage to me.

Fixed, assert all 6 buttons based on deviceSettingsData.

rtibbles · 2026-03-18T22:52:55Z

kolibri/plugins/device/frontend/views/DeviceSettingsPage/__tests__/DeviceSettingsPage.spec.js

-    const checked = buttonProps.buttonValue === buttonProps.currentValue;
-    expect(checked).toBe(expected);
-  }
+    expect(screen.getAllByText(/english/i)[0]).toBeInTheDocument();


This is quite a broad test - this could also pass, if "english" just happens to be anywhere in the page, rather than specifically in the language selector drop down.

Agreed I change it to use getAllByText('English')[0] with exact case match instead /english/i. now this ismore specific since it will not match partial case variations else where on the page.

curiouscoder-cmd · 2026-03-19T13:05:11Z

There's a CI failure in the Python postgres unit tests but it appears to be unrelated to my changes.
The failing test is:

FAILED kolibri/core/content/test/test_content_app.py::ContentNodeAPITestCase::test_contentnode_tree
This is a backend content API test, while my PR only modifies frontend test files in the Device plugin. This looks like it may be a flaky test.

could a maintainer re-run the failed check when possible?
and let me know if any other changes need to be made. Thanks.

curiouscoder-cmd · 2026-03-23T10:07:03Z

Hello , just wanted to follow up , if the changes are meeting the requirements or any other changes updates are needed

github-actions bot added APP: Device Re: Device App (content import/export, facility-syncing, user permissions, etc.) DEV: frontend SIZE: medium labels Mar 12, 2026

rtibbles requested a review from rtibblesbot March 13, 2026 00:16

rtibblesbot reviewed Mar 13, 2026

View reviewed changes

curiouscoder-cmd force-pushed the refactor-device-tests-vtl branch from fd15e9a to c69b6b7 Compare March 13, 2026 09:08

rtibblesbot reviewed Mar 13, 2026

View reviewed changes

curiouscoder-cmd force-pushed the refactor-device-tests-vtl branch 2 times, most recently from 9b63386 to 819c09a Compare March 13, 2026 14:46

rtibblesbot reviewed Mar 13, 2026

View reviewed changes

kolibri/plugins/device/frontend/views/DeviceSettingsPage/__tests__/DeviceSettingsPage.spec.js Show resolved Hide resolved

kolibri/plugins/device/frontend/views/DeviceSettingsPage/__tests__/DeviceSettingsPage.spec.js Show resolved Hide resolved

rtibblesbot approved these changes Mar 13, 2026

View reviewed changes

Refactor device plugin test files to use Vue Testing Library

faf1e6c

curiouscoder-cmd force-pushed the refactor-device-tests-vtl branch from 2fc5e05 to faf1e6c Compare March 13, 2026 16:49

[pre-commit.ci lite] apply automatic fixes

4b970a9

curiouscoder-cmd mentioned this pull request Mar 16, 2026

Migrate Device settings tests to Vue Testing Library #14264

Open

7 tasks

rtibbles requested changes Mar 18, 2026

View reviewed changes

Address review: assert button states from mock data

295f5c5

rtibbles self-assigned this Mar 19, 2026

Conversation

curiouscoder-cmd commented Mar 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

References

Reviewer guidance

Uh oh!

learning-equality-bot bot commented Mar 12, 2026

Uh oh!

curiouscoder-cmd commented Mar 12, 2026

Uh oh!

rtibbles commented Mar 12, 2026

Uh oh!

curiouscoder-cmd commented Mar 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

rtibbles commented Mar 13, 2026

Uh oh!

learning-equality-bot bot commented Mar 13, 2026

Uh oh!

rtibblesbot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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 Finding Status

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rtibblesbot left a comment

Choose a reason for hiding this comment

Prior Finding Status

Uh oh!

Uh oh!

Uh oh!

rtibblesbot left a comment

Choose a reason for hiding this comment

Prior Finding Status

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!

curiouscoder-cmd commented Mar 13, 2026

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

curiouscoder-cmd commented Mar 19, 2026

Uh oh!

curiouscoder-cmd commented Mar 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

curiouscoder-cmd commented Mar 12, 2026 •

edited

Loading

curiouscoder-cmd commented Mar 12, 2026 •

edited

Loading

github-actions bot commented Mar 12, 2026 •

edited

Loading