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 unit tests for HashingContext #43459

Merged
merged 1 commit into from
Mar 8, 2021

Conversation

maxmutant
Copy link
Contributor

This is part of issue #43440.
The unit tests should cover the basic use cases of HashingContext.

I thought about making tests for invalid input as well (e.g. call to update with an empty PackedByteArray), but all these cases are already handled with proper asserts, so I omitted them (as they would pollute the output with ERROR messages).

Ps.: This is my first contribution. So any feedback/critic is very welcome!

@Calinou Calinou added this to the 4.0 milestone Nov 11, 2020
@Calinou
Copy link
Member

Calinou commented Nov 11, 2020

but all these cases are already handled with proper asserts, so I omitted them (as they would pollute the output with ERROR messages).

You can surround the "invalid" code path with ERR_PRINT_OFF; and ERR_PRINT_ON;. This disables error reporting but still runs the code. (Try to put those macros as close as possible to the invalid code you need to run.)

@maxmutant
Copy link
Contributor Author

Oh, ok. That's good to know, thanks!
I will add further test cases then.

@maxmutant
Copy link
Contributor Author

I have added more tests that should cover the invalid use of HashingContext.

@maxmutant
Copy link
Contributor Author

Made an update to current master to solve conflicts.

Copy link
Contributor

@Xrayez Xrayez left a comment

Choose a reason for hiding this comment

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

Cannot comment on the topic, but tests look good to me!

Needs another rebase...

@maxmutant maxmutant requested a review from a team as a code owner February 27, 2021 11:26
@Xrayez
Copy link
Contributor

Xrayez commented Feb 27, 2021

Need to bump year to 2021 in headers: https://github.com/godotengine/godot/pull/43459/checks?check_run_id=1993702277.

@maxmutant
Copy link
Contributor Author

Yeah just recognized it, thx!

Should be fine now.

Copy link
Collaborator

@Faless Faless left a comment

Choose a reason for hiding this comment

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

This looks good 🏅 .

It would be nice to also add comparison to the actual known hashes.
I see the String hash functions are tested separately in tests/test_string.h so it feels like testing the same thing 2 times, but this specific test feels a bit incomplete without it.

That said, I don't have a strong opinion about it, so I'm going to approve anyway.

@maxmutant
Copy link
Contributor Author

I'm with you.
I'll update the tests so they aren't repetitive with the one's of the String class.

@maxmutant
Copy link
Contributor Author

maxmutant commented Mar 7, 2021

The actual hashes are now used for comparison instead of the String hash functions.
So the tests should be independent from the one's of the String class.

Hope this makes more sense now :)

Copy link
Collaborator

@Faless Faless left a comment

Choose a reason for hiding this comment

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

Great work, thanks!

@Faless Faless merged commit aafbeb2 into godotengine:master Mar 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants