-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Preferences/index.jsx unit test file #1779
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
Preferences/index.jsx unit test file #1779
Conversation
… few UI elements in preferences
we can't test if the ui elements update because that would require passing in new props in response to a parent state change and this is a unit test, so no parent component
Release Environmentsp5.js-web-editor |
}); | ||
|
||
// expect that setFontSize has been called once with the argument 14 | ||
expect(props.setFontSize).toHaveBeenCalledTimes(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know we were working on this before we totally got the distinction between unit and integration tests, but this feels like it's testing implementation details, right? Like, what if the names of the Redux functions changed, then the test would break even though the UI hadn't changed at all (i.e. the input and output hadn't changed). Maybe it doesn't even make sense to test this component without Redux because it relies on it so heavily?
Anyway... you totally don't have to change this and maybe it would be good practice for me anyway to try to update it 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oooo I see what you mean! The input is the user doing something but then the most abstract output that we could test is that the props.setFontSize gets called because nothing on the page actually changes since the parent component hasn't passed in a new set of props in response... I was understanding "implementation detail" as not testing class methods that are hidden within the component, but given a Preferences component ,we can assert that if we provide certain callbacks and then act on the component, the component will call those callbacks with the right values but it doesn't matter what it did in the middle before it finally called the function, but I think it does make sense to test it even more abstractly and say that output needs to be a change that the user senses (or maybe the output is that the right redux action is created?).
I totally see what you mean! It does make more sense to test it with redux
I was able to remove all of the data-testids by fixing a web accessibility issue! It's cool that by writing these tests by selecting elements by their accessible name, it's easy to see which elements are not accessible 😄 |
Adds a test file that tests that clicking on the preferences UI elements correctly calls the corresponding setter functions that are passed in as props and passes in the correct arguments when calling them.
For example, it tests that clicking on the Plus button for font size will then call props.setFontSize(props.fontSize + 2).
There should be multiple tests per Preference setting.
Mostly uses roles to query for ui elements, but I had to use data-testid attributes for two of the components as a last resort.
I have verified that this pull request:
npm run lint
)develop
branch. (If I was asked to make more changes, I have made sure to rebase ontodevelop
then too)