jedisct1 / libsodium

A modern, portable, easy to use crypto library.
https://libsodium.org
Other
12.06k stars 1.72k forks source link

Add testcases and expected output files #1274

Closed arindam-8 closed 10 months ago

arindam-8 commented 1 year ago

This is the PR following the discussion on the libsodium repository's discussion board. It adds 3 testcases extracted from 2 different libsodium clients: dovecot and blobcrypt.

jedisct1 commented 1 year ago

Hi @arindam-8 !

And thanks for these tests!

But I'm a little bit confused. They don't seem to test anything that is not already present in existing tests.

They just evaluate crypto_pwhash and crypto_generichash with fixed parameters, but the pwhash_argon2* and generichash* tests already do it, along with other parameters.

A more interesting thing to do would be to run the tests, look at code coverage, and try to write tests that exercise code that was not previously covered.

pengwinsurf commented 1 year ago

Hi @jedisct1,

I am working with @arindam-8 on generating those test cases for libsodium from clients using the library.

Thank you for the quick response on the PR, really appreciate it. Also thank you for the feedback.

I did some coverage measurement on for those test cases and I can confirm I see an increase in coverage. The way I did the testing is I built the latest stable release with coverage and ran make check to get the baseline coverage. Following that I ran the test cases and collected coverage using lcov.

The baseline I got running make check was

Lines: 12669/14466 (87.6%) , Functions: 986/1067 (92.4%) , Branches: 3220/5155 (62.5%) 

After running the test cases the coverage shows the following

Lines: 12781/14466 (88.4%), Functions: 992/1067 (93%), Branches: 3267/5155 (63.4%)

The additional coverage is in 3 files argon2-fill-block-ref.c , blamka-round-ref.h and blake2b-compress-ref.c

Thats 88 lines in argon2-fill-block-ref.c, 4 lines in blamka-round-ref.h and 20 lines in blake2b-compress-ref.c.

Hope this information helps.

One last thing, I wanted to ask is regarding the CI tests. It seems that at the moment, running the CI requires some authorisation to run all of the tests? Is there an easier way to run all the CI tests locally for a faster turn around ?

Thanks!

arindam-8 commented 1 year ago

Hey,

Is there anything else that would help clarify this PR further?

Thanks Arindam

pengwinsurf commented 1 year ago

Hi @jedisct1,

Is there any chance you could comment on this PR given the information regarding coverage etc ?

jedisct1 commented 10 months ago

These tests don't test anything that is not already exercised by the current test suite.