Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement visual tests for KButton component #710

Merged
merged 11 commits into from
Aug 16, 2024
Merged

Implement visual tests for KButton component #710

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
c6a3ab3
Add support for default slots rendering
KshitijThareja Jul 31, 2024
fe94675
Add option to specify custom dimensions for snapshots
KshitijThareja Aug 6, 2024
05809de
Update jsdoc
AlexVelezLl Aug 14, 2024
855a5a2
Merge branch 'gsoc/visual-testing' into vt-test
AlexVelezLl Aug 14, 2024
d1ead28
Merge branch 'gsoc/visual-testing' into vt-test
AlexVelezLl Aug 14, 2024
798dd24
Modify testing playground to render named and default slots in a sing…
KshitijThareja Aug 15, 2024
b84eafa
Add jsdocs for renderComponent and takeSnapshot functions
KshitijThareja Aug 15, 2024
384e593
Fix lint error
KshitijThareja Aug 15, 2024
9471e37
Remove async from describe block
KshitijThareja Aug 15, 2024
6f3ab99
Merge branch 'vt-test' of https://github.com/KshitijThareja/kolibri-d…
KshitijThareja Aug 15, 2024
625cff0
Add snapshot options specifying test widths
KshitijThareja Aug 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 29 additions & 2 deletions docs/pages/testing-playground.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,17 @@
Please do not modify the contents of this file.
-->
<div id="testing-playground" style="padding: 24px">
<component :is="component" v-bind="componentProps" />
<component :is="component" v-bind="componentProps">
<!-- Render default slot if provided -->
<template v-if="slots.default">
<!-- eslint-disable-next-line vue/no-v-html -->
<span v-html="slots.default"></span>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👐.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>"
    }
  }
}

</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" />
Copy link
Member

@AlexVelezLl AlexVelezLl Aug 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@AlexVelezLl AlexVelezLl Aug 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

</template>
</component>
</div>

</template>
Expand All @@ -32,9 +42,25 @@
* @type {Object} The props to be passed to the dynamically rendered component.
*/
componentProps: {},
/**
* @type {Object} The slots to be passed to the dynamically rendered component.
*/
slots: {},
};
},

/**
* Computed property that filters out the default slot from the slots object,
* returning only the named slots.
*/
computed: {
namedSlots() {
// eslint-disable-next-line no-unused-vars
const { default: defaultSlot, ...rest } = this.slots;
return rest;
},
},

/**
* Adds an event listener for messages from the test runner.
* This listener will trigger the `handleMessage` method.
Expand All @@ -53,12 +79,13 @@
methods: {
/**
* Handles messages received from the test runner to render a specified component.
* @param {MessageEvent} event - The message event containing the component and its props.
* @param {MessageEvent} event - The message event containing the component and its props.
*/
handleMessage(event) {
if (event.data.type === 'RENDER_COMPONENT') {
this.component = event.data.component;
this.componentProps = event.data.props;
this.slots = event.data.slots || {};
}
},
},
Expand Down
33 changes: 28 additions & 5 deletions jest.conf/visual.testUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,24 @@
import percySnapshot from '@percy/puppeteer';

export async function renderComponent(component, props) {
export async function renderComponent(component, props, slots = {}) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, will get it done 👍

const beforeRenderState = await page.evaluate(() => {
const testing_playground = document.querySelector('#testing-playground');
return testing_playground ? testing_playground.innerHTML : '';
});

await page.evaluate(
({ component, props }) => {
({ component, props, slots }) => {
window.postMessage(
{
type: 'RENDER_COMPONENT',
component: component,
props: props,
slots: slots,
},
'*'
);
},
{ component, props }
{ component, props, slots }
);
await page.waitForSelector('#testing-playground');

Expand All @@ -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 = {}) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure 👍

if (process.env.TEST_TYPE == 'visual') {
await percySnapshot(page, name);
await percySnapshot(page, name, options);
}
}

export async function delay(time) {
return new Promise(function(resolve) {
setTimeout(resolve, time);
});
}

export const click = async selector => {
await page.locator(selector).click();
};

export const hover = async selector => {
await page.locator(selector).hover();
};

export const scrollToPos = async (selector, scrollOptions) => {
await page.locator(selector).scroll(scrollOptions);
};

export const waitFor = async selector => {
await page.locator(selector).wait();
};
99 changes: 95 additions & 4 deletions lib/buttons-and-links/__tests__/KButton.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@
import { shallowMount } from '@vue/test-utils';
import KButton from '../KButton.vue';
import { renderComponent, takeSnapshot } from '../../../jest.conf/visual.testUtils';
import {
renderComponent,
takeSnapshot,
delay,
click,
hover,
} from '../../../jest.conf/visual.testUtils';

describe('KButton', () => {
describe('icon related props', () => {
Expand Down Expand Up @@ -67,9 +73,94 @@ describe('KButton', () => {
});

describe.visual('KButton Visual Tests', () => {
it('renders correctly with default props', async () => {
await renderComponent('KButton', { text: 'Test Button' });
await takeSnapshot('KButton - Default');
it('renders correctly with different appearances', async () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

await renderComponent('KButton', {
text: 'Raised Button',
primary: false,
appearance: 'raised-button',
});
await takeSnapshot('KButton - Secondary Raised Button', { widths: [400], minHeight: 512 });

await renderComponent('KButton', {
text: 'Flat Button',
primary: true,
appearance: 'flat-button',
});
await takeSnapshot('KButton - Primary Flat Button', { widths: [400], minHeight: 512 });

await renderComponent('KButton', {
text: 'Flat Button',
primary: false,
appearance: 'flat-button',
});
await takeSnapshot('KButton - Secondary Flat Button', { widths: [400], minHeight: 512 });

await renderComponent('KButton', { text: 'Basic Link', appearance: 'basic-link' });
await takeSnapshot('KButton - Basic Link', { widths: [400], minHeight: 512 });
});
it('renders correctly when disabled', async () => {
await renderComponent('KButton', {
text: 'Raised Button',
disabled: true,
appearance: 'raised-button',
});
await takeSnapshot('KButton - Disabled Raised Button', { widths: [400], minHeight: 512 });

await renderComponent('KButton', {
text: 'Flat Button',
disabled: true,
appearance: 'flat-button',
});
await takeSnapshot('KButton - Disabled Flat Button', { widths: [400], minHeight: 512 });
});
it('renders with hover state', async () => {
await renderComponent('KButton', { text: 'Raised Button', appearance: 'raised-button' });
await hover('button');
await delay(4000);
await takeSnapshot('KButton - Raised Button Hover', { widths: [400], minHeight: 512 });

await renderComponent('KButton', { text: 'Flat Button', appearance: 'flat-button' });
await hover('button');
await delay(4000);
await takeSnapshot('KButton - Flat Button Hover', { widths: [400], minHeight: 512 });
});
it('renders correctly with icon and iconAfter', async () => {
await renderComponent('KButton', { text: 'Icon Button', icon: 'add' });
await takeSnapshot('KButton - With Icons', { widths: [400], minHeight: 512 });

await renderComponent('KButton', { text: 'Icon After', iconAfter: 'video' });
await takeSnapshot('KButton - With Icons After', { widths: [400], minHeight: 512 });
});
it('renders correctly with KDropdownMenu slot and shows options on click', async () => {
await renderComponent(
'KButton',
{ text: 'Button with Dropdown' },
{
menu: {
content: 'KDropdownMenu',
props: { options: ['Option 1', 'Option 2'] },
},
}
);
await click('button');
await takeSnapshot('KButton - Dropdown Opened', { widths: [400], minHeight: 512 });
});
it('should render the slot when the slot has content', async () => {
await renderComponent(
'KButton',
{ text: 'Button' },
{
default: '<span>Default Slot</span>',
}
);
await takeSnapshot('KButton - With Default Slot', { widths: [400], minHeight: 512 });
});
});
});
Loading