add kill test #2996

YamasouA · 2024-11-17T04:30:22Z

part of #361
I implement kill test case in runtime-tools to youki.

YJDoc2 · 2024-11-18T06:27:08Z

@YamasouA please sign your commits. The instructions can be found at https://github.com/youki-dev/youki/pull/2996/checks?check_run_id=33094881373

YamasouA · 2024-11-18T15:16:28Z

@YJDoc2
Thank you for your review!
I pass DCO.

YJDoc2 · 2024-11-23T15:08:34Z

tests/contest/contest/src/tests/kill/kill_test.rs

+    let kill_test = ConditionalTest::new(
+        "test_kill_container",
+        Box::new(|| true),
+        Box::new(run_kill_test_cases),
+    );


You can use normal test instead of ConditionalTest here, that will not require a conditional function.

YJDoc2 · 2024-11-23T15:11:39Z

Hey @YamasouA , thanks for the PR. I have taken a basic look, and want to request a change -
The go tests usually put all test cases in a single function, but here we try to separate each test case as a different test, so it is easier to understand and maintain. see the code in #2976 , how it has created one test function for readonly=true and another for readonly=false.

Can I request you to change this code to split each test condition into its own testcase? So it will be one function for testing kill without id, one for kill non-existing container and so on.

YamasouA · 2024-12-07T09:03:04Z

@YJDoc2
I fix your comment!
please review again!!

YJDoc2

overall looks ok, two things I'd request you to fix, Then I'll do the final review.

YJDoc2 · 2024-12-09T04:56:07Z

tests/contest/contest/src/tests/kill/kill_test.rs

+        TestResult::Passed => TestResult::Failed(anyhow!("Expected failure but got success")),
+        _ => TestResult::Failed(anyhow!("Unexpected test result")),


Suggested change

TestResult::Passed => TestResult::Failed(anyhow!("Expected failure but got success")),

_ => TestResult::Failed(anyhow!("Unexpected test result")),

TestResult::Passed => TestResult::Failed(anyhow!("Expected killing container with empty id to fail, but was successful")),

_ => TestResult::Failed(anyhow!("Unexpected test result")),

Similarly update the failed messages in others as well.

YJDoc2 · 2024-12-09T05:06:48Z

tests/contest/contest/src/tests/kill/kill_test.rs

+    match container.create() {
+        TestResult::Passed => {}
+        _ => return TestResult::Failed(anyhow!("Failed to create container")),
+    }
+    let result = match container.kill() {
+        TestResult::Passed => TestResult::Passed,
+        TestResult::Failed(_) => {
+            TestResult::Failed(anyhow!("Expected success but got failure"))
+        }
+        _ => TestResult::Failed(anyhow!("Unexpected test result")),
+    };


I think in both these cases you can use test_result! macro to directly return the error without having to do manual match. Same for the cases below.

container.kill() return TestResult. type.
So I think we don't have to use test_result macro.
Is it true?

YamasouA · 2024-12-09T14:54:02Z

@YJDoc2
Thank you for your review.
I fix your comment and fix lint error.

YJDoc2

Hey, so few things need change -

I have left a couple of comments, please take a look.
In the original go tests, it uses special config for stopped container and running container :
- It uses a config with command true which would immediately exit, and thus we get a stopped container
- It uses a config with command sleep 30 and waits 10 seconds so to make sure container is running and then attempts to kill the container for running container
  In our current lifecycle impl, we do not have support for passing custom command, we always use sleep 10. So to get the correct behavior we can either modify the lifecycle impl (maybe with a new fn) to allow customizing the command, or for stopped container test, we sleep 15 seconds, then kill and for running test we sleep ~3 seconds and then kill.

Can you check what would be better and update the test accordingly? Thanks :)

YJDoc2 · 2024-12-13T15:43:10Z

tests/contest/contest/src/tests/kill/kill_test.rs

+    match container.create() {
+        TestResult::Passed => {}
+        _ => return TestResult::Failed(anyhow!("Failed to create container")),
+    }
+    match container.delete() {
+        TestResult::Passed => {}
+        _ => return TestResult::Failed(anyhow!("Failed to delete container")),
+    }


I think here you need to call container.start instead of delete, then wait a little, then call kill and finally delete.

YJDoc2 · 2024-12-13T15:44:51Z

tests/contest/contest/src/tests/kill/kill_test.rs

