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

Set '--test-threads' option to 1 in unit tests #2685

Merged
merged 1 commit into from
Feb 16, 2024

Conversation

YJDoc2
Copy link
Collaborator

@YJDoc2 YJDoc2 commented Feb 15, 2024

Another try at #2615 and #2144

done with ref #2615 (comment)

Instead of splitting the tests by configs, tried setting the number of test threads to 1, so the tests are always run one at a time. There is no considerable change in time, the basic check CI (whole) takes about 5-6 minutes now instead of 3 minutes previously. I ran it 10 times, and it didn't fail in any attempt : https://github.com/YJDoc2/youki/actions/workflows/basic.yml?query=branch%3Atests%2Ffix-for-2144-v2

This is a much simpler solution for the issue, and as mentioned in original problem, the root cause comes from tests running in multiple threads, and should not occur in actual use ; so doing this seems a better option than code changes, if this works.

@codecov-commenter
Copy link

Codecov Report

Merging #2685 (dd44599) into main (ae48534) will increase coverage by 0.00%.
Report is 20 commits behind head on main.
The diff coverage is n/a.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2685   +/-   ##
=======================================
  Coverage   65.40%   65.41%           
=======================================
  Files         133      133           
  Lines       16942    16942           
=======================================
+ Hits        11081    11082    +1     
+ Misses       5861     5860    -1     

@YJDoc2 YJDoc2 requested a review from utam0k February 15, 2024 09:20
Copy link
Contributor

@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.

IIUC, the process will still have 2 threads, one driving the testing framework and another running the actual test. But the thread running the framework will be sleepping the whole time the test thread is running, so it should be fine.

@YJDoc2
Copy link
Collaborator Author

YJDoc2 commented Feb 15, 2024

IIUC, the process will still have 2 threads, one driving the testing framework and another running the actual test. But the thread running the framework will be sleepping the whole time the test thread is running, so it should be fine.

Yes, I think that is correct. But if we are to use cargo test, or anything else, I don't think that can be avoided, so limiting the thread to 1 was the best option I could think of :)

Another approach was as utam0k suggested, to separate the hanging tests with some kind of feature, and run them single threaded separately, but I found that some of them already had a serial attribute to them, which ideally should have run them in a single thread. Thus, went for this appraoch. If nothing else, I'm hoping single threading this would at least reduce the failure rate to a degree which can be considered rare enough to be ignored. AFAIK, the increased time due to running them serially is not that much more (considering time taken by all CI jobs, specially containerd tests) so would be ok, compared against having to re run CI due to test hangs.

Copy link
Member

@utam0k utam0k left a comment

Choose a reason for hiding this comment

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

The code is a bit changed. However, this change has been based on a lot of your research. And it solves a critical problem that bothers Youki's maintainer. I appreciate you 😍

@utam0k utam0k merged commit 762a2e8 into containers:main Feb 16, 2024
29 of 30 checks passed
@github-actions github-actions bot mentioned this pull request Feb 13, 2024
@YJDoc2 YJDoc2 deleted the tests/fix-for-2144-v2 branch February 21, 2024 05:23
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.

None yet

4 participants