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

feat(fireEvent): make read-only event target properties definable #1020

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

9inpachi
Copy link

@9inpachi 9inpachi commented Aug 30, 2021

What:

This makes read-only properties of the target element like scrollWidth definable through fireEvent or createEvent.

Why:

Because as of now it's not possible to test events with read-only target properties. If a component for example makes use of scrollWidth to calculate the scroll width inside a scroll event and thus applied some sort of limit to the scroll, it can't be tested for that event.

For example, this test throws an error but passes after the changes in this PR.

it("scrolls the list to right", async () => {
  render(<TestComponent length={20} />);
  const list = screen.getByRole("list");

  await act(async () => {
    fireEvent.scroll(list, { target: { scrollWidth: 100, scrollLeft: 10 } });
  });

  expect(screen.getByText("List is scrolled")).toBeTruthy();
});

How:

By using Object.defineProperties and modifying the read-only target properties. The solution is the same as or similar to handling files but it's generic for all other properties as well.

Checklist:

  • Documentation added to the
    docs site
  • Tests
  • TypeScript definitions updated
  • Ready to be merged

@codesandbox-ci
Copy link

codesandbox-ci bot commented Aug 30, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit b901f7f:

Sandbox Source
react-testing-library-examples Configuration

@9inpachi
Copy link
Author

9inpachi commented Aug 30, 2021

I will add tests and other items on the checklist once I can get some sort of confirmation if this is all right. There may be a reason to keep the read-only target properties as is that I don't know of. So please let me know if this is fine so I can move forward.

Peace. :)

@codecov
Copy link

codecov bot commented Aug 31, 2021

Codecov Report

Merging #1020 (b901f7f) into main (edffb7c) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main     #1020   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           24        24           
  Lines          998       999    +1     
  Branches       326       326           
=========================================
+ Hits           998       999    +1     
Flag Coverage Δ
node-12 100.00% <100.00%> (ø)
node-14 100.00% <100.00%> (ø)
node-16 100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/events.js 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@9inpachi 9inpachi marked this pull request as ready for review August 31, 2021 14:41
@9inpachi 9inpachi force-pushed the pr/modifiable-event-properties branch from df76448 to b55ffc1 Compare September 26, 2022 08:35
@juanca
Copy link
Contributor

juanca commented Sep 26, 2022

I will add tests and other items on the checklist once I can get some sort of confirmation if this is all right. There may be a reason to keep the read-only target properties as is that I don't know of. So please let me know if this is fine so I can move forward.

Peace. :)

As far as I can tell, the an Element's scrollWidth property is read-only.

What does the rest of your code do to the scrollWidth? I would think it shouldn't change after emitting the event.

@9inpachi
Copy link
Author

9inpachi commented Sep 26, 2022

What does the rest of your code do to the scrollWidth?

The code just reads and uses the scrollWidth which comes from a scroll event emitted by the browser.

I would think it shouldn't change after emitting the event.

It doesn't. But I think the emitted event from the test (as shown in the example) should be able to specify it as part of the event itself. So the read-only usage of scrollWidth by the component under test can be tested.

@9inpachi
Copy link
Author

What I am trying to test is this scroll behavior.

Before horizontal scroll, the left arrow is invisible:

image

After horizontal scroll, the component uses scrollLeft and scrollWidth to determine if there is sufficient scroll to show the left arrow:

image

For firing the event with this horizontal scroll, I need to specify scrollWidth and scrollLeft properties.

@9inpachi 9inpachi force-pushed the pr/modifiable-event-properties branch from e3b2c99 to b901f7f Compare October 11, 2022 09:13
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.

None yet

2 participants