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 test coverage for der.rs #223

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

st3fan
Copy link

@st3fan st3fan commented Apr 22, 2021

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
Copy link

codecov bot commented Apr 22, 2021

Codecov Report

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

❗ 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%) ⬆️
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%) ⬆️
src/calendar.rs 90.90% <0.00%> (+1.89%) ⬆️
tests/dns_name_tests.rs 92.85% <0.00%> (+15.07%) ⬆️

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
Copy link
Owner

@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
Copy link
Author

st3fan commented Apr 22, 2021

  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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants