Added unit tests for key loading, signing, and verification functions, fixed Manifest Type Detection Logic #4
Conversation
…, fixed manifest determ fn
There was a problem hiding this comment.
Thanks for adding all these tests @sandlbn . They pass on my end, too! I have a couple of comments/suggestions.
src/manifest/utils.rs
Outdated
| // Default case if nothing else matches | ||
| ManifestType::Model |
There was a problem hiding this comment.
Should we add a ManifestType::Unknown for cases where a type can't be easily determined? This way, we could directly check for this type when verification happens and error early.
There was a problem hiding this comment.
Updated to Unknown.
src/signing/mod.rs
Outdated
| } | ||
|
|
||
| #[test] | ||
| fn test_key_file_permissions() -> Result<()> { |
There was a problem hiding this comment.
Since it's a test key in temp, do we need this test? Instead, we might just want to delete the key explicitly at the end of each test. Plus key loading is already tested above.
There was a problem hiding this comment.
Good point about the redundancy! Though I think this test has a slightly different purpose - it's checking that we can load keys with restrictive permissions (0x600), which is pretty common for private keys. The temp cleanup happens automatically when TempDir is dropped, so we're good there. Maybe we could just rename it to something clearer like test_load_key_with_restrictive_permissions() to make it obvious why it exists?
There was a problem hiding this comment.
ah, thanks for clarifying that. Makes sense to me!
2 participants
When writing tests I figure out that determine_manifest_type function wasn't correctly handling cases where conflicting information was present in assertions and ingredients. Tests were failing because the function was not following the expected priority when determining manifest types. Now checks for both assertions and ingredients for each type in priority order: Dataset > Software > Model