Add integration tests for life cycle #113

niwatoliver · 2021-06-27T19:44:20Z

Implemented integration tests.
First of all, I implemented tests for the container lifecycle and the create command, referring to opencontainers/runtime-tools.

See tests/README.md for how to run the integration tests.

The flow of test execution is not to run cargo test directly, but to first generate a binary and then run it via sudo.

I am currently trying not to put it on the CI, but will do so if I can.

ref: #56

utam0k · 2021-06-28T10:55:19Z

@minakawa-daiki
Thank you for the PR. I'll review it later. I'm a little busy so it may be later.

niwatoliver · 2021-07-02T21:49:35Z

I'd like to ask about binary generation for tests. Currently, the binary obtained by cargo test --no-run has a hash value and cannot be uniquely identified.
The binary obtained by cargo test --no-run currently has a hash value and cannot be uniquely identified, so as discussed in the following issue, we parse the json with --message-format=json to get the binary filename.
I can't think of a better idea at the moment, but what do you think?

rust-lang/cargo#1924
rust-lang/cargo#3670

utam0k · 2021-07-03T01:20:13Z

@minakawa-daiki
I think it would be better to make it independent from youki's main code, like oci-spec. And how about writing it in main instead of the test folder as a test tool instead of test? What do you think?

tsturzl · 2021-07-08T15:58:11Z

I agree with @utam0k. It would be best to split this out into another module. I would be really interested in contributing in this area, as I think we'll want to start building out our own test suite for cgroups v2. Maybe we could even make this not a cargo test at all, and do something similar to the oci runtime-tools and use the test anything protocol or similar. This way we can essentially just run the built tests which can invoke a new build of the project and test it without having to do anything hacky with cargo. Food for thought.

niwatoliver · 2021-07-10T06:04:41Z

I apologize for the delay in my response. It's certainly better to separate it as a testing tool, and I've reminded myself that there's no need to force it to be included in cargo test. I'm a little busy with my personal life and will deal with this a little later, but I'll try.

tsturzl · 2021-07-14T22:44:15Z

@minakawa-daiki Let me know if you need any help with this. I think if the effort to break this out into it's own tool is too high that perhaps we allow this change in, and when I have time I can move this into it's own crate which will serve as a place to build out our own integration testing tool. What do you think @utam0k?

utam0k · 2021-07-15T13:39:49Z

@minakawa-daiki Let me know if you need any help with this. I think if the effort to break this out into it's own tool is too high that perhaps we allow this change in, and when I have time I can move this into it's own crate which will serve as a place to build out our own integration testing tool. What do you think @utam0k?

@tsturzl
I would like to hear the opinion of the author of this PR and make a decision. If he is busy, I think your suggestion is fine.

YJDoc2 · 2021-07-15T13:51:53Z

Hey, I was checking the test anything protocol which is used by oci-spec validation tests, and rust has testanything crate which provides producer of data for tests as per TAP protocol. I'm not sure how much maintained and active it is, but that might be useful. We can use that as the part of our main program, and take path of youki executable and bundle etc.as cmd options. Then our binary will run all the tests that are in oci-validation tests, written in rust, and produce output using testanything. Maybe that way we won't need to create a complete tool for testing right now, instead just port validation suite, which will be used by us to validate the changes. @minakawa-daiki @tsturzl ?
Also, If you decide to port the tests to rust in some way, let me know , I would like to help.

niwatoliver · 2021-07-15T17:08:18Z

@tsturzl @utam0k
Thank you for your concern!
I'm currently writing the code to migrate using oci_spec as a reference. I'll be able to submit a pull request after this, and I hope you'll take a look at that code and decide for yourself!

niwatoliver · 2021-07-15T17:10:42Z

@YJDoc2
I think testanything is a very good idea!
I think we can use this to implement it once, and if testanything is not active, we can make it a policy to replace the implementation ourselves later. Thanks!

Update Integration test lifecycle

niwatoliver · 2021-07-15T20:06:27Z

@utam0k @tsturzl @YJDoc2
I wrote out the integration test as a test tool.
Since the integration-test directory already exists, it is temporarily named youki-integration-test.
As an implementation, I used testanything that @YJDoc2 taught me. As the first test, I've added tests for the container lifecycle and the create command.
Please refer to the Readme.md for more information on how to use the tests.
I hope you will review if this is a good policy for now.

utam0k · 2021-07-17T15:10:29Z

youki_integration_test/Cargo.toml

+rand = "0.8.0"
+tar = "0.4"
+flate2 = "1.0"
+testanything = "0.2.1"


How about error handling anyhow?

utam0k · 2021-07-17T15:14:45Z

youki_integration_test/src/main.rs

