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

Fixes to tests to make cargo clippy succeed #132

Merged
merged 3 commits into from
Jun 9, 2023

Conversation

ipuustin
Copy link
Contributor

@ipuustin ipuustin commented Jun 8, 2023

Add delete() to Wasmtime tests. Also clean up the code a little to make cargo clippy succeed.

@ipuustin ipuustin changed the title wasmtime: fixes to tests. Fixes to tests to make cargo clippy succeed Jun 8, 2023
Copy link
Collaborator

@jprendes jprendes left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

@@ -219,12 +219,9 @@ mod noptests {
fn test_nop_kill_sigkill() -> Result<(), Error> {
let nop = Arc::new(Nop::new("".to_string(), None));
Copy link
Collaborator

Choose a reason for hiding this comment

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

IIUC, you don't need this Arc either.
Same for all the other ones in the PR (inc. the wasi one).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I think you are right. Removed the Arc wrapping from tests now.

@@ -372,6 +371,8 @@ mod wasitest {
let output = read_to_string(dir.path().join("stdout"))?;
assert_eq!(output, "hello world\n");

wasi.delete()?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it make sense to have as part of trait Drop?

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'm not sure! It would look nicer but we would lose the error handling if delete() fails. Also it would be a breaking change in the API. I would maybe not add that change into this PR but think about it later.

Copy link
Member

@Mossaka Mossaka left a comment

Choose a reason for hiding this comment

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

LGTM! Happy to merge this once comments are resolved.

Copy link
Collaborator

@jprendes jprendes left a comment

Choose a reason for hiding this comment

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

LGTM!

@Mossaka Mossaka merged commit c0f8673 into containerd:main Jun 9, 2023
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