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

Add some simple unit tests for SettingsHandler #12597

Merged

Conversation

nlebeck
Copy link
Contributor

@nlebeck nlebeck commented Feb 27, 2024

This is my first attempt at adding unit tests in the Dolphin codebase. It looks like most of the unit test files don't have many comments, but please let me know if test plan comments would be helpful here, or if you have any feedback on the scope or style of the tests.

@JosJuice
Copy link
Member

Instead of having tests that both encrypt and decrypt and then check the result, I would suggest having tests that do just encryption and just decryption. (Possibly in addition to ones that do both. GetValueOnSameInstance can be kept the way it is, for instance.) SettingsHandler is implementing a format defined by Nintendo, and we have to make sure we read and write files in a way that's compatible with Nintendo's files.

Additionally, if you're able to, it would be nice to have tests for the edge case implemented in PR #8704.

@nlebeck
Copy link
Contributor Author

nlebeck commented Feb 28, 2024

Sounds good, I'll make that change!

Adding test coverage for the edge case in PR #8704 seems fun. I'll either do that in here or leave it for a follow-up PR depending on how tricky it is.

@nlebeck
Copy link
Contributor Author

nlebeck commented Feb 29, 2024

Changed (most of) the tests to test encryption and decryption separately. PTAL! I'm not used to writing tests where the input/output data is raw u8 arrays, so let me know if there are any style concerns.

I left a TODO for PR #8704. I'd like to add that coverage, but I have pretty limited free time these days, so it might be a while before I get to it.

Source/UnitTests/Common/SettingsHandlerTest.cpp Outdated Show resolved Hide resolved
Source/UnitTests/Common/SettingsHandlerTest.cpp Outdated Show resolved Hide resolved
Source/UnitTests/Common/SettingsHandlerTest.cpp Outdated Show resolved Hide resolved
Source/UnitTests/Common/SettingsHandlerTest.cpp Outdated Show resolved Hide resolved
@nlebeck
Copy link
Contributor Author

nlebeck commented Feb 29, 2024

Thanks for the review!

Copy link
Member

@JosJuice JosJuice left a comment

Choose a reason for hiding this comment

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

Please squash your commits into one.

Source/UnitTests/Common/SettingsHandlerTest.cpp Outdated Show resolved Hide resolved
Source/UnitTests/Common/SettingsHandlerTest.cpp Outdated Show resolved Hide resolved
Source/UnitTests/UnitTests.vcxproj Show resolved Hide resolved
@nlebeck nlebeck force-pushed the settingshandler-unittests-thirdtry branch from 7e445a6 to 7793cf6 Compare March 1, 2024 01:11
@nlebeck
Copy link
Contributor Author

nlebeck commented Mar 1, 2024

Thanks for the review! Commits squashed.

@nlebeck nlebeck force-pushed the settingshandler-unittests-thirdtry branch 3 times, most recently from 2a8b1a6 to 691cb29 Compare March 1, 2024 21:19
@nlebeck nlebeck force-pushed the settingshandler-unittests-thirdtry branch from 691cb29 to 0344ec6 Compare March 2, 2024 04:53
@nlebeck nlebeck requested a review from JosJuice March 3, 2024 19:13
@OatmealDome OatmealDome merged commit 6372bf5 into dolphin-emu:master Mar 3, 2024
11 checks passed
@nlebeck nlebeck deleted the settingshandler-unittests-thirdtry branch March 3, 2024 23:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants