test: Wave 14 - comprehensive adapter and facade tests#40
test: Wave 14 - comprehensive adapter and facade tests#40EffortlessSteven merged 4 commits intomainfrom
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Review Summary by QodoAdd 79 comprehensive adapter and facade integration tests
WalkthroughsDescription• Adds 79 comprehensive integration tests across rustcrypto and rustls adapters • Covers sign-verify round trips, deterministic key reproduction, and debug safety • Includes facade e2e tests verifying order-independent derivation and label uniqueness • Adds property-based tests validating determinism under random seeds Diagramflowchart LR
A["Test Suite"] --> B["rustcrypto_comprehensive<br/>31 tests"]
A --> C["rustls_comprehensive<br/>20 tests"]
A --> D["facade_e2e<br/>18 tests"]
A --> E["prop_tests<br/>10 tests"]
B --> B1["RSA/ECDSA/Ed25519<br/>sign-verify"]
B --> B2["Deterministic<br/>reproduction"]
B --> B3["Debug safety<br/>no key leaks"]
C --> C1["Certificate/key<br/>DER conversion"]
C --> C2["TLS handshakes<br/>mTLS support"]
C --> C3["Deterministic<br/>chains"]
D --> D1["Order-independent<br/>derivation"]
D --> D2["Label uniqueness<br/>verification"]
E --> E1["Property-based<br/>determinism"]
E --> E2["PEM format<br/>invariants"]
File Changes1. crates/uselesskey-rustcrypto/tests/rustcrypto_comprehensive.rs
|
Code Review by Qodo
1. PEM blob in proptests
|
There was a problem hiding this comment.
Pull request overview
Adds a new “Wave 14” test suite expanding coverage for the uselesskey facade and adapter crates, focusing on deterministic behavior and end-to-end correctness across key types.
Changes:
- Add facade property tests (proptest) covering determinism and basic invariants across RSA/ECDSA/Ed25519/HMAC/token.
- Add facade end-to-end integration tests for determinism, ordering, label separation, debug-safety, and negative fixtures.
- Add comprehensive adapter integration tests for
uselesskey-rustcrypto(sign/verify, conversions, debug-safety) anduselesskey-rustls(config conversion + TLS/mTLS handshakes + determinism).
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/uselesskey/tests/prop_tests.rs | New property-based tests for facade determinism and basic formatting/invariant checks. |
| crates/uselesskey/tests/facade_e2e.rs | New end-to-end facade tests validating deterministic derivation and safety expectations. |
| crates/uselesskey/Cargo.toml | Adds proptest as a dev-dependency for new property tests. |
| crates/uselesskey-rustls/tests/rustls_comprehensive.rs | New rustls adapter integration tests including handshake scenarios and determinism checks. |
| crates/uselesskey-rustcrypto/tests/rustcrypto_comprehensive.rs | New rustcrypto adapter integration tests for sign/verify, determinism, debug-safety, and cross-type usage. |
| Cargo.lock | Locks proptest for the uselesskey crate’s new dev-dependency. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let bad_header = corrupt_pem(pem, CorruptPem::BadHeader); | ||
| let bad_footer = corrupt_pem(pem, CorruptPem::BadFooter); | ||
| let bad_base64 = corrupt_pem(pem, CorruptPem::BadBase64); | ||
|
|
||
| // Suppress unused variable warning | ||
| let _ = seed; | ||
|
|
||
| prop_assert_ne!(bad_header, pem); | ||
| prop_assert_ne!(bad_footer, pem); | ||
| prop_assert_ne!(bad_base64, pem); |
There was a problem hiding this comment.
This proptest case parameter (seed) is unused, so the test executes the exact same assertions for every generated input (wasted runtime and misleading “property” semantics). Either convert this to a regular #[test], or use seed to vary the corruption (e.g., choose variant/position) so each case explores different inputs.
| let bad_header = corrupt_pem(pem, CorruptPem::BadHeader); | |
| let bad_footer = corrupt_pem(pem, CorruptPem::BadFooter); | |
| let bad_base64 = corrupt_pem(pem, CorruptPem::BadBase64); | |
| // Suppress unused variable warning | |
| let _ = seed; | |
| prop_assert_ne!(bad_header, pem); | |
| prop_assert_ne!(bad_footer, pem); | |
| prop_assert_ne!(bad_base64, pem); | |
| // Use the seed to choose which corruption variant to apply, | |
| // so each proptest case can explore different inputs. | |
| let choice = seed[0] % 3; | |
| let variant = match choice { | |
| 0 => CorruptPem::BadHeader, | |
| 1 => CorruptPem::BadFooter, | |
| _ => CorruptPem::BadBase64, | |
| }; | |
| let corrupted = corrupt_pem(pem, variant); | |
| prop_assert_ne!(corrupted, pem); |
| let seed = Seed::from_env_value(original).expect("seed"); | ||
| let fx = Factory::deterministic(seed); | ||
| assert!(matches!(fx.mode(), Mode::Deterministic { .. })); |
There was a problem hiding this comment.
seed_round_trip doesn’t actually test a round-trip (it only checks that a deterministic Factory reports Mode::Deterministic, which is already covered by deterministic_factory_mode). Either rename the test to reflect what it asserts, or extend it to validate a real seed property (e.g., that Seed::from_env_value is deterministic / stable for a given input).
| let seed = Seed::from_env_value(original).expect("seed"); | |
| let fx = Factory::deterministic(seed); | |
| assert!(matches!(fx.mode(), Mode::Deterministic { .. })); | |
| // Seed must be stable/deterministic for a given env value | |
| let seed1 = Seed::from_env_value(original).expect("seed1"); | |
| let seed2 = Seed::from_env_value(original).expect("seed2"); | |
| assert_eq!(seed1, seed2, "Seed::from_env_value must be deterministic"); | |
| // Factories constructed from the same seed must be deterministic mode | |
| let fx1 = Factory::deterministic(seed1); | |
| let fx2 = Factory::deterministic(seed2); | |
| assert!(matches!(fx1.mode(), Mode::Deterministic { .. })); | |
| assert!(matches!(fx2.mode(), Mode::Deterministic { .. })); |
| let signing = SigningKey::<Sha256>::new_unprefixed(private); | ||
| let verifying = VerifyingKey::<Sha256>::new_unprefixed(public); | ||
|
|
||
| for msg in [b"hello" as &[u8], b"", b"a".repeat(4096).as_slice()] { |
There was a problem hiding this comment.
for msg in [.., b"a".repeat(4096).as_slice()] takes a slice of a temporary Vec<u8>, which won’t compile (temporary dropped while borrowed). Bind the repeated buffer to a local variable (or use a Vec/array stored outside the loop) and iterate over stable references.
| for msg in [b"hello" as &[u8], b"", b"a".repeat(4096).as_slice()] { | |
| let long_msg_buf = b"a".repeat(4096); | |
| let long_msg: &[u8] = &long_msg_buf; | |
| for msg in [b"hello" as &[u8], b"", long_msg] { |
| let signing = kp.p256_signing_key(); | ||
| let verifying = kp.p256_verifying_key(); | ||
|
|
||
| for msg in [b"hello" as &[u8], b"", b"x".repeat(8192).as_slice()] { |
There was a problem hiding this comment.
This loop uses b"x".repeat(8192).as_slice() inside an array literal, which borrows from a temporary Vec<u8> and will fail to compile. Store the repeated message in a local variable first, then iterate over slices to it.
| for msg in [b"hello" as &[u8], b"", b"x".repeat(8192).as_slice()] { | |
| let long_msg = b"x".repeat(8192); | |
| for msg in [b"hello" as &[u8], b"", long_msg.as_slice()] { |
| let signing = kp.ed25519_signing_key(); | ||
| let verifying = kp.ed25519_verifying_key(); | ||
|
|
||
| for msg in [b"hello" as &[u8], b"", b"y".repeat(10000).as_slice()] { |
There was a problem hiding this comment.
b"y".repeat(10000).as_slice() borrows from a temporary Vec<u8> created inside the array literal; this will not compile due to the temporary being dropped. Create the large message buffer in a variable (e.g., let big = vec![..];) and reference it in the loop.
| for msg in [b"hello" as &[u8], b"", b"y".repeat(10000).as_slice()] { | |
| let big = vec![b'y'; 10000]; | |
| for msg in [b"hello" as &[u8], b"", big.as_slice()] { |
| if !progress { | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
try_handshake returns Ok(()) even if the handshake never completes (e.g., loop exits due to !progress or hitting the iteration cap). This can cause false positives/negatives in both success and failure tests. Consider returning an error (or panicking in the helper) if client.is_handshaking() or server.is_handshaking() is still true after the loop.
| } | |
| } | |
| if client.is_handshaking() || server.is_handshaking() { | |
| return Err(rustls::Error::General( | |
| "try_handshake: TLS handshake did not complete".into(), | |
| )); | |
| } |
| fn install_provider() { | ||
| INIT.call_once(|| { | ||
| let _ = rustls::crypto::ring::default_provider().install_default(); | ||
| }); | ||
| } |
There was a problem hiding this comment.
install_provider() installs a process-global rustls default provider and ignores the result. This is unnecessary here because the tests already build configs with an explicit CryptoProvider, and duplicating global installs across multiple test modules/binaries can create flaky races. Prefer removing the global install and relying solely on the explicit provider, or centralize provider installation in shared testutil so it happens exactly once per test binary.
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| let pem = "-----BEGIN PRIVATE KEY-----\nMIIBVQIBADANBg==\n-----END PRIVATE KEY-----\n"; | ||
|
|
There was a problem hiding this comment.
1. Pem blob in proptests 📘 Rule violation ⛨ Security
The new property tests commit a PEM-looking private key block string literal. This is a secret-shaped blob in a test path and violates the fixture policy.
Agent Prompt
## Issue description
A PEM-looking private key string literal is committed in property tests, which violates the no-secret-shaped-blobs rule.
## Issue Context
These literals can trip secret scanners and are disallowed even if they are not real keys.
## Fix Focus Areas
- crates/uselesskey/tests/prop_tests.rs[189-203]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| let key = chain.private_key_der_rustls(); | ||
| assert_eq!( | ||
| key.secret_der(), | ||
| chain.leaf_private_key_pkcs8_der(), | ||
| "private key DER must match source" | ||
| ); |
There was a problem hiding this comment.
2. assert_eq! prints key der 📘 Rule violation ⛨ Security
The new rustls adapter tests compare private key DER bytes using assert_eq!, which will print key material on failure. This can leak private keys into CI logs when a test fails.
Agent Prompt
## Issue description
Rustls adapter tests use `assert_eq!` to compare private key DER values. On failure, this prints private key bytes into logs.
## Issue Context
Assertion failures are visible in CI output, and the compliance policy forbids leaking key material via logs/errors.
## Fix Focus Areas
- crates/uselesskey-rustls/tests/rustls_comprehensive.rs[94-104]
- crates/uselesskey-rustls/tests/rustls_comprehensive.rs[333-346]
- crates/uselesskey-rustls/tests/rustls_comprehensive.rs[155-160]
- crates/uselesskey-rustls/tests/rustls_comprehensive.rs[177-182]
- crates/uselesskey-rustls/tests/rustls_comprehensive.rs[185-190]
- crates/uselesskey-rustls/tests/rustls_comprehensive.rs[199-204]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
rustcrypto (31 tests): - RSA/ECDSA/Ed25519/HMAC sign-verify round trips - Wrong message verification failures - Deterministic key reproduction - Different labels produce different keys - RSA-4096 key conversion - Cross-curve panic tests (p256/p384) - HMAC SHA-256/384/512 compute and verify - Tag length verification - Debug safety (no key material leakage) - Cross-adapter all-types from same factory - Deterministic all-types stability rustls (20 tests): - Certificate/key DER conversion correctness - Chain structure (leaf + intermediate) - Root certificate matching - RSA/ECDSA/Ed25519 private key conversion - TLS handshake with chain - mTLS handshake - Wrong SNI fails handshake - Cross-CA fails handshake - Deterministic chain and self-signed reproduction - Different seeds produce different chains - Debug safety for chains and self-signed certs Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
facade_e2e.rs (18 tests): - Deterministic reproduction for RSA, ECDSA, Ed25519, HMAC, token - Order-independent derivation (ABC vs CBA generation order) - Different labels produce different keys - Debug safety (no PEM markers leaked) - Negative fixture CorruptPem variants - PEM format well-formedness - Factory mode verification - Seed round-trip prop_tests.rs (10 tests): - Property-based determinism for RSA, ECDSA, Ed25519, HMAC, token - PEM well-formedness under random seeds - HMAC secret length matches spec - Token prefix invariant - Different seeds produce different ECDSA keys - Corrupt PEM never returns original Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
EffortlessSteven
force-pushed
the
wave-14/adapter-tests
branch
from
494e025 to
f423152
Compare
Adds 79 tests across adapter crates and facade: rustcrypto (31), rustls (20), facade e2e (18), prop tests (10). All tests pass locally. No determinism/policy impact.