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

KTable: Migrate the test suite to VTL and add new tests #961

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

Conversation

Pandaa007
Copy link
Contributor

Description

Migrated the test suite from enzyme to VTL for KTable so that we use the accessible roles and user-facing queries to test the behaviour of the component.

  • Reduced the boilerplate associated with the exsiting tests to make the tests conscise and more expressive.
  • Added a stronger test suite for arrow key naviagation.
  • Added a new simple test suite as a proof of concept for tab and shift-tab navigation.
  • Restructured the tests so that all of live under a directory.

Changelog

  • Description: Enhanced the test coverage for KTable
  • Products impact: none
  • Addresses: -
  • Components: -
  • Breaking: no
  • Impacts a11y: no
  • Guidance: -

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

@MisRob
Copy link
Member

MisRob commented Mar 17, 2025

Thanks @Pandaa007! @BabyElias you're welcome to assign yourself as a reviewer if you'd like, or I can ask another reviewer later on.

Copy link
Contributor

@EshaanAgg EshaanAgg left a comment

Choose a reason for hiding this comment

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

Thanks @Pandaa007 for your contribution. Overall I am a huge fan of how expressive the test suite now has become, specially with the use of proper role selectors to check the rendered content instead of using DOM selectors as we had to earlier do with enzyme. The introduction of the multiple utilities is also much appreciated.

I have left a few comments, most of them can be safely considered as nits. Feel free to address them as you have the time, or ignore them if they feel irrelevant. Thanks again.

assertActiveBackground(rowIdx, colIdx, allCells);
}
}
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add some more test cases here? A test case with some focusable elements inside some of the cells might be good to have.

Also KTable has a hovering functionality, wherein if we use the pointer to hover over any cell, the whole row is highlighted. So it might be nice to have a test suite that:

  • Checks this behaviour only when the pointer is used
  • When pointer and tab focus are used on cells in
    • Different rows
    • Same row but different cells
    • Same cell

This test suite might be a bit too complex for the initial draft, and would help to safeguard a lot of regressions going forward in a component as nuanced as KTable. If you feel that this might be a but too much to implement for the scope of this PR, please feel free to drop a comment and we can try to address them in a new PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestions @EshaanAgg. I have added some basic tests related to the pointer functionality as well as the focusable element functionality as you directed in the latest commits. I think so the test suite is already quite involed as of now, so I didn't add the tests composing the pointer and tab functionality together in this PR.

I would like to open a new PR for them in which I would add tests for:

  • Multiple focusable elements in a cell
  • Having disabled elements in a cell
  • Checking that the navigation doesn't remain trapped inside or outside the table

I think that this functionality would be better suited there. Hope that is a right scale of balance for this PR and future work.

@BabyElias BabyElias self-requested a review March 17, 2025 13:15
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.

3 participants