test: Wave 12 - insta snapshot tests for HMAC and PGP crates#38
Closed
EffortlessSteven wants to merge 4 commits intomainfrom
Closed
test: Wave 12 - insta snapshot tests for HMAC and PGP crates#38EffortlessSteven wants to merge 4 commits intomainfrom
EffortlessSteven wants to merge 4 commits intomainfrom
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
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 (10)
📒 Files selected for processing (4)
✨ 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 insta snapshot tests for HMAC and PGP crates
WalkthroughsDescription• Add insta snapshot tests for HMAC crate (HS256/HS384/HS512 shapes and spec matrix) • Add insta snapshot tests for PGP crate (Ed25519/RSA-2048 shapes and negative fixtures) • Add dev dependencies (insta, serde) to both crate Cargo.toml files Diagramflowchart LR
A["HMAC Crate"] -->|"4 snapshot tests"| B["HS256/HS384/HS512 shapes + spec matrix"]
C["PGP Crate"] -->|"5 snapshot tests"| D["Ed25519/RSA-2048 shapes + negative fixtures"]
E["Dev Dependencies"] -->|"insta, serde"| F["Both crates"]
File Changes1. crates/uselesskey-hmac/tests/snapshots_hmac.rs
|
Code Review by Qodo
1.
|
There was a problem hiding this comment.
Pull request overview
Adds insta YAML snapshot test coverage for the uselesskey-hmac and uselesskey-pgp crates to lock down key/secret “shape” outputs (headers, lengths, etc.) and a few negative-fixture helpers.
Changes:
- Add snapshot test suites for HMAC (HS256/384/512 + spec matrix) and PGP (Ed25519/RSA-2048 + negative fixtures).
- Add corresponding
.snapartifacts under each crate’stests/snapshots/. - Add
insta+serdeas dev-dependencies for the two crates and updateCargo.lock.
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/uselesskey-pgp/tests/snapshots_pgp.rs | New PGP snapshot tests (shapes + negative fixtures) |
| crates/uselesskey-pgp/tests/snapshots/snapshots_pgp__pgp_truncated_binary.snap | New snapshot artifact for truncated binary test |
| crates/uselesskey-pgp/tests/snapshots/snapshots_pgp__pgp_rsa_2048_shape.snap | New snapshot artifact for RSA-2048 shape |
| crates/uselesskey-pgp/tests/snapshots/snapshots_pgp__pgp_negative_mismatch.snap | New snapshot artifact for mismatch negative fixture |
| crates/uselesskey-pgp/tests/snapshots/snapshots_pgp__pgp_negative_corrupt_armored.snap | New snapshot artifact for corrupt-armor negative fixture |
| crates/uselesskey-pgp/tests/snapshots/snapshots_pgp__pgp_ed25519_shape.snap | New snapshot artifact for Ed25519 shape |
| crates/uselesskey-pgp/Cargo.toml | Add insta and serde dev-deps for snapshots |
| crates/uselesskey-hmac/tests/snapshots_hmac.rs | New HMAC snapshot tests (shapes + spec matrix) |
| crates/uselesskey-hmac/tests/snapshots/snapshots_hmac__hmac_hs512_shape.snap | New snapshot artifact for HS512 shape |
| crates/uselesskey-hmac/tests/snapshots/snapshots_hmac__hmac_hs384_shape.snap | New snapshot artifact for HS384 shape |
| crates/uselesskey-hmac/tests/snapshots/snapshots_hmac__hmac_hs256_shape.snap | New snapshot artifact for HS256 shape |
| crates/uselesskey-hmac/tests/snapshots/snapshots_hmac__hmac_all_specs.snap | New snapshot artifact for all-specs matrix |
| crates/uselesskey-hmac/Cargo.toml | Add insta and serde dev-deps for snapshots |
| Cargo.lock | Lockfile updates for new dev-deps |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+69
to
+75
| insta::assert_yaml_snapshot!("pgp_rsa_2048_shape", result, { | ||
| ".fingerprint_len" => insta::dynamic_redaction(|val, _| { | ||
| let n = val.as_u64().unwrap(); | ||
| assert!(n >= 40, "fingerprint too short: {n}"); | ||
| "[FINGERPRINT_LEN_OK]" | ||
| }), | ||
| }); |
There was a problem hiding this comment.
Same issue here: redacting fingerprint_len to a placeholder means the snapshot won’t surface or fail on fingerprint length changes (beyond the >= 40 check). If the goal is regression detection, it’s more effective to snapshot the actual fingerprint_len value (or assert it equals the expected length).
Comment on lines
+44
to
+50
| insta::assert_yaml_snapshot!("pgp_ed25519_shape", result, { | ||
| ".fingerprint_len" => insta::dynamic_redaction(|val, _| { | ||
| let n = val.as_u64().unwrap(); | ||
| assert!(n >= 40, "fingerprint too short: {n}"); | ||
| "[FINGERPRINT_LEN_OK]" | ||
| }), | ||
| }); |
There was a problem hiding this comment.
fingerprint_len is being dynamically redacted to a constant placeholder, which makes this snapshot unable to detect changes in fingerprint length (it only enforces >= 40). Since the fingerprint length isn’t secret, consider snapshotting the actual numeric length (and, if needed, add an explicit assertion for the expected value) so this test can catch unintended changes.
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
EffortlessSteven
force-pushed
the
wave-12/hmac-pgp-snapshots
branch
2 times, most recently
from
3f4a2ef to
194ca17
Compare
HMAC snapshots: HS256/HS384/HS512 shapes and spec matrix. PGP snapshots: Ed25519/RSA-2048 shapes, negative fixtures (corrupt, mismatch, truncated). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Remove unnecessary .clone() on Copy type HmacSpec. Snapshot fingerprint_len directly instead of redacting. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The fuzz target file is named jwks_order.rs but the Cargo.toml bin entry referenced jwk_order.rs. This caused xtask fuzz to fail when auto-discovering targets by filename. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
RSA PGP key binary lengths may differ between Windows and Linux due to platform-specific PGP key serialization. Redact these values to prevent cross-platform snapshot failures during mutation testing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
EffortlessSteven
force-pushed
the
wave-12/hmac-pgp-snapshots
branch
from
65f43b7 to
9887e9b
Compare
Member
Author
|
Closing: Branch is too stale to salvage cheaply. The fuzz/Cargo.toml misses the renamed jwks_order target from main, and most HMAC snapshot coverage is superseded by merged waves 17 and 23. Superseded by wave-53 fuzz expansion (PR #68). [Class E: superseded] |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds insta snapshot tests for HMAC and PGP crates. HMAC: 4 tests (HS256/HS384/HS512 shapes + spec matrix). PGP: 5 tests (Ed25519/RSA-2048 shapes + negative fixtures).