briansmith / webpki

WebPKI X.509 Certificate Validation in Rust
https://briansmith.org/rustdoc/webpki/
Other
459 stars 164 forks source link

Add test coverage for der.rs #223

Open st3fan opened 3 years ago

st3fan commented 3 years ago

This patch adds some basic tests around optional_boolean() to validate that it correctly generates the expected results.

@briansmith let me know if you are interested in test coverage like this. I'm happy to expand this and try to cover a large chunk of functionality in src/der.rs with tests like this. I enjoy writing tests and this is a great way for me to learn some Rust skills. If it is too soon for tests on this code, or not needed, let me know or point me at something else. Happy to contribute.

codecov[bot] commented 3 years ago

Codecov Report

Merging #223 (706f20f) into main (2208a22) will increase coverage by 2.22%. The diff coverage is 100.00%.

:exclamation: Current head 706f20f differs from pull request most recent head e4d2683. Consider uploading reports for the commit e4d2683 to get more accurate results Impacted file tree graph

@@            Coverage Diff             @@
##             main     #223      +/-   ##
==========================================
+ Coverage   50.60%   52.82%   +2.22%     
==========================================
  Files          17       17              
  Lines        1984     2069      +85     
==========================================
+ Hits         1004     1093      +89     
+ Misses        980      976       -4     
Impacted Files Coverage Δ
src/der.rs 93.87% <100.00%> (+3.69%) :arrow_up:
src/signed_data.rs 100.00% <0.00%> (ø)
tests/integration.rs 100.00% <0.00%> (ø)
tests/cert_v1_unsupported.rs 100.00% <0.00%> (ø)
tests/cert_without_extensions.rs 100.00% <0.00%> (ø)
src/name/dns_name.rs 62.45% <0.00%> (+1.05%) :arrow_up:
src/calendar.rs 90.90% <0.00%> (+1.89%) :arrow_up:
tests/dns_name_tests.rs 92.85% <0.00%> (+15.07%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 2208a22...e4d2683. Read the comment docs.

briansmith commented 3 years ago

@stefan I love the idea of this.

Two thoughts:

  1. It would make more sense to start with the fundamental ASN.1 parsing stuff in ring::io::der, particularly the very basic building blocks with unit tests (in ring::io::der), and then eventually integration tests of the (hidden) API that ring exposes to webpki (in ring's tests/der_tests.rs). Would you be able to start there?
  2. My plan is to supplement the current testing with fuzzing. So, I'd like to see tests (parser tests in particular) split in such a way that the "core" of each test could in theory be used with fuzzer-selected inputs. Have you done things like this before?
st3fan commented 3 years ago
  1. It would make more sense to start with the fundamental ASN.1 parsing stuff in ring::io::der, particularly the very basic building blocks with unit tests (in ring::io::der), and then eventually integration tests of the (hidden) API that ring exposes to webpki (in ring's tests/der_tests.rs). Would you be able to start there?

Yeah I'm happy to move to Ring and see what I can contribute there. It looks like the test_bit_string_with_no_unused_bits() that I just pushed could also go to Ring.

  1. My plan is to supplement the current testing with fuzzing. So, I'd like to see tests (parser tests in particular) split in such a way that the "core" of each test could in theory be used with fuzzer-selected inputs. Have you done things like this before?

I have not done much fuzzing work but I am eager to learn some of that. What you describe sounds good - do you have an idea what you want that to look like or shall I create a starting point?