+fn kill_start_container_test() -> TestResult {
+    let container = ContainerLifecycle::new();
+
+    // kill start container
+    match container.create() {
+        TestResult::Passed => {}
+        _ => return TestResult::Failed(anyhow!("Failed to recreate container")),
+    }


Is this supposed to be the last test case from original go test kill a running container ?

YamasouA · 2024-12-15T14:28:53Z

@YJDoc2
I wrote some util function to support this test like your advice, and update test.

YamasouA · 2025-01-22T13:36:29Z

ping @YJDoc2

YJDoc2 · 2025-01-22T13:58:25Z

Hey @YamasouA , apologies I haven't been able to take a look at this. I had been quite busy, and then got a bit sick, and currently just have a lot of things I need to work on 😅
I'll try my best to take a look at this as soon as possible. In the meantime, can I ask you to sync this with main branch, and resolve the conflict in main?

Again, sincere apologies, I should have done better managing this 🙏

YamasouA · 2025-01-24T14:23:13Z

@YJDoc2
I understand your situation!
No problem.
Thanks for the reply!

utam0k · 2025-01-31T12:25:27Z

@YamasouA I've checked your PR this weekend.

utam0k · 2025-02-02T02:30:35Z

tests/contest/contest/src/tests/kill/kill_test.rs

+use crate::tests::lifecycle::ContainerLifecycle;
+
+fn create_spec(args: &[&str]) -> Result<Spec> {
+    let args_vec: Vec<String> = args.iter().map(|&a| a.to_string()).collect();


Suggested change

let args_vec: Vec<String> = args.iter().map(|&a| a.to_string()).collect();

let args_vec: Vec<String> = args.iter().map(|&a| a.into()).collect();

utam0k · 2025-02-02T02:33:40Z

tests/contest/contest/src/tests/kill/kill_test.rs

+            "Unexpected killing non existed container test result"
+        )),
+    };
+    container.delete();


I guess we should delete the container even if the test failed to clean up.

Also, please take care of the return value of delete()

YamasouA · 2025-02-03T14:44:14Z

@utam0k
Thank you for your review!
I fix your review point.
I take a mistake to rebase and commit log is dirty.
Should I create new PR?

utam0k · 2025-02-04T11:34:49Z

Should I create new PR?

No needed. How about re-checking the branch from the latest main branch and applying the diff? Of course, I don't mind using force-push.
https://patch-diff.githubusercontent.com/raw/youki-dev/youki/pull/2996.diff

Signed-off-by: Akiyama <[email protected]>

YamasouA · 2025-02-04T14:10:14Z

@utam0k
Super thank you!!
I didn't know apply patch command.
It's very useful knowledge!!

YamasouA · 2025-03-01T03:30:58Z

@utam0k
The review points were corrected in this commit. 78df79a
Sorry for not separating the commits.
Could you please review it again?
And how to rerun tests?

utam0k · 2025-03-06T12:25:27Z

tests/contest/contest/src/tests/lifecycle/container_lifecycle.rs

        )
    }
+
+    pub fn waiting_for_status(


This function is duplicated with https://github.com/youki-dev/youki/pull/3082/files#diff-7c81e28b8064c3c2c13f68c038ca10661530915ffd2159b939e1dba7684b40e8

The PR has been merged.

YJDoc2 · 2025-04-21T09:40:01Z

@YamasouA , are you still working on this? If it is not possible for you , that is ok, just let us know.
If you are still working on this, there are couple of comments and conflicts that needs to be fixed , may I ask you to take a look at them? Thanks :)

Signed-off-by: YamasouA <[email protected]>

YJDoc2 · 2025-04-28T12:40:29Z

Hey, thanks for the update :)
I think you need to either rebase or merge with main to fix the failing CIs, can you also take a look at that? Thanks :)

Signed-off-by: Akiyama <[email protected]>

YamasouA · 2025-05-03T06:13:56Z

@YJDoc2
Sorry, I overlooked a few corrections.

Signed-off-by: Akiyama <[email protected]>

YJDoc2

Hey, LGTM! Thanks for the PR. One last thing - Can I ask you to add comments to the test functions and explanation for what is the expected behavior for those tests? That will help when we work on these tests in future. Not a blocking though, so if you can't get to it in few days, I'll merge this as-is.

Thanks again :)

Signed-off-by: Akiyama <[email protected]>

YamasouA · 2025-05-11T04:37:31Z

@YJDoc2
I add some comment.
Please check!!

