kolibri-design-system icon indicating copy to clipboard operation
kolibri-design-system copied to clipboard

[Visual testing]: Add visual tests for KCheckbox

Open AlexVelezLl opened this issue 10 months ago • 18 comments

🌱 Are you new to the codebase? Welcome! Please see the contributing guidelines.

Context

We recently introduced a new visual testing system in KDS to prevent visual regressions and ensure UI consistency, and we want to start populating our KDS components with visual tests.

If you are not familiar yet with our visual testing system. Please refer to our Visual Testing Guide. You can also check the existing tests for KButton as an example.

Additionally, see the KCheckbox documentation for reference.

Desired behavior

KCheckbox should have visual tests to cover at least these snapshots:

  • KCheckbox deselected
  • KCheckbox indeterminate
  • KCheckbox selected
  • KCheckbox deselected disabled
  • KCheckbox indeterminate disabled
  • KCheckbox selected disabled
  • KCheckbox without label (with showLabel=false)
  • KCheckbox using the default slot to set the label.
  • KCheckbox with description

Note

Since we currently dont have a test file for KCheckbox. Please move the current KCheckbox.vue file to be a folder KCheckbox/index.vue and create the test file in KCheckbox/__tests__/KCheckbox.spec.js

Acceptance criteria

  • A new visual test suite is created for KCheckbox.
  • The test includes all required snapshots.
  • The test runs successfully and captures the expected snapshots.

AlexVelezLl avatar Feb 04 '25 22:02 AlexVelezLl

@AlexVelezLl I would love to work on this issue. Please assign this to me.

you-think-you-know-me avatar Feb 04 '25 23:02 you-think-you-know-me

Hey @you-think-you-know-me! For sure! I will assign this to you, please let us know any question :).

AlexVelezLl avatar Feb 05 '25 12:02 AlexVelezLl

Hi @AlexVelezLl , in case @you-think-you-know-me is busy in some other engagements, I think i can give this issue a try!

Abhishek-Punhani avatar Feb 24 '25 08:02 Abhishek-Punhani

Hey @Abhishek-Punhani, I have already done most of the work and will soon raise PR. I was busy with my exams, but now I am free.

you-think-you-know-me avatar Feb 24 '25 08:02 you-think-you-know-me

No pressure @you-think-you-know-me

MisRob avatar Feb 24 '25 08:02 MisRob

Hey @Abhishek-Punhani, I have already done most of the work and will soon raise PR. I was busy with my exams, but now I am free.

Oh sorry I didn’t see any linked PR to this issue.

Abhishek-Punhani avatar Feb 24 '25 08:02 Abhishek-Punhani

Automatically unassigning @you-think-you-know-me due to no comments here, or updates on the associated pull request for 1 month. @you-think-you-know-me, if you're still interested in this issue or already have work in progress, please message us here, and we'll assign you again. Thank you!

Hey @AlexVelezLl ! I'd like to work on this. could you please assign it to me?

SukhvirKooner avatar Apr 07 '25 06:04 SukhvirKooner

Hi @SukhvirKooner, I originally posted that we will assign to you, but now I noticed @you-think-you-know-me mentioned that most of the work is done and there were exams - so I think on this issue I will wait for a bit more to see if we hear back. If not, we will re-assign to you.

MisRob avatar Apr 08 '25 17:04 MisRob

Sure! No Problem 👍

SukhvirKooner avatar Apr 08 '25 18:04 SukhvirKooner

I will assign you now @SukhvirKooner

MisRob avatar Apr 15 '25 09:04 MisRob

Thank you! @MisRob assigning this issue to me. I'll start working on this.

SukhvirKooner avatar Apr 16 '25 16:04 SukhvirKooner

Hi @MisRob

Quick update on this issue

  • Error: nuxt_plugin_visualloadtestcomponents_* 500 error during the visual test setup when i ran yarn test:visual

I think 500 error happened because Nuxt only bundles plugins in its plugins/ directory (or ones explicitly listed in nuxt.config.js), and we had visual.load-test-components.js tucked away under jest.conf/. Nuxt generated an internal alias for that file but never actually included it, so every request for the visual playground blew up.

  • Fix: I moved visual.load-test-components.js into our plugins/ folder (under docs/plugins/). In that plugin I globally register:
import KCheckbox from '~~/lib/KCheckbox/index.vue'
Vue.component('KCheckbox', KCheckbox);

This ensures <KCheckbox> renders in the testing playground instead of causing “unknown custom element” errors.

  • Verification: After that, I ran the visual tests I wrote to cover all snapshots mentioned in the issue description KCheckbox deselected, indeterminate, selected, deselected disabled, indeterminate disabled, selected disabled, without label, using the default slot for the label and with description and they all passed without errors.
Image

Questions before PR

  1. Should I keep visual.load-test-components.js in the plugins/ folder for this PR, or would you prefer it housed somewhere else or loaded via a different mechanism? Let me know your thoughts!
  2. Is there anything else I should cover or be aware of before opening the PR?

SukhvirKooner avatar Apr 20 '25 18:04 SukhvirKooner

Thanks for reporting @SukhvirKooner. @AlexVelezLl do you have any thoughts on what would be best to do, or have you observed this for any other components?

MisRob avatar May 01 '25 03:05 MisRob

@SukhvirKooner I think you can feel free to open a pull request anyway, and then this can be fine-tuned there.

MisRob avatar May 01 '25 03:05 MisRob

Hi @SukhvirKooner! Apologies I missed this message. Can you add a capture of the error in the console?

In that plugin I globally register: import KCheckbox from '~~/lib/KCheckbox/index.vue' Vue.component('KCheckbox', KCheckbox);

You dont need to register KCheckbox for this, We just need to register Vue components that we use to render more complex layouts that we cant achieve with our renderComponentForVisualTest method, please see https://github.com/learningequality/kolibri-design-system/blob/develop/dev_docs/07_visual_testing_guide.md?plain=1#L104.

KCheckbox is a KDS component, we already declare this in our KThemePlugin, so we dont need to declare it again. Perhaps this is the reason why this was failing?

AlexVelezLl avatar May 01 '25 16:05 AlexVelezLl

Just opened a PR to include more instructions about this ^ https://github.com/learningequality/kolibri-design-system/pull/1016.

AlexVelezLl avatar May 01 '25 22:05 AlexVelezLl

thank you @AlexVelezLl for the guidance! I’ve removed the unnecessary manual registration of KCheckbox:

import KCheckbox from '~~/lib/KCheckbox/index.vue';
Vue.component('KCheckbox', KCheckbox);

since it’s already provided by KThemePlugin. I’ve opened a PR with this visual tests, please take a look and let me know if anything else needs adjustment. Thanks!

SukhvirKooner avatar May 03 '25 09:05 SukhvirKooner

Closed by https://github.com/learningequality/kolibri-design-system/pull/1017

MisRob avatar May 22 '25 04:05 MisRob