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

Marshall core unit tests #44797

Merged
merged 2 commits into from
Jan 24, 2021

Conversation

a-ivanov
Copy link
Contributor

@a-ivanov a-ivanov commented Dec 29, 2020

First-time contributor here!

Added unit tests for core functions from marshalls.h as discussed in #43440.
Haven't created ones for encode/decode Variant functions yet. In process of creating tests for different cases of Variant types.

Thanks.

@a-ivanov
Copy link
Contributor Author

a-ivanov commented Dec 30, 2020

Ah, has just noticed the requirement about intermediate commits.
Do I need to squash current commits into one?

Also, if I plan to add Variant related tests, should I squash it too? Or should I create a new PR with Variant tests in separate commit?

@akien-mga
Copy link
Member

Ah, has just noticed the requirement about intermediate commits.
Do I need to squash current commits into one?

Indeed, that would be good. See PR workflow for instructions.

Also, if I plan to add Variant related tests, should I squash it too? Or should I create a new PR with Variant tests in separate commit?

If it's tests which should be part of the Marshall unit tests, they should likely be in this PR too. Could be a separate commit, or squash into the original one, depending on how "separate" they are in scope and implementation.

@a-ivanov a-ivanov changed the title Marshall core unit tests [WIP] Marshall core unit tests Dec 30, 2020
@a-ivanov a-ivanov marked this pull request as draft January 3, 2021 22:47
@a-ivanov a-ivanov force-pushed the marshall-core-unit-tests branch 2 times, most recently from 1a39333 to 7130760 Compare January 10, 2021 09:24
@a-ivanov a-ivanov marked this pull request as ready for review January 10, 2021 10:02
@a-ivanov a-ivanov changed the title [WIP] Marshall core unit tests Marshall core unit tests Jan 10, 2021
@akien-mga akien-mga requested a review from Faless January 11, 2021 08:37
uint8_t arr[] = { 0x55, 0x55, 0x55, 0x55, 0x55, 0x55, 0xd5, 0x3f };

// See floating point encoding test case for details behind expected values
CHECK(Math::is_equal_approx(decode_double(arr), 0.333333333333333314829616256247390992939472198486328125));
Copy link
Member

@aaronfranke aaronfranke Jan 11, 2021

Choose a reason for hiding this comment

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

Since we are testing the validity of an encoded value, this should be exact equality, not approximate equality.

Also, you don't need to use the exact decimal value for the literal. Any decimal literal will automatically become the closest possible double, so we just need to put enough threes (17 digits is the safe amount) for it to become 0.333333333333333314829616256247390992939472198486328125.

Suggested change
CHECK(Math::is_equal_approx(decode_double(arr), 0.333333333333333314829616256247390992939472198486328125));
CHECK(decode_double(arr) == 0.33333333333333333));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, done.

@a-ivanov a-ivanov force-pushed the marshall-core-unit-tests branch from 7130760 to d3034da Compare January 11, 2021 19:20
@a-ivanov a-ivanov requested a review from aaronfranke January 11, 2021 19:22
@a-ivanov a-ivanov force-pushed the marshall-core-unit-tests branch from 15e36d8 to d588623 Compare January 11, 2021 22:17
@a-ivanov a-ivanov requested a review from aaronfranke January 11, 2021 22:18
Copy link
Member

@aaronfranke aaronfranke left a comment

Choose a reason for hiding this comment

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

Looks good to me. Someone else should also review this though.

@akien-mga akien-mga requested a review from a team January 15, 2021 15:10
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.

Looks good enough to me!

@a-ivanov a-ivanov force-pushed the marshall-core-unit-tests branch from 9681662 to 69b554e Compare January 24, 2021 14:15
@akien-mga akien-mga merged commit 6ddfc8e into godotengine:master Jan 24, 2021
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@a-ivanov
Copy link
Contributor Author

Actually it was an interesting dig to become more familiar with the engine.

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.

5 participants