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

tests: add initial test suite for CoreMenu #11934

Merged
merged 12 commits into from
Apr 8, 2024

Conversation

EshaanAgg
Copy link
Contributor

Summary

  • Adds the complete tests for CoreMenuDivider
  • The CoreMenuOption component has two major parts:
    • Toggle component which controls where the subroutes are to be rendered or not
    • The subroutes
      In this PR, I add the complete test suite for the former, and plan to add the one for later in the next PR for easier reviews
  • Added a validator for the subRoutes props based on the usage of the same in the component

Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@MisRob MisRob requested a review from AlexVelezLl March 12, 2024 11:22
@AlexVelezLl AlexVelezLl self-assigned this Mar 13, 2024
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 @EshaanAgg! You are making great progress towards using vue-testing library in Kolibri!! I left just a couple of comments and considerations :).

@EshaanAgg
Copy link
Contributor Author

Hi! I'm so sorry. It seems like this the updates on the PR just completely got lost in my inbox. Would work on addressing the review. Thanks a lot @AlexVelezLl!

@github-actions github-actions bot added DEV: tools Internal tooling for development SIZE: very large labels Apr 4, 2024
yarn.lock Outdated Show resolved Hide resolved
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.

Thank you for all your patience! @EshaanAgg Promise that now these will be last comments about things that I had not noticed before. After this we should be ready to go =).

const menuItem = screen.getByRole('menuitem');

// Clicking should show the subroutes
await fireEvent.click(menuItem);
Copy link
Member

Choose a reason for hiding this comment

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

Im sorry I havent noticed this before, until now I thought we were using the user-event library 😅, but I think Its worth getting used to preferring the userEvent library instead of fireEvent, as it is the recommended way for the most use cases, so we can replace all these clicks and keydowns methods of fireEvent with the userEvent.click or userEvent.type methods.

Copy link
Contributor Author

@EshaanAgg EshaanAgg Apr 8, 2024

Choose a reason for hiding this comment

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

Hey! Nice catch. I have updated all the instances of fireEvent.click with userEvent.click as instructed. The userEvent library does not provide a method to simulate a keyDown event with respect to a particular element, as we require in the case of menuitem and the Enter key. I tried using the .keyboard and .type methods, but they didn't seem to trigger the desired effects. Is there any way that I'm missing?

Copy link
Member

Choose a reason for hiding this comment

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

(Just for the reference) It seems that with userEvent.type was not working because before typing on the element, it internally clicks on the element to have the focus. And as we have the handler also on the click element, it was calling the function twice, so visibleSubMenu=!visibleSubMenu called twice ends in the sub menu not being visible. So its ok to keep the fireEvent here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh got it! Thanks so much for the explanation!

import userEvent from '@testing-library/user-event';
import VueRouter from 'vue-router';
import CoreMenuOption from '../CoreMenuOption.vue';
import '@testing-library/jest-dom';
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 now remove this import, as we already have it in the jest setup.js file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried that, but it didn't work. Removing the import for userEvent started giving a linting error, as well as an error when running the test suite (saying that userEvent is not defined).

Copy link
Member

Choose a reason for hiding this comment

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

Oh I was talking about the import '@testing-library/jest-dom'; 😅 but it seems that you already deleted it 🤗

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 for your work here @EshaanAgg! LGTM!!

@AlexVelezLl AlexVelezLl merged commit d6d78fd into learningequality:develop Apr 8, 2024
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants