Skip to content

virt: allow cleanup to run on the platform after the test - #241

Open
sosthene-nitrokey wants to merge 1 commit into
trussed-dev:mainfrom
sosthene-nitrokey:virt-cleanup
Open

sosthene-nitrokey wants to merge 1 commit into
trussed-dev:mainfrom
sosthene-nitrokey:virt-cleanup

Conversation

@sosthene-nitrokey

@sosthene-nitrokey sosthene-nitrokey commented Nov 9, 2026 •

Copy link
Copy Markdown
Contributor

This is useful in tests to make sure that there is not memory/storage leak for example.

@robin-nitrokey robin-nitrokey left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What do you think about storing the callback in a Box and adding it as a field to the runner instead of adding all the *_with_cleanup variations?

@sosthene-nitrokey

sosthene-nitrokey commented Nov 10, 2026 •

Copy link
Copy Markdown
Contributor Author

We could make it part of the runner, but most downstreams don't use it directly and they use the Platform instead through with_platform, then run_client*.

For exemple the piv-authenticator test suite never sees a Runner value.
So the duplication in the run_client* would still be needed.

@robin-nitrokey

Copy link
Copy Markdown
Member

Couldn’t we make it a field for both Platform and Runner to solve this issue? So instead of:

platform.run_client_with_backends_and_cleanup(..., cleanup)

we could have:

platform.cleanup(cleanup).run_client_with_backends(...)

This would also cause some duplication, but I think it’s much easier to manage than adding another variant to the run_with_* functions.

@robin-nitrokey robin-nitrokey left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That’s even better, thanks.

Comment thread src/service.rs Resolved

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants