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.

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

[KListWithOverflow]: Add unit tests #920

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

you-think-you-know-me
Copy link

@you-think-you-know-me you-think-you-know-me commented Feb 2, 2025

Description

We currently don't have unit tests for KListWithOverflow. It adds unit tests for KListWithOverflow.

Issue addressed

Closes #833

Changelog

  • Description: Add unit tests for KListWithOverflow
  • Products impact: NA
  • Addresses: [KListWithOverflow]: Add unit tests #833
  • Components: KListWithOverflow
  • Breaking: Will this change break something in a consumer? no
  • Impacts a11y: Does this change improve a11y or adds new features that can be used to improve it? no
  • Guidance: NA

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

Comments

😄

Copy link
Member

@AlexVelezLl AlexVelezLl left a comment

Choose a reason for hiding this comment

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

Thanks a lot @you-think-you-know-me! This looks super good! I have left just some minor comments, and requested a couple of additional test cases, but apart from that everything else looks good!

const MORE_BUTTON_WIDTH = 20;

// Utility function to wait for two nextTicks after mounting
async function waitForNextTicks(wrapper) {
Copy link
Member

Choose a reason for hiding this comment

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

Is this method actually needed? I just removed it and all tests kept working fine.

const wrapper = mount(KListWithOverflow, {
propsData: { items },
slots: {
item: '<div class="list-item">{{ typeof item === "object" ? item.label : item }}</div>',
Copy link
Member

Choose a reason for hiding this comment

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

We dont need this ternary here, because when we have a divider we dont render the item slot.


expect(wrapper.vm.overflowItems).toEqual(['Item 3']);
expect(wrapper.vm.isMoreButtonVisible).toBe(true);
expect(listItems.at(1).element).toHaveStyle({ visibility: 'hidden' });
Copy link
Member

Choose a reason for hiding this comment

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

We can be a little bit more explicit with the code so its easier to understand for other people. For example here, we can tweak this code extracting variables and adding comments to be more expressive with the intention. For example:

const divider = listItems.at(1).element;
// The divider its the last non-overflowed item, it should be hidden
expect(divider).toHaveStyle({ visibility: 'hidden' });

expect(wrapper.vm.isMoreButtonVisible).toBe(true);
expect(listItems.at(1).element).toHaveStyle({ visibility: 'hidden' });
});
});
Copy link
Member

Choose a reason for hiding this comment

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

Lets add just a couple of tests more:

  • It renders the divider slot if we pass a divider object.
  • The first overflowed item shouldnt be a divider
  • All items are shown if they fit if we dont render the more button
    • This means: If our LIST_WRAPPER_WIDTH is 101, we can still render two items of width 50 for example.
    • With this we will find an error because in this line we set the item visibility to visible but we dont restore the position to unset. So if you can include this line too it would be nice.

@EshaanAgg
Copy link
Contributor

EshaanAgg commented Feb 17, 2025

Hi! I was passing by and felt that the way we are trying to test this component is a bit counter-intuitive: we are trying to set the sizes of the elements via JS, which is not the way the component would be used! Most probably, the user would have a pre-defined markup for the item slot, which will be passed to it, and the size of the item would be determined by it's styling! So shouldn't our tests also try to mimic the same?

Something like following perhaps?

import { mount } from '@vue/test-utils';
import KListWithOverflow from '../KListWithOverflow.vue';

// Constants for styling
const LIST_STYLES = `
  .list-item {
    display: inline-block;
    white-space: nowrap;
  }
  .list-item.fits {
    width: 80px;
    height: 20px;
  }
  .list-item.overflows {
    width: 25px;
    height: 20px;
  }
  .list-item.medium {
    width: 60px;
    height: 20px;
  }
  .list-item.small {
    width: 20px;
    height: 20px;
  }
`;

describe('KListWithOverflow.vue', () => {
  // Add styles to document before tests
  // TODO: Check if it actually works with Enzyme
  beforeAll(() => {
    const styleElement = document.createElement('style');
    styleElement.textContent = LIST_STYLES;
    document.head.appendChild(styleElement);
  });

  it('renders all items when they fit within available space', async () => {
    const items = ['Item 1'];

    const wrapper = mount(KListWithOverflow, {
      propsData: { items },
      slots: {
        // Can now just use the class on the list item to control its size
        // The class clearly indicates the size of the item and the intent of the test
        item: '<div class="list-item fits">{{ item }}</div>',
        more: '<button class="more-button">More</button>',
      },
    });

    await wrapper.vm.$nextTick();

    expect(wrapper.vm.overflowItems).toEqual([]);
    expect(wrapper.vm.isMoreButtonVisible).toBe(false);
    
    const listItem = wrapper.find('.list-item');
    expect(listItem.element).toHaveStyle({ 
      visibility: 'visible',
      position: 'unset'
    });
  });

  // Can easily write more tests by switching the class of the list item
});

This is just a brief idea, and I have never actually ran this code to ensure it works! but we probably should spend some time But seeing this PR, I thought that it might be worthwhile to look at this alternative approach. I would like to advocate for it as it does not make use of JS to control the sizing of the elements, and thus helps us to get rid of many "magic constants" that appear throughout the tests, or of having to manage the rendering ourselves from inside the tests.

What do you think about this @AlexVelezLl @you-think-you-know-me?

@AlexVelezLl
Copy link
Member

Hey @EshaanAgg, thanks for jumping in. Yes! This is a very good alternative too, although Im not a big fan of having to scroll up to the definition of LIST_STYLES and look for the specific width of a given item to understand the test, in that regard, i feel that having something like setElementSize express better the intention of the test by explicitly declaring the inputs and outputs within the same test case, and it makes it looks a bit more easy to read.

A cons of overwiting the getBoundingClientRect of the element is that if in the future we change this method to get the sizes, we will need to update this method too, although we will just only need to update it inside the setElementSize method as it is well encapsulated, but its nice to have some alternatives.

I think we could implement something more complicated like defining unique classes dynamically in each test case and assigning them to the items inside a setElementSize method (or override the classes, although that could easily break the independency of the tests and one could affect the following ones), if this sounds good, @you-think-you-know-me can give it a try, but if we are spending too much time on it, we I think we can move forward with overriding the getBoundingClientRect method, at least for now, as the intention right now if making sure that the inner logic of setOverflowItems is working well. To have a better accuracy of the actual DOM rendering we can always use visual tests to complement these unit tests, although if in the future we found a better way to set these sizes, we can just change the setElementStyle method, and all the tests should keep working fine :).

@EshaanAgg
Copy link
Contributor

Thanks, @AlexVelezLl, for the detailed response. Even I do agree that having to scroll to the top of the document breaks the encapsulation of the test logic in the test itself approach and thus might be a bit counterintuitive. We can definitely ideate better ways to do the same later. Meanwhile, we will currently keep the main aim of this PR to introduce a test suite that covers all the edge cases! Thank a lot again :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[KListWithOverflow]: Add unit tests
3 participants