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

refactor(test): refine function create_container #2973

Merged

Conversation

xujihui1985
Copy link
Contributor

add CreateOptions as para to function create_container for create container with different options
this is a followup PR for #2923

@xujihui1985
Copy link
Contributor Author

@YJDoc2 this PR is to follow up the previous PR #2923, remove the redundant function and fix the test as well

@YJDoc2 YJDoc2 self-requested a review November 5, 2024 13:28
@YJDoc2
Copy link
Collaborator

YJDoc2 commented Nov 5, 2024

Hey @xujihui1985 thanks! I'll try to get to this in this week.

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Nov 5, 2024

Also, would you be willing to wait for few other PRs to be merged before this? There are several other test PRs which are in review, and if possible, I'd like to merge them before and ask you to rebase on main after that so there are no tests with older impl. If you're fine, I'll ping you once this is ready to rebase.

@xujihui1985
Copy link
Contributor Author

Also, would you be willing to wait for few other PRs to be merged before this? There are several other test PRs which are in review, and if possible, I'd like to merge them before and ask you to rebase on main after that so there are no tests with older impl. If you're fine, I'll ping you once this is ready to rebase.

sure, no rush. take your time

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Nov 23, 2024

@xujihui1985 , please go ahead and rebase this on main. Once this is updated, I'll merge 👍

add CreateOptions as para to function create_container for create
container with different options

Signed-off-by: xujihui1985 <[email protected]>
@xujihui1985 xujihui1985 force-pushed the refactor/test_container_with_options branch from fac48fd to 17f2131 Compare November 25, 2024 10:25
Signed-off-by: xujihui1985 <[email protected]>
@xujihui1985
Copy link
Contributor Author

@xujihui1985 , please go ahead and rebase this on main. Once this is updated, I'll merge 👍

I have rebased from main branch and fix all the tests

Comment on lines -193 to -203
let create_process = create_container(&id_str, &bundle).unwrap();
// here we do not wait for the process by calling wait() as in the test_outside_container
// function because we need the output of the runtimetest. If we call wait, it will return
// and we won't have an easy way of getting the stdio of the runtimetest.
// Thus to make sure the container is created, we just wait for sometime, and
// assume that the create command was successful. If it wasn't we can catch that error
// in the start_container, as we can not start a non-created container anyways
std::thread::sleep(std::time::Duration::from_millis(1000));
match start_container(&id_str, &bundle)
.unwrap()
.wait_with_output()
Copy link
Collaborator

Choose a reason for hiding this comment

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

This diff is wild! 😂

Copy link
Collaborator

@YJDoc2 YJDoc2 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for your efforts!

@YJDoc2 YJDoc2 merged commit 7c18c67 into youki-dev:main Nov 25, 2024
27 checks passed
@github-actions github-actions bot mentioned this pull request Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants