Skip to content

Commit

Permalink
fix(errors): fix remaining failing test cases
Browse files Browse the repository at this point in the history
Though running `cargo nextest run -p core-crypto --no-fail-fast` always
produces approx 10 failing tests, going through that list is not profitable.
The failing cases fall into two buckets:

- `mls::conversation::leaf_node_validation::tests::stages::should_validate_leaf_node_when_adding`,
  which also appears to fail on `main`
- the rest of the errors. This set varies each time I run a test job,
  and each case I've tried passes when run in isolation. So I have to
  assume that there is some kind of concurrency problem in the test suite,
  which is outside the scope of this PR.

As such I am deciding to move on to broader testing instead of continuing
to debug these test cases.
  • Loading branch information
coriolinus committed Dec 9, 2024
1 parent e62037a commit a14bad0
Show file tree
Hide file tree
Showing 4 changed files with 36 additions and 27 deletions.
13 changes: 3 additions & 10 deletions crypto/src/mls/conversation/leaf_node_validation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ mod tests {

use crate::{mls, test_utils::*, MlsError, MlsErrorKind};

use openmls::prelude::{AddMembersError, KeyPackageVerifyError, ProposeAddMemberError};
use openmls::prelude::{AddMembersError, KeyPackageVerifyError};

wasm_bindgen_test_configure!(run_in_browser);

Expand Down Expand Up @@ -46,16 +46,9 @@ mod tests {
}

let proposal_creation = alice_central.context.new_add_proposal(&id, invalid_kp).await;
assert!(matches!(
assert!(innermost_source_matches!(
proposal_creation.unwrap_err(),
mls::Error::Mls(MlsError {
source: MlsErrorKind::ProposeAddMemberError(
ProposeAddMemberError::KeyPackageVerifyError(
KeyPackageVerifyError::InvalidLeafNode(_)
)
),
..
})
KeyPackageVerifyError::InvalidLeafNode(_)
));
assert!(alice_central.pending_proposals(&id).await.is_empty());

Expand Down
2 changes: 1 addition & 1 deletion crypto/src/mls/credential/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ pub enum Error {
InvalidCertificateChain,
#[error("decoding X509 certificate")]
DecodeX509(#[source] x509_cert::der::Error),
#[error("Client presented an invalid identity")]
#[error("client presented an invalid identity")]
InvalidIdentity,
/// Unsupported credential type.
///
Expand Down
46 changes: 31 additions & 15 deletions crypto/src/mls/credential/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -230,12 +230,30 @@ mod tests {
let alice_identifier = ClientIdentifier::X509(HashMap::from([(case.signature_scheme(), certs)]));

let (bob_identifier, _) = x509_test_chain.issue_simple_certificate_bundle("bob", None);
assert!(matches!(
try_talk(&case, Some(&x509_test_chain), alice_identifier, bob_identifier)
.await
.unwrap_err(),
Error::InvalidIdentity
));
let err = try_talk(&case, Some(&x509_test_chain), alice_identifier, bob_identifier)
.await
.unwrap_err();
// assert!(innermost_source_matches!(err, Error::InvalidIdentity));
//
// The above should work, in this case. But it fails
// for mysterious reasons that I do not understand.
// Digging into it, the `downcast_ref` internal to the `innermost_source_matches`
// macro produces `None`, so Rust thinks that it's a different error internally.
// But changing the debug/display implementations on `Error::InvalidIdentity`
// also changes the implementations on the inner type; I'm reasonably confident
// that it is in fact this enum and variaint.
//
// So let's abuse the debug implementation, on the assumption
// that nobody is going to make a different unrelated error
// which just happens to share the same debug name.
assert!({
let mut err: &dyn std::error::Error = &err;
while let Some(inner) = err.source() {
err = inner;
}

format!("{err:?}") == "InvalidIdentity"
})
}

#[apply(all_cred_cipher)]
Expand Down Expand Up @@ -468,14 +486,13 @@ mod tests {

let (bob_identifier, _) = x509_test_chain.issue_simple_certificate_bundle("bob", None);

assert!(matches!(
try_talk(&case, Some(&x509_test_chain), alice_identifier, bob_identifier)
.await
.unwrap_err(),
Error::Mls(MlsError {
source: MlsErrorKind::MlsCryptoError(openmls::prelude::CryptoError::ExpiredCertificate),
..
})
let err = try_talk(&case, Some(&x509_test_chain), alice_identifier, bob_identifier)
.await
.unwrap_err();

assert!(innermost_source_matches!(
err,
MlsErrorKind::MlsCryptoError(openmls::prelude::CryptoError::ExpiredCertificate),
))
}
}
Expand All @@ -485,7 +502,6 @@ mod tests {
// let now_since_epoch = now_std().as_secs() as i64;
// wire_e2e_identity::prelude::OffsetDateTime::from_unix_timestamp(now_since_epoch).unwrap()
// }

pub(crate) fn now_std() -> std::time::Duration {
let now = fluvio_wasm_timer::SystemTime::now();
now.duration_since(fluvio_wasm_timer::UNIX_EPOCH).unwrap()
Expand Down
2 changes: 1 addition & 1 deletion crypto/src/test_utils/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ macro_rules! innermost_source_matches {

let outcome = matches!(err.downcast_ref(), Some($pattern) $($guard)?);
if !outcome {
dbg!(err);
eprintln!("{err:?}: {err}");
}

outcome
Expand Down

0 comments on commit a14bad0

Please sign in to comment.