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

clean up the shell script for youki integration test. #600

Merged

Conversation

utam0k
Copy link
Member

@utam0k utam0k commented Jan 9, 2022

No description provided.

@utam0k utam0k requested a review from YJDoc2 January 9, 2022 08:40
@codecov-commenter
Copy link

codecov-commenter commented Jan 9, 2022

Codecov Report

Merging #600 (b02f1b8) into main (9dd1c9d) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #600   +/-   ##
=======================================
  Coverage   70.18%   70.18%           
=======================================
  Files          82       82           
  Lines       10909    10909           
=======================================
  Hits         7656     7656           
  Misses       3253     3253           

@utam0k utam0k force-pushed the improvement/integration-test-refactoring-1 branch 3 times, most recently from 8e0fbd3 to 664651d Compare January 9, 2022 08:52
@utam0k utam0k force-pushed the improvement/integration-test-refactoring-1 branch from 664651d to b02f1b8 Compare January 9, 2022 08:58
@YJDoc2
Copy link
Collaborator

YJDoc2 commented Jan 9, 2022

Hey, I know you are still working on this but some points :

  • I would request to keep the build part, which I had specifically added for integration test developing purposes. When adding new test I have to go over iteration of change source, build the test binary and run test. But instead of running tests through the script, I run it manually so I can only run the test which I am working on , rather than all tests each time. For that the build option quickly builds and copies the binaries into the integration test directory and is easier than manually running cargo build and then copying binaries from the target dir to test dir
  • I see you have added a fixme that tests should pass with debug level as well, I'm not sure why it needs specifying that, as originally we ran the tests without specifying that env var.

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Jan 9, 2022

Also as this is the aftermath of #582 , let me know if there are any big issues with it, as in your comment #582 (review) you have said there were some details and improvements that you'd do on your side and This PR will inevitably have large impact on other people's work .

If there are any issues that need to be fixed, let me know I'll try my best 😅

@utam0k
Copy link
Member Author

utam0k commented Jan 9, 2022

@YJDoc2
The current integration test directory structure (including opencontainers/runtime-tools) is difficult to relate to, so I'm going to fix it little by little. However, this task is probably boring and uninteresting, so I'll do it. I would like you to do something more fun.
I'm sorry for the lack of explanation 🙇

@utam0k
Copy link
Member Author

utam0k commented Jan 9, 2022

@YJDoc2
I see, that was your intention. Sorry, I didn't catch your intention. Was that PR still in progress? 🙇
I understand that we need that build subcommand. However, I did not want to include an empty implementation, so I removed it once. Can I ask you to submit a PR once the implementation is finished? Sorry for the inconvenience.

I would request to keep the build part, which I had specifically added for integration test developing purposes. When adding new test I have to go over iteration of change source, build the test binary and run test. But instead of running tests through the script, I run it manually so I can only run the test which I am working on , rather than all tests each time. For that the build option quickly builds and copies the binaries into the integration test directory and is easier than manually running cargo build and then copying binaries from the target dir to test dir

@utam0k
Copy link
Member Author

utam0k commented Jan 9, 2022

Anyway, in this PR, I wanted to make sure that shell scripts can be run in any directory in order to organize the directory structure in the future.

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Jan 9, 2022

That PR was mainly for adding the test and runtimetest, so it was finished. However the script could have been improved (as seen in this PR 😅 ). The implementation of build command was nothing different, the way it worked was it would build and copy over all the binaries, and if the $2 arg was "build" it would stop there and exit the script. Otherwise, it would continue to execute the tests. In that sense, the build command was more of an early exit, rather than a command.

I see, that was your intention. Sorry, I didn't catch your intention. Was that PR still in progress? bow
I understand that we need that build subcommand. However, I did not want to include an empty implementation, so I removed it once. Can I ask you to submit a PR once the implementation is finished? Sorry for the inconvenience.

I agree, I felt this strongly as I was working on the runtimetest tool, and especially when I had to separate it into its own crate. Even now it has an issue where the cargo build or cargo clippy does not include it by default as it is not part of the workspace, but thankfully due to the way our CI is structured, it does run the clippy in CI.

I feel (in the long term) a good idea would be to separate test framework, integration tests and runtimetest into a separate repo, like opencontainers/runtime-tools. That way we can include that as a submodule like we have included the runtime-tools.

Either way we do need to have a better structure, one suggestion would be to move the whole youki workspace, including its cargo toml and target into a directory of its own so the structure would be something like /youki_workspace/crates/... that way we might be able to separate integration test into its own workspace , but I'm sure you would have a much better idea than me.

The current integration test directory structure (including opencontainers/runtime-tools) is difficult to relate to, so I'm going to fix it little by little.

Not at all ; however I'd prefer a single person to work on this, rather than multiple people creating conflicts for each other. So if you want to work on this, please go ahead, I'll work on some other tests or something else. If you want me to work on this, I don't mind it either.

However, this task is probably boring and uninteresting, so I'll do it. I would like you to do something more fun.
I'm sorry for the lack of explanation bow

This is great!

Anyway, in this PR, I wanted to make sure that shell scripts can be run in any directory in order to organize the directory structure in the future.

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 👍

@YJDoc2 YJDoc2 merged commit 0fdd344 into containers:main Jan 10, 2022
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.

None yet

3 participants