YJDoc2 · 2025-05-12T04:48:16Z

@utam0k I think your change request has been satisfied, so I'll go ahead and merge.

YamasouA changed the title ~~[WIP] add kill test~~ add kill test Nov 17, 2024

YJDoc2 added the kind/test label Nov 18, 2024

YamasouA force-pushed the test/kill branch from 3365ecf to 553ed4d Compare November 18, 2024 15:11

YJDoc2 reviewed Nov 23, 2024

View reviewed changes

YamasouA force-pushed the test/kill branch 3 times, most recently from 8db2447 to c2ed117 Compare December 7, 2024 09:01

YJDoc2 requested changes Dec 9, 2024

View reviewed changes

YJDoc2 reviewed Dec 13, 2024

View reviewed changes

YamasouA force-pushed the test/kill branch from 7ef3f5a to 5dc2dbf Compare December 15, 2024 14:26

utam0k self-requested a review January 24, 2025 09:48

utam0k requested changes Feb 2, 2025

View reviewed changes

YamasouA force-pushed the test/kill branch 4 times, most recently from fc10174 to 4423738 Compare February 3, 2025 14:41

apply test kill patch

78df79a

Signed-off-by: Akiyama <[email protected]>

YamasouA force-pushed the test/kill branch from 4423738 to 78df79a Compare February 4, 2025 14:07

YamasouA added 2 commits February 12, 2025 14:05

Merge branch 'main' into test/kill

ab2461e

Merge branch 'main' into test/kill

af0f0ed

utam0k reviewed Mar 6, 2025

View reviewed changes

utam0k force-pushed the main branch from e02e0d9 to a8b8fe2 Compare March 18, 2025 11:56

fix conflict

86ff111

Signed-off-by: YamasouA <[email protected]>

YamasouA added 2 commits May 3, 2025 06:09

fix

354b99a

Signed-off-by: Akiyama <[email protected]>

Merge branch 'main' into test/kill

e86b7a6

lint

076a1c2

Signed-off-by: Akiyama <[email protected]>

YJDoc2 approved these changes May 6, 2025

View reviewed changes

add comment

d2bba7d

Signed-off-by: Akiyama <[email protected]>

YJDoc2 enabled auto-merge (squash) May 12, 2025 04:30

YJDoc2 disabled auto-merge May 12, 2025 04:44

YJDoc2 merged commit a490965 into youki-dev:main May 12, 2025
27 checks passed

github-actions bot mentioned this pull request May 4, 2025

Release for v0.5.4 #3124

Merged

This was referenced Jun 9, 2025

Remove oci tests that are duplicates of contest #3184

Merged

[Tracking Issue] Implement Integraion Tests #361

Open

		TestResult::Passed => TestResult::Failed(anyhow!("Expected failure but got success")),
		_ => TestResult::Failed(anyhow!("Unexpected test result")),

	let args_vec: Vec<String> = args.iter().map(\|&a\| a.to_string()).collect();
	let args_vec: Vec<String> = args.iter().map(\|&a\| a.into()).collect();

add kill test #2996

add kill test #2996

Conversation

YamasouA commented Nov 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

YJDoc2 commented Nov 18, 2024

Uh oh!

YamasouA commented Nov 18, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

YJDoc2 commented Nov 23, 2024

Uh oh!

YamasouA commented Dec 7, 2024

Uh oh!

YJDoc2 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

YamasouA commented Dec 9, 2024

Uh oh!

YJDoc2 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

YamasouA commented Dec 15, 2024

Uh oh!

YamasouA commented Jan 22, 2025

Uh oh!

YJDoc2 commented Jan 22, 2025

Uh oh!

YamasouA commented Jan 24, 2025

Uh oh!

utam0k commented Jan 31, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

YamasouA commented Feb 3, 2025

Uh oh!

utam0k commented Feb 4, 2025

Uh oh!

YamasouA commented Feb 4, 2025

Uh oh!

YamasouA commented Mar 1, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

YJDoc2 commented Apr 21, 2025

Uh oh!

YJDoc2 commented Apr 28, 2025

Uh oh!

YamasouA commented May 3, 2025

Uh oh!

YJDoc2 left a comment

Choose a reason for hiding this comment

Uh oh!

YamasouA commented May 11, 2025

Uh oh!

YJDoc2 commented May 12, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

YamasouA commented Nov 17, 2024 •

edited

Loading