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

tests: transformed tests/sha256 into a unittest #2181

Merged
merged 1 commit into from
Dec 12, 2014

Conversation

phiros
Copy link
Member

@phiros phiros commented Dec 11, 2014

Partial fix for issue #1131.

tests/sha256 -> unittest/tests-crypto/tests-crypto-sha256.c

@phiros phiros added Area: tests Area: tests and testing framework Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation labels Dec 11, 2014
"Frank jagt im komplett verwahrlosten Taxi quer durch Bayern",
"78206a866dbb2bf017d8e34274aed01a8ce405b69d45db30bafa00f5eeed7d5e"));
TEST_ASSERT(calc_and_compare_hash("",
"e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"));
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to devide those up into several tests?

Also: there is the macro TEST_ASSERT_EQUAL_STRING. With it the output in case of error looks a little bit more helpful.

Copy link
Member Author

Choose a reason for hiding this comment

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

Whenever I compare strings in C I prefer to use strncmp. Maybe thats just nit-picking but I think it
makes sense (especially in the context of a test). The TEST_ASSERT_EQUAL_STRING macro is essentially a strcmp.

@miri64
Copy link
Member

miri64 commented Dec 12, 2014

I know this is just a tranformation of tests/tests-crypto/, but I would the unittests to be less bulky and testing one API function at a time. BTW: What's with corner cases?

@LudwigKnuepfer LudwigKnuepfer changed the title tests: transformed tests/sha256 into an unittest tests: transformed tests/sha256 into a unittest Dec 12, 2014
@phiros
Copy link
Member Author

phiros commented Dec 12, 2014

For now I'd really prefer to get this PR merged so that I can ask the authors of this code the crypto module ( @mehlis and @Kijewski ) to improve this unit test (they know the implementation far better than I do; it would be easier for them to identify those corner cases).
Also, this PR would ensure that the test is at least executed. Currently, this is not the case.

BTW: As previously mentioned, the overall test coverage of crypto is quite poor (all 5 cipher implementations are untested). I think it makes sense to open a separate issue for this.

@LudwigKnuepfer
Copy link
Member

In general a fresh mind might be better at finding corner cases..

@miri64
Copy link
Member

miri64 commented Dec 12, 2014

I'm with @LudwigOrtmann. It's better if the test author is someone else than the author of the tested module, but since our manpower does not really allow for that (yet), I see where you coming from. Plus, this is not topical to this PR. Let's merge it: ACK (tested for native, samr21-xpro and msba2)

miri64 added a commit that referenced this pull request Dec 12, 2014
…test

tests: transformed tests/sha256 into a unittest
@miri64 miri64 merged commit 17ef92e into RIOT-OS:master Dec 12, 2014
@phiros phiros deleted the tests_sha256_transform_into_unittest branch December 12, 2014 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: tests Area: tests and testing framework Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants