bettertls: test both pathbuilding and nameconstraints. #151
Conversation
Codecov Report
@@ Coverage Diff @@
## main #151 +/- ##
==========================================
+ Coverage 96.21% 96.46% +0.25%
==========================================
Files 15 15
Lines 4304 4304
==========================================
+ Hits 4141 4152 +11
+ Misses 163 152 -11 see 3 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Great to have better test coverage of the name constraint validation!
Honestly not sure about the compression. The smaller git clone size comes at the expense of making it harder to inspect the test contents, and in my mind git clone size isn't a priority. (Subsequent pulls shouldn't be affected much.)
That's fair, I don't feel very strongly but I also don't find the raw JSON very helpful for inspection. I suppose it shows the expected pass/fail but otherwise it seems opaque in decompressed form as well since most of the content is base64 certificates. |
There was a problem hiding this comment.
Pleased to know that this didn't find any latent bugs in name constraints!
It's a bit annoying that the default rust test runner is so low on features, such as runtime test case generation and fixtures. Ideally all these test cases would have names so someone could cargo test them individually. Oh well!
Also fair! 🤷 |
|
@ctz Do you have an opinion RE: compressing the data file? Should I back that out? |
I do miss the Go test runner's nice support for table driven tests w/ named subtests |
You can do more with a custom test harness, like here: https://github.com/servo/rust-url/blob/master/url/tests/wpt.rs. I think there are also crates that help with this? Some folks on the Cargo team are investigating making this better, see https://epage.github.io/blog/2023/06/iterating-on-test/. |
Previously the bettertls test data and runner only included the 'pathbuilding' suite. This commit brings in the remaining 'nameconstraints' tests. It turns out very little needs to change to also run these tests. In addition to trying path building with `verify_for_usage`, we also need to perform name verification for the subject indicated in the testcase 'hostname' field with `verify_is_valid_for_subject_name`. That's about it, the rest is plumbing. :) One additional point of interest: with the full tests included the vendored JSON was about ~33mb. The `Cargo.toml` config doesn't include the `third-party` directory in the packaged crate, but it still felt too large. As a workaround this commit applies bzip2 compression, bringing it down to ~7mb, and updates the test runner to decompresses it on the fly.
cpu
force-pushed
the
cpu-bettertls-even-better
branch
from
e94f0d5
to
7294be7
Compare
I'm in favour of compressing it, but for a slightly odd reason -- I use |
Previously (#116) the bettertls test data and runner only included the 'pathbuilding' suite. This commit brings in the remaining 'nameconstraints' tests.
It turns out very little needs to change to also run these tests. In addition to trying path building with
verify_for_usage, we also need to perform name verification for the subject indicated in the testcase 'hostname' field withverify_is_valid_for_subject_name. That's about it, the rest is plumbing. :)One additional point of interest: with the full tests included the vendored JSON was about ~33mb. The
Cargo.tomlconfig doesn't include thethird-partydirectory in the packaged crate, but it still felt too large. As a workaround this commit appliesbzip2compression, bringing it down to ~7mb, and updates the test runner to decompresses it on the fly.