+}
+
+// This tests the entire lifecycle of the container.
+fn life_cycle_test(project_path: &PathBuf) {


The same applies to the other functions, it is recommended to do the following

Suggested change

fn life_cycle_test(project_path: &PathBuf) {

fn life_cycle_test(project_path: &Path) {

https://rust-lang.github.io/rust-clippy/master/index.html#ptr_arg

utam0k · 2021-07-17T15:16:24Z

youki_integration_test/src/support.rs

+
+pub fn initialize_test(project_path: &PathBuf) -> Result<(), std::io::Error> {
+    let result = prepare_test_workspace(project_path);
+    return result;


The return is unnecessary.

utam0k · 2021-07-17T15:16:29Z

youki_integration_test/src/support.rs

+
+pub fn cleanup_test(project_path: &PathBuf) -> Result<(), std::io::Error> {
+    let result = delete_test_workspace(project_path);
+    return result;


The return is unnecessary.

utam0k · 2021-07-17T15:16:39Z

youki_integration_test/src/support.rs

+
+pub fn create_project_path() -> PathBuf {
+    let current_dir_path_result = env::current_dir();
+    return match current_dir_path_result {


The return is unnecessary.

utam0k · 2021-07-17T15:17:12Z

youki_integration_test/src/support.rs

+        .finalize()
+}
+
+pub fn print_test_results(test_name: &str, tests: Vec<TapTest>) -> () {


The () is unnecessary.

utam0k · 2021-07-17T15:19:41Z

youki_integration_test/src/command/create.rs

+// TODO Allow to receive arguments.
+// TODO Wrapping up the results
+pub fn exec(project_path: &PathBuf, id: &str) -> bool {
+    let status = Command::new(project_path.join(PathBuf::from("youki")))


I think "youki" is too hard coding, so how about using an environment value, e.g. RUNTIME
I think the default value should be youki.

What would be your thoughts on using cmd args instead of env vars? That way we user won't have to define env vars specifically for this purpose, and it might make more sense to pass those as args to the command, rather than to set them as general env vars.

@utamoku What do you think about this?
My feeling is that when comparing env vars and cmd args, env vars is basically unchanged and users are expected to use default values, while cmd args is assumed to be specified by users.
In this case, I think env vars is fine because it is basically used with youki, but cmd args may be an option if you are considering using a runtime other than youki in the future.

utam0k · 2021-07-17T15:31:36Z

youki_integration_test/src/main.rs

+
+// This tests the entire lifecycle of the container.
+fn life_cycle_test(project_path: &PathBuf) {
+    let youki = Container::new(project_path);


I felt that the variable name container_runtime was a better level of abstraction.

utam0k

Nice!
However, I commented on a few points.
But basically, I think that once the commented parts are fixed, we can merge them and discuss them with the developers to improve the whole` thing. It is good enough for a first prototype.

utam0k · 2021-07-17T15:39:32Z

@minakawa-daiki I ran the test according to the README, and it failed. Is there something wrong with this?

 sudo ./youki_integration_test
# Start Create comand test suite
1..6
ok 1 Create a new container test
# This operation must create a new container.
ok 2 Execute state test
# This operation must state the container.
ok 3 Execute start test
# This operation must start the container.
ok 4 Execute state test
# This operation must state the container.
ok 5 Execute kill test
# This operation must kill the container.
not ok 6 Execute delete test
# This operation must delete the container.
# End Create comand test suite
# Start Create comand test suite
1..3
ok 1 create with no ID test
# This operation MUST generate an error if it is not provided a path to the bundle and the container ID to associate with the container.
ok 2 create with ID test
# This operation MUST create a new container.
ok 3 create with an already existing ID test
# If the ID provided is not unique across all containers within the scope of the runtime, or is not valid in any other way, the implementation MUST generate an error and a new container MUST NOT be created.
# End Create comand test suite

YJDoc2 · 2021-07-19T04:15:35Z

Would it be possible , that instead of relying on panic as indication of test failure, we take the result of spawned process, along with its stderr, and accordingly set status and error message? That way we can have more information on why the test failed.
The process ExitStatus implements fmt , so it can convert the exit code of the process to readable string, and we might be able to use it as an error message.

tsturzl · 2021-07-19T04:53:25Z

I agree with @YJDoc2. I think that would provide more context, and in one of @utam0k's review comments he mentions using the anyhow crate that we use heavily in youki already. I think that could give you a lot of useful tools for dealing with errors, and should be pretty simple to add.

YJDoc2 · 2021-07-19T09:05:55Z

@utam0k I pulled this PR and ran tests as stated, several times, but all tests give ok for me consistently. 🤔 🤔
@tsturzl can you once check and verify when you get time?

niwatoliver · 2021-07-19T18:44:35Z

As for the error messages, I'll change the implementation to use anyhow.
Please wait for a while as we will respond to other reviews as they come in.
The reason why the test fails is not yet known, so we are investigating.

niwatoliver · 2021-07-23T17:21:04Z

@utamoku Corrected the reviews we received except for the hard coding of the youki command.
I have commented on the hard coding of the youki command separately, so please check it.

utam0k · 2021-07-28T14:01:14Z

Sorry, It had nothing to do with this PR, so I sent out a PR to fix it.
#165

@utam0k I pulled this PR and ran tests as stated, several times, but all tests give ok for me consistently. thinking thinking

utam0k

This is a great PR move. And it is one of the most needed features in youki right now. However, I think there are still many things to be developed in this PR. Let's merge it once and improve it together.

niwatoliver added 6 commits June 28, 2021 04:23

Add modules needed for integration testing

27a0924

Add youki commands

8fb3e18

Implement tests to check the container lifecycle

3e6e899

Implement the code to test the create command

239305c

Add a procedure to run tests in binary

884a88c

Add skip integration

6a051ed

utam0k self-requested a review June 28, 2021 10:54

niwatoliver mentioned this pull request Jul 15, 2021

Update Integration test lifecycle niwatoliver/youki#3

Merged

niwatoliver added 4 commits July 16, 2021 04:55

Delete cargo test version

e88b3c6

Impl original integration test tool

3e6e933

Merge pull request #3 from minakawa-daiki/integration-test-lifecycle-2

13e97ab

Update Integration test lifecycle

Merge branch 'main' into integration-test-lifecycle

847df4c

utam0k reviewed Jul 17, 2021

View reviewed changes

utam0k requested changes Jul 17, 2021

View reviewed changes

niwatoliver added 4 commits July 24, 2021 01:41

Apply cargo clippy

8b42965

Rename variable name to container_runtime

345dc60

Update error handlings

3c98b5e

Run cargo fmt

83bc28a

utam0k approved these changes Jul 28, 2021

View reviewed changes

utam0k merged commit 12cede7 into youki-dev:main Jul 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add integration tests for life cycle #113

Add integration tests for life cycle #113

niwatoliver commented Jun 27, 2021 •

edited

Loading

utam0k commented Jun 28, 2021

niwatoliver commented Jul 2, 2021

utam0k commented Jul 3, 2021

tsturzl commented Jul 8, 2021 •

edited

Loading

niwatoliver commented Jul 10, 2021

tsturzl commented Jul 14, 2021

utam0k commented Jul 15, 2021 •

edited

Loading

YJDoc2 commented Jul 15, 2021

niwatoliver commented Jul 15, 2021

niwatoliver commented Jul 15, 2021

niwatoliver commented Jul 15, 2021

utam0k Jul 17, 2021

utam0k Jul 17, 2021 •

edited

Loading

utam0k Jul 17, 2021

utam0k Jul 17, 2021

utam0k Jul 17, 2021

utam0k Jul 17, 2021

utam0k Jul 17, 2021 •

edited

Loading

YJDoc2 Jul 19, 2021

niwatoliver Jul 23, 2021

utam0k Jul 17, 2021

utam0k left a comment

utam0k commented Jul 17, 2021

YJDoc2 commented Jul 19, 2021

tsturzl commented Jul 19, 2021 •

edited

Loading

YJDoc2 commented Jul 19, 2021 •

edited

Loading

niwatoliver commented Jul 19, 2021

niwatoliver commented Jul 23, 2021

utam0k commented Jul 28, 2021

utam0k left a comment

	fn life_cycle_test(project_path: &PathBuf) {
	fn life_cycle_test(project_path: &Path) {

Add integration tests for life cycle #113

Add integration tests for life cycle #113

Conversation

niwatoliver commented Jun 27, 2021 • edited Loading

utam0k commented Jun 28, 2021

niwatoliver commented Jul 2, 2021

utam0k commented Jul 3, 2021

tsturzl commented Jul 8, 2021 • edited Loading

niwatoliver commented Jul 10, 2021

tsturzl commented Jul 14, 2021

utam0k commented Jul 15, 2021 • edited Loading

YJDoc2 commented Jul 15, 2021

niwatoliver commented Jul 15, 2021

niwatoliver commented Jul 15, 2021

niwatoliver commented Jul 15, 2021

Choose a reason for hiding this comment

utam0k Jul 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

utam0k Jul 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

utam0k left a comment

Choose a reason for hiding this comment

utam0k commented Jul 17, 2021

YJDoc2 commented Jul 19, 2021

tsturzl commented Jul 19, 2021 • edited Loading

YJDoc2 commented Jul 19, 2021 • edited Loading

niwatoliver commented Jul 19, 2021

niwatoliver commented Jul 23, 2021

utam0k commented Jul 28, 2021

utam0k left a comment

Choose a reason for hiding this comment

niwatoliver commented Jun 27, 2021 •

edited

Loading

tsturzl commented Jul 8, 2021 •

edited

Loading

utam0k commented Jul 15, 2021 •

edited

Loading

utam0k Jul 17, 2021 •

edited

Loading

utam0k Jul 17, 2021 •

edited

Loading

tsturzl commented Jul 19, 2021 •

edited

Loading

YJDoc2 commented Jul 19, 2021 •

edited

Loading