Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Improve ReceiveMessagesTest with log asserts #230

Merged
merged 2 commits into from
May 18, 2022

Conversation

sdsantos
Copy link
Collaborator

@sdsantos sdsantos commented May 9, 2022

Fixes #49

@gnarea Notice the failing test. Feels weird that deleting the ThirdPartyEndpoint is not enough, since it triggers another error. Are we forgetting to delete something inside the delete() method?

@sdsantos sdsantos changed the title Improve ReceiveMessagesTest with log asserts fix: Improve ReceiveMessagesTest with log asserts May 9, 2022
Comment on lines +147 to +152
val collectParcelsCall = CollectParcelsCall(Result.success(flowOf(parcelCollection)))
pdcClient = MockPDCClient(collectParcelsCall)

channel.firstPartyEndpoint.delete()

val messages = subject.receive().toCollection(mutableListOf())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you noticed, this is failing with the error: MissingKeyException: There is no session key for 03d6f89f597e881f275831faaa7abb92553f18fc563e53494372db18461533178.

That's because this test currently simulates a situation where the 1st party endpoint exists when we start collecting parcels, but no longer exists by the time we get the parcel. However, I think what you meant to simulate is the scenario where the 1st party endpoint didn't even exist when parcel collection started -- that is, parcel.recipientAddress != channel.firstPartyEndpoint.privateAddress is what you want to replicate.

I think the former is worth testing to make sure it's handled (I don't think I considered this scenario but it's great we came across it as it could certainly happen in real life!)

I'm not sure we can test the latter, but I could be wrong: If the 1st party endpoint didn't exist when parcel collection started (because it never existed or it was deleted), then that should/will throw an InvalidMessageException in:

val parcel = try {
parcelCollection.deserializeAndValidateParcel()
} catch (exp: RAMFException) {
parcelCollection.disregard("Malformed incoming parcel", exp)
return@mapNotNull null
} catch (exp: InvalidMessageException) {
parcelCollection.disregard("Invalid incoming parcel", exp)
return@mapNotNull null
}

... because the sender's PDA wouldn't have been issued by any id certificate of our 1st party endpoints.

Thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By "I think the former is worth testing to make sure it's handled" I mean add a catch for MissingKeyException

@sdsantos sdsantos added the automerge Allow kodiak to automerge commit when all checks pass label May 18, 2022
@kodiakhq kodiakhq bot merged commit 583e796 into main May 18, 2022
@kodiakhq kodiakhq bot deleted the improve-receive-messages-test branch May 18, 2022 17:26
@github-actions
Copy link

🎉 This PR is included in version 1.12.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Allow kodiak to automerge commit when all checks pass released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tests for incoming messages don't check that failure is due to the right reason
2 participants