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

test: some improvements #1754

Merged

Conversation

TimoGlastra
Copy link
Contributor

@TimoGlastra TimoGlastra commented Feb 12, 2024

We're having a lot of issues with tests randomly failing. I think the issue is in the in memory wallet being faster (espeically in closing etc..) and therefore the closing / shutdown after tests is now going wrong when we use message pickup.

This PR adds some small improvements, but I think the proper way to fix this is by keeping track of which pickup messages we have sent, and waiting for a response to the sent messages after stopMessagePickup has been called. That way we know for sure no more messages will be exchanged before we call shutdown on the agents.

Partially fixes #1750 It moves more to in-memory. splits e2e from non-e2e, splits non-e2e into shards (2 at the moment) to speed up tests. And I made a first pass at using pre-generated anoncreds definitions in tests. The tests i did this for were significantly faster

Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
.
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
@TimoGlastra TimoGlastra requested review from genaris, berendsliedrecht and karimStekelenburg and removed request for genaris February 13, 2024 08:59
Signed-off-by: Timo Glastra <timo@animo.id>
Copy link
Contributor

@genaris genaris left a comment

Choose a reason for hiding this comment

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

Great additions here! This pre-creation of AnonCreds objects will significantly reduce the risks of tests failing due to timeout errors (apart from making them faster, as you've already benchmarked).

I've left some minor comments, nothing really significant besides a doubt I have regarding the closing of WsOutboundTransport that may introduce issues if works as I remember it did in nodejs environment (probably it has changed since the time I checked).

jest.config.base.ts Outdated Show resolved Hide resolved
packages/anoncreds/tests/preCreatedAnonCredsDefinition.ts Outdated Show resolved Hide resolved
packages/core/src/modules/oob/__tests__/implicit.test.ts Outdated Show resolved Hide resolved
packages/core/src/transport/WsOutboundTransport.ts Outdated Show resolved Hide resolved
@@ -109,5 +129,33 @@ export async function e2eTest({

// We want to stop the mediator polling before the agent is shutdown.
await recipientAgent.mediationRecipient.stopMessagePickup()
await sleep(2000)

const pickupRequestMessages = [V2DeliveryRequestMessage.type.messageTypeUri, V1BatchPickupMessage.type.messageTypeUri]
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not completely sure, but maybe the new "MessagePickupCompleted" event can help on this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so yes, but we would still need to hook into the sent event to catch the thread id that was used for the last sent message.

I think we should probably also hook the initateMessagePickup into this and the subscription from initiateMessagePickup so stopMessagePickup will return when the message pickup is fully complete. If a pickup message has just been sent, it should wait for the delivery message before resolving. I think if we can do that in a follow up, It'll allow us to clean up quite some test logic related to shutting down at the correct time

@TimoGlastra
Copy link
Contributor Author

Merging for now. Thanks for the feedback! We can improve the pickup finished handling in a follow up PR (if we add the right handles in the stopMessagePickup)

@TimoGlastra TimoGlastra enabled auto-merge (squash) February 14, 2024 05:01
@TimoGlastra TimoGlastra modified the milestones: 0.5, v0.3.0 Feb 14, 2024
@TimoGlastra TimoGlastra added this to the 0.5 milestone Feb 14, 2024
@TimoGlastra TimoGlastra merged commit 793527c into openwallet-foundation:main Feb 14, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Improvements to tests in CI
2 participants