-
Notifications
You must be signed in to change notification settings - Fork 14
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: Write unit tests for settings page and components #290
Conversation
|
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.
This is awesome! Yay tests! This will really help increase confidence that things are doing what they should be.
I left some small comments about combining tests and focusing testing unique display logic. Overall, great work!
src/components/settings/DeleteAccountSetting/DeleteAccountSetting.test.tsx
Outdated
Show resolved
Hide resolved
src/components/settings/DeleteAccountSetting/DeleteAccountSetting.test.tsx
Show resolved
Hide resolved
src/components/settings/DeleteAccountSetting/DeleteAccountSetting.test.tsx
Outdated
Show resolved
Hide resolved
src/components/settings/DeleteAccountSetting/DeleteAccountSetting.test.tsx
Show resolved
Hide resolved
src/components/settings/EditBirthplaceSetting/EditBirthplaceSetting.test.tsx
Outdated
Show resolved
Hide resolved
src/components/settings/EditNameSetting/EditNameSetting.test.tsx
Outdated
Show resolved
Hide resolved
src/components/settings/EditResidenceSetting/EditResidenceSetting.test.tsx
Outdated
Show resolved
Hide resolved
src/components/settings/EditThemeSetting/EditThemeSetting.test.tsx
Outdated
Show resolved
Hide resolved
src/components/settings/EditThemeSetting/EditThemeSetting.test.tsx
Outdated
Show resolved
Hide resolved
Updated all the points except the Tell me if we need to tweak anything else! (Kudos for the comments) |
This is awesome! Thank you so much for the tests and the changes. If you can't figure out a way to eliminate the |
I did a last cleanup but we'll figure out a way for the famous |
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'm going to go ahead and approve this because all the tests look great! If you're able to figure out how to remove as unknown
, even better, but I'd consider that a bonus—don't spend too much time on that if it's not leading anywhere.
Leave a comment when you feel ready to go.
Re-reviewed I think we're good to go🎉 |
What changed?
Resolves #235
DeleteAccountSetting
EditBirthplaceSetting
EditMinorSetting
EditNameSetting
EditResidenceSetting
EditThemeSetting
Why?
Providing test coverage in the user settings functionalities
How was this change made?
Mocked external services to isolate the component behavior, particularly API calls related to user data.
How was this tested?
By testing tests.