Implement visual tests for KButton component #710

KshitijThareja · 2024-07-31T13:12:09Z

Description

This PR aims to introduce concrete visual tests for the KButton component, supplemented by modifications to the existing testing playground file and the renderComponent function to allow for usage of slots in the visual tests.

Issue addressed

#689

Addresses #PR# HERE

Before/after screenshots

Changelog

[#PR no]
- Description: Implement visual tests for KButton component along with some other modifications.
- Products impact: -
- Addresses: [Visual Testing]: Implement visual tests of KButton #689
- Components: -
- Breaking: -
- Impacts a11y: -
- Guidance: -

[#PR no]: PR link

Steps to test

Set the PERCY_TOKEN env variable in your environment.
Run yarn test:visual to execute visual tests
Check Percy dashboard to see visual diffs generated.

At a high level, how did you implement this?

This was accomplished by referring to the Puppeteer documentation for implementing abstract methods for interacting with the components. The changes to the render function and playground file were made in order to render the components if slots were passed from within the tests.

Testing checklist

Contributor has fully tested the PR manually
If there are any front-end changes, before/after screenshots are included
Critical and brittle code paths are covered by unit tests
The change is described in the changelog section above

Reviewer guidance

Is the code clean and well-commented?
Are there tests for this change?
Are all UI components LTR and RTL compliant (if applicable)?
Add other things to check for here

After review

The changelog item has been pasted to the CHANGELOG.md

Comments

github-actions · 2024-08-06T15:02:54Z

Percy Visual Test Results

Percy Dashboard: View Detailed Report

Environment:

Node.js Version: 18.x
OS: Ubuntu-latest

Instructions for Reviewers:

Click on the Percy Dashboard link to view detailed visual diffs.
Review the visual changes highlighted in the report.
Approve or request changes based on the visual differences.

AlexVelezLl · 2024-08-14T13:42:01Z

docs/pages/testing-playground.vue

+      <!-- Render default slot if provided -->
+      <template v-if="slots.default">
+        <!-- eslint-disable-next-line vue/no-v-html -->
+        <span v-html="slots.default"></span>


Why should this be in as span? I think a div would be more appropiate in case we need to add more elements inside

If I take the KButton test with default slot as an example, if you use div, the button will become twice the size, with the upper part showing slot content and the lower part showing the content passed to button itself. That's the reason for using span here.

Hmm I see, but there could be some components where a span could also be a problem. What if we add something like a "wrapperEl" prop that defines the element container, and a "content" that defines the v-html prop? Any other idea is welcome 👐.

I think we can combine both and have something like:

{ slots: { default: { element: "div", // or any vue component like KIcon elementProps: { class: "some-class" }, innerHTML: "<div> Some nested <a>content</a> </div>" } } }

AlexVelezLl · 2024-08-14T13:43:52Z

docs/pages/testing-playground.vue

+      </template>
+      <!-- Render named slots passed to the component -->
+      <template v-for="(slot, name) in namedSlots" #[name]>
+        <component :is="slot.content" v-bind="slot.props" :key="name" />


I think it would be better if we follow the same pattern in both default slot and named slot, I would vote to render it inside a v-html. Or in any case, if rendering inside a div gives any error, we can be flexible and provide both options. If slots.name is String, then render it inside a v-html, if not, we can render it as you are doing it here. What do you think?

Won't the named slots be other components that are being nested in a particular component? In that case they'll always have a name and specific props attached to them just like the parent component, which is being handled correctly in the current scenario.
I thought that default slots are used to handle the case in which slots.name is expected to be a string. I may be wrong though; I am not fully aware of how slots are used throughout KDS.

Not necessarily, we can have html elements that are not vue components as part of named slots (e.g. this #divider in KListWithOverflow, It is a correct Vue syntax, there are actually no differences in the restrictions of what we can do in the default slot vs named slots.

AlexVelezLl · 2024-08-14T13:46:38Z

jest.conf/visual.testUtils.js

@@ -1,23 +1,24 @@
 import percySnapshot from '@percy/puppeteer';

-export async function renderComponent(component, props) {
+export async function renderComponent(component, props, slots = {}) {


Here we need to add a jsdoc explaining these props, and explaining the structure slots should follow

Sure, will get it done 👍

AlexVelezLl · 2024-08-14T13:51:24Z

jest.conf/visual.testUtils.js

@@ -40,8 +41,30 @@ export async function renderComponent(component, props) {
  global.expect(isComponentRendered).toBe(true);
 }

-export async function takeSnapshot(name) {
+export async function takeSnapshot(name, options = {}) {


Here would be good also to have some jsdoc, and let users know what options could we use (I think it should be a lot of options that percySnapshot receives, in any case we could link to their documentation).

AlexVelezLl · 2024-08-14T14:01:25Z

lib/buttons-and-links/__tests__/KButton.spec.js

-    it('renders correctly with default props', async () => {
-      await renderComponent('KButton', { text: 'Test Button' });
-      await takeSnapshot('KButton - Default');
+    it('renders correctly with different appearances', async () => {


IMO each snapshot should be a test case. Unit test cases should be simple and test just one thing. If we want to group multiple test cases like for example with the disable state, we could use a describe block inside de discribe.visual block and group them.

So, if I am getting it correctly, for the case where we are testing different appearances of KButton for example, we need to write them all in separate it blocks and group those it blocks in one describe block?

Yes, although Im not sure if we should group them as "different appearances", in general all snapshots are different appearances.

Oh different appearances means different values for the appearance prop. I hadnt get it, in that case its up to you if it makes sense to group them, you can group them, if not its fine too.

…le template

…esign-system into vt-test

KshitijThareja · 2024-08-15T09:44:14Z

Hi @AlexVelezLl. I've updated the PR based on the suggestions 👍

AlexVelezLl

Thanks @KshitijThareja!! Code looks good to me, and snapshots are working as expected! I think we have reach to a very neat and flexible api for slots elements, which is super great. I left just a minor observation, once it is fixed, please feel free to hit that merge button.

We are almost thereee!!! Congratulations 🎉 🎉 🎉 🎉.

AlexVelezLl · 2024-08-16T13:40:45Z

lib/buttons-and-links/__tests__/KButton.spec.js

+          primary: true,
+          appearance: 'raised-button',
+        });
+        await takeSnapshot('KButton - Primary Raised Button', { widths: [400], minHeight: 512 });


As we will probably have the same sizes for (almost) all the screenshots of a single component, I think we can make this a constant object defined in the context of the describe.visual block and use the same options object thoughout the tests.

I've made the changes as requested ✅

Thank you!!

Add support for default slots rendering

c6a3ab3

AlexVelezLl mentioned this pull request Aug 6, 2024

Introduce visual testing to the existing Javascript tests workflow #686

Merged

9 tasks

Add option to specify custom dimensions for snapshots

fe94675

KshitijThareja temporarily deployed to percy_tests August 6, 2024 14:57 — with GitHub Actions Inactive

Update jsdoc

05809de

AlexVelezLl temporarily deployed to percy_tests August 14, 2024 13:06 — with GitHub Actions Inactive

Merge branch 'gsoc/visual-testing' into vt-test

855a5a2

AlexVelezLl temporarily deployed to percy_tests August 14, 2024 13:17 — with GitHub Actions Inactive

Merge branch 'gsoc/visual-testing' into vt-test

d1ead28

AlexVelezLl temporarily deployed to percy_tests August 14, 2024 13:28 — with GitHub Actions Inactive

AlexVelezLl reviewed Aug 14, 2024

View reviewed changes

KshitijThareja added 5 commits August 15, 2024 13:09

Modify testing playground to render named and default slots in a sing…

798dd24

…le template

Add jsdocs for renderComponent and takeSnapshot functions

b84eafa

Fix lint error

384e593

Remove async from describe block

9471e37

Merge branch 'vt-test' of https://github.com/KshitijThareja/kolibri-d…

6f3ab99

…esign-system into vt-test

KshitijThareja temporarily deployed to percy_tests August 15, 2024 09:42 — with GitHub Actions Inactive

AlexVelezLl approved these changes Aug 16, 2024

View reviewed changes

Add snapshot options specifying test widths

625cff0

KshitijThareja had a problem deploying to percy_tests August 16, 2024 17:26 — with GitHub Actions Failure

AlexVelezLl merged commit 3bddaab into learningequality:gsoc/visual-testing Aug 16, 2024
4 of 10 checks passed

AlexVelezLl mentioned this pull request Aug 26, 2024

[Visual Testing]: Implement visual tests of KButton #689

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement visual tests for KButton component #710

Implement visual tests for KButton component #710

KshitijThareja commented Jul 31, 2024

github-actions bot commented Aug 6, 2024 •

edited

Loading

AlexVelezLl Aug 14, 2024

KshitijThareja Aug 14, 2024

AlexVelezLl Aug 14, 2024

AlexVelezLl Aug 14, 2024

AlexVelezLl Aug 14, 2024 •

edited

Loading

KshitijThareja Aug 14, 2024

AlexVelezLl Aug 14, 2024 •

edited

Loading

AlexVelezLl Aug 14, 2024

KshitijThareja Aug 14, 2024

AlexVelezLl Aug 14, 2024

KshitijThareja Aug 14, 2024

AlexVelezLl Aug 14, 2024

KshitijThareja Aug 14, 2024

AlexVelezLl Aug 14, 2024

AlexVelezLl Aug 14, 2024

KshitijThareja commented Aug 15, 2024

AlexVelezLl left a comment

AlexVelezLl Aug 16, 2024

KshitijThareja Aug 16, 2024

AlexVelezLl Aug 16, 2024

Implement visual tests for KButton component #710

Implement visual tests for KButton component #710

Conversation

KshitijThareja commented Jul 31, 2024

Description

Issue addressed

Before/after screenshots

Changelog

Steps to test

At a high level, how did you implement this?

Testing checklist

Reviewer guidance

After review

Comments

github-actions bot commented Aug 6, 2024 • edited Loading

Percy Visual Test Results

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexVelezLl Aug 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexVelezLl Aug 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KshitijThareja commented Aug 15, 2024

AlexVelezLl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 6, 2024 •

edited

Loading

AlexVelezLl Aug 14, 2024 •

edited

Loading

AlexVelezLl Aug 14, 2024 •

edited

Loading