Benchmark results

Instruction counts

Significant differences

There are no significant instruction count differences

Other differences

Wall-time

Significant differences

There are no significant wall-time differences

Other differences

Additional information

Historical results

Checkout details:

• Base repo: https://github.com/rustls/rustls.git
• Base branch: main (73339c2)
• Candidate repo: https://github.com/rustls/rustls.git
• Candidate branch: jbp-extensions-testing (382cbe1)

Codecov Report

Attention: Patch coverage is 89.47368% with 2 lines in your changes missing coverage. Please review.

Project coverage is 95.25%. Comparing base (97a63bf) to head (382cbe1).
Report is 4 commits behind head on main.

@@            Coverage Diff             @@
##             main    #2443      +/-   ##
==========================================
- Coverage   95.26%   95.25%   -0.02%     
==========================================
  Files          95       95              
  Lines       21256    21256              
==========================================
- Hits        20250    20247       -3     
- Misses       1006     1009       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

I've been playing with this because I find the proposed interface pretty hard to grok. I wonder whether it could be built around something more like this:

trait ExtensionEncoder {
    fn encode<'a>(&self, ext: &impl Extension<'a>) -> Result<bool, ()>;
}

trait Extension<'a>: Codec<'a> {
    fn ext_type(&self) -> ExtensionType;
}

@djc ready for another look -- this no longer adds any code to the core crate.

I'm not sure I'm a big fan.

I think the main issue here is we are testing several different things, but testing them in the same way. The original purpose of tests/api.rs is to (as the name hints) exercise the public API in realistic, representative and complete ways, so we can be sure that the public surface is sufficient to do all the things we want to. Naturally, if we move those tests into the crate, we lose that.

The other property of those tests is they don't become invalid from testing rustls against itself. In other words, if we screw up the encoding and decoding of a ClientHello in a consistent way, these tests will pass even though we no longer interop with other implementations. That is fine, but is undesirable for protocol-level tests and for those we want an implementation that is independent (bogo, openssl, or a some straightforward encoding/decoding written in a different style that this PR introduced).

As time has gone on we've added to tests/api.rs things like protocol error testing, to give coverage of the cases that bogo doesn't cover. Those things don't need to be tested outside the crate. Perhaps putting them inside the crate might allow them to become smaller tests, but if not we'd still need to (say) create a ClientConnection with the boilerplate that entails. Maybe we could find a way of reusing tests/common/mod.rs both inside and outside the crate, maybe with a #[path] or something.

Lastly, the other constraint is the desire to have tightly defined and misuse-resistant internal APIs, that those by design make it more difficult to construct erroneous messages for testing. As an example, ffac73a makes it impossible to represent the clienthello issued by

tldr: onboard with moving the protocol tests into the crate, but i still think some of the encoding cases need to diverge from the "real" code

Maybe we could find a way of reusing tests/common/mod.rs both inside and outside the crate, maybe with a #[path] or something.

This seems to be extremely difficult:

• a crate can't refer to itself via its name, so code that works outside the crate does not work inside it, and vv.
• a testing-only crate that contains common/mod.rs is basically unusable inside the crate, because all the types are from a separate compilation of the same code (and so none of the types match).