The Wayback Machine - https://web.archive.org/web/20201113041458/https://github.com/mui-org/material-ui/issues/23398
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Slider] Testing onChangeCommitted from click #23398

Open
Npervic opened this issue Nov 5, 2020 · 14 comments
Open

[Slider] Testing onChangeCommitted from click #23398

Npervic opened this issue Nov 5, 2020 · 14 comments

Comments

@Npervic
Copy link

@Npervic Npervic commented Nov 5, 2020

  • The issue is present in the latest release.
  • [ x] I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 馃槸

When testing the slider functionality using rtl and jest no significant DOM update happens when clicking on labels (or doing mousedown or other events)

Expected Behavior 馃

DOM should update with the selected mark as Slider value

Steps to Reproduce 馃暪

In the codesadbox example i tried to test this component of a slider value change on label (slider mark) click. But i cant seem to get the onChangeCommitted to trigger when testing. The slider component dose not indicate any change
https://codesandbox.io/s/romantic-goldstine-gpjbj?file=/src/App.test.js

Context 馃敠

I am trying to test onChangeCommitted in the slider component. But ive tried up and down to test this event on the slider and its getting really frustrating as there dosent seem to be many threads or SO posts about so I might just be missing something trivial. Ive added a sample on codesandbox. Hope some one can help thanks :)

Your Environment 馃寧

Tech Version
Material-UI v4.3.1
React v16.8.4
Browser Chrome
TypeScript Y (but not in example)
etc.
@eps1lon
Copy link
Member

@eps1lon eps1lon commented Nov 5, 2020

Thanks for the report.

You have a controlled component but use defaultValue which is only valid with uncontrolled components. The console warns about this.

When I use value everything seems to behave as expected. Note that onChangeCommitted is different to onChange. onChangeCommitted is similar to oninput on native elements.

Could you expand a bit on the "Steps to reproduce"? Just the user interface is not enough. What did you do? What did you expect? What happened instead?

@Npervic
Copy link
Author

@Npervic Npervic commented Nov 5, 2020

@eps1lon I replaced the defaultValue with value and updated the original post with suggested info.

@eps1lon
Copy link
Member

@eps1lon eps1lon commented Nov 5, 2020

Right, it's about testing not actual user interaction. The Slider is using pointer position which a programmatic click doesn't include. You would need to calculate the pointer position when the pointer would click the specified element and pass that.

I can't help you with that right now.

@eps1lon eps1lon added test and removed status: incomplete labels Nov 5, 2020
@eps1lon eps1lon changed the title Cant test `onChangeCommitted` in <Slider /> component [Slider] Testing onChangeCommitted from click Nov 5, 2020
@Npervic
Copy link
Author

@Npervic Npervic commented Nov 5, 2020

@eps1lon thanks il give your suggestion a shot

@Npervic
Copy link
Author

@Npervic Npervic commented Nov 6, 2020

Hey @eps1lon I tried passing clientX and clientY to mousedown events options but what ever values i pass it seems that i allways end up with the same change(i.e. the end result is always 1h+ and none of the other values seems to be targetable). Also code coverage says that even when testing like this the onChangeCommitted is not called (althought the value of aria-valuenow changes on slider input).
Screenshot from 2020-11-06 14-43-41

I'm sorry, this probably is not a bug but it seems to me that testing these should be more straightforward so I made this issue. Any help is appreciated and I think it will help other people with the same issue.

@oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Nov 6, 2020

@Npervic Did you consider asking on StackOverflow? This sounds like a support request.

@eps1lon
Copy link
Member

@eps1lon eps1lon commented Nov 8, 2020

@Npervic mousedown does not commit the change. You probably need click.

@oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Nov 8, 2020

It seems that we can improve our tests. It's more logical to fire up and down on the same node:

diff --git a/packages/material-ui/src/Slider/Slider.test.js b/packages/material-ui/src/Slider/Slider.test.js
index f7afee0554..85956e704c 100644
--- a/packages/material-ui/src/Slider/Slider.test.js
+++ b/packages/material-ui/src/Slider/Slider.test.js
@@ -59,7 +59,7 @@ describe('<Slider />', () => {
     const slider = getByRole('slider');

     fireEvent.mouseDown(container.firstChild);
-    fireEvent.mouseUp(document.body);
+    fireEvent.mouseUp(container.firstChild);

     expect(handleChange.callCount).to.equal(1);
     expect(handleChangeCommitted.callCount).to.equal(1);

We could also test the end value, based on the position:

diff --git a/packages/material-ui/src/Slider/Slider.test.js b/packages/material-ui/src/Slider/Slider.test.js
index f7afee0554..5f815a8578 100644
--- a/packages/material-ui/src/Slider/Slider.test.js
+++ b/packages/material-ui/src/Slider/Slider.test.js
@@ -56,13 +56,25 @@ describe('<Slider />', () => {
     const { container, getByRole } = render(
       <Slider onChange={handleChange} onChangeCommitted={handleChangeCommitted} value={0} />,
     );
+    stub(container.firstChild, 'getBoundingClientRect').callsFake(() => ({
+      width: 100,
+      left: 0,
+    }));
     const slider = getByRole('slider');

-    fireEvent.mouseDown(container.firstChild);
-    fireEvent.mouseUp(document.body);
+    fireEvent.mouseDown(container.firstChild, {
+      buttons: 1,
+      clientX: 10,
+    });
+    fireEvent.mouseUp(container.firstChild, {
+      buttons: 1,
+      clientX: 10,
+    });

     expect(handleChange.callCount).to.equal(1);
+    expect(handleChange.args[0][1]).to.equal(10);
     expect(handleChangeCommitted.callCount).to.equal(1);
+    expect(handleChangeCommitted.args[0][1]).to.equal(10);

     slider.focus();
     fireEvent.keyDown(slider, {

@Npervic Why are you testing this in jsdom and not a browser?

@Npervic
Copy link
Author

@Npervic Npervic commented Nov 8, 2020

Why wouldn't I? I dont see a reason a component as such would have problems being tested in jsdom or am i missing something @oliviertassinari ?

@oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Nov 8, 2020

@Npervic jsdom has no layouting, all the elements are width: 0, height: 0.

@Npervic
Copy link
Author

@Npervic Npervic commented Nov 8, 2020

@oliviertassinari I see so testing this in js dom is not possible if I understand you correctly? How should I go about testing this then?

@oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Nov 9, 2020

@Npervic I have added the good first issue as we can improve the test cases of the component: #23398 (comment). If this is something you could be interested feel free to pick it up :).

Otherwise, in your case, the Slider relies on mouse down & mouse up & the layout dimension to trigger its event. I would suggest you to either 1. apply the same approach as the proposed diff or 2. test the thing in a real browser.

@Npervic
Copy link
Author

@Npervic Npervic commented Nov 9, 2020

@oliviertassinari this sounds actually interesting, and I'd love to contribute if I can. Il take a look at solving this over the work week for my case, and on the weekend I could try and contribute from my personal GH account.

@oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Nov 9, 2020

@Npervic Great :), to remove any source possible of confusion. We apply the "good first issue" anytime there is a working diff that very likely solves the problem, in our case, it's #23398 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.