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

[test] Test vertical scrollbar #1932

Merged
merged 4 commits into from Jun 21, 2021

Conversation

oliviertassinari
Copy link
Member

Add a test case for #1831 (comment). We could have easily missed it. I have added a visual and function test base the functional test is flaky to catch the issue. The order of resolution between the two effects varies.

@oliviertassinari
Copy link
Member Author

Ok, I got the screenshot test to fail as expected:

Capture d’écran 2021-06-19 à 22 20 57

The function test is not failing however, the events fire twice. It seems that triggering this problem is actually hard. AFAIK we don't have any test case for the scrollbar, so these new ones won't harm.

// A function test counterpart of ScrollbarOverflowVerticalSnap.
it('should not have a horizontal scrollbar if not needed', () => {
const TestCase = () => {
const data = useData(100, 1);
Copy link
Member

Choose a reason for hiding this comment

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

this is a simple case. The test should cover when we have the total column width that is within the container width - scroll width ie
container width of 500, total column width of 495 no scroll bar if no vertical scrollbar, also we could check with a total width of 485 so no scroll bar even if there is a vertical one
Does that make sense?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is interesting but outside of the scope of the problem I'm trying to solve. It can be a new test case in another PR. This one is as simple as possible, while still enough to catch the regression we were close to release.

@oliviertassinari oliviertassinari merged commit 26fca7e into mui:master Jun 21, 2021
@oliviertassinari oliviertassinari deleted the increase-test-coverage branch June 21, 2021 13:17
@oliviertassinari
Copy link
Member Author

Ok, so now, if somebody try to apply the changes I did in #1831, the CI will fail.

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.

None yet

2 participants