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

fix(tests and events): resolves unsupported events and passing tests #57

Merged
merged 5 commits into from
May 24, 2022

Conversation

mikerob215
Copy link
Contributor

@mikerob215 mikerob215 commented May 24, 2022

fixes #51
fixes #23

I updated the test suite to use names for event handlers in camel case like users do( ie: onClick was being tested as onclick). Once this is corrected,
a number of tests will fail as JSDOM doesnt have support for them. When a user writea a component with a prop like onPointerEnter, preact will do something like 'on' + eventName.toLowerCase() in elem and in jsdom that wont work. It leads to preact creating an event handler called PointerEnter rather than pointerEnter.

To solve I've overridden fireEvent similar to how its done in RTL, the difference is that we make the same check as preact, eventName.toLowerCase() in dom, to determine what the dispatch event's name should be.

Currently the PTL library typings indicate that this is supported and it also works fine in RTL.

First I fixed a test case then spent some time in a bugger to figure who was renaming the event names

Checklist:

  • [N/A ] Documentation added
  • [ X] Tests
  • [ N/A] Typescript definitions updated
  • [X ] Ready to be merged

@@ -169,9 +172,9 @@ test('onInput works', () => {
test('calling `fireEvent` directly works too', () => {
Copy link
Member

@JoviDeCroock JoviDeCroock May 24, 2022

Choose a reason for hiding this comment

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

Let's add a test specifically for the issue tagged here or is that one not intended to be solved by this?

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 added those to the big list of tests, they should be covered, they were just missing before. With this bug they would have passing tests anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The other related issue, I think the bottom issue in open was about events that were already covered in this test suite too. All passed.

Copy link
Member

Choose a reason for hiding this comment

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

updated the description to link to the issue as that was missing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would it be worth adding reverse tests? I dont know if if thats a thing but we could write tests that check 'onpointerenter' in dom and once they fail, we'd know we could delete these patches?

Copy link
Member

Choose a reason for hiding this comment

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

I would leave them out for now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Worth adding them in another pr? Like I said, looking for ways to contribute

src/file-event.js Outdated Show resolved Hide resolved
src/pure.js Outdated Show resolved Hide resolved
mikerob215 and others added 2 commits May 24, 2022 16:51
Co-authored-by: Jovi De Croock <decroockjovi@gmail.com>
Co-authored-by: Jovi De Croock <decroockjovi@gmail.com>
src/file-event.js Outdated Show resolved Hide resolved
src/fire-event.js Outdated Show resolved Hide resolved
typo

Co-authored-by: Jovi De Croock <decroockjovi@gmail.com>
@JoviDeCroock JoviDeCroock merged commit d547700 into testing-library:master May 24, 2022
@mikerob215
Copy link
Contributor Author

Thanks for the support @JoviDeCroock

@github-actions
Copy link

🎉 This PR is included in version 3.0.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@JoviDeCroock
Copy link
Member

No worries should be published now @mikerob215 will focus on #50 now

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

Successfully merging this pull request may close these issues.

touch events (onTouchEnter) do not work in jest jsdom Can not test fireEvent.touchStart side effects
2 participants