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 after unit tests #4050

Merged
merged 5 commits into from
Feb 17, 2023
Merged

Conversation

abetomo
Copy link
Contributor

@abetomo abetomo commented Feb 10, 2023

Which issue(s) this PR fixes:

Fixes #4048

What this PR does / why we need it:

After unit testing, clean up in /tmp.

(If you cannot terminate with SIGTERM and terminate with SIGKILL, some files cannot be cleaned up. #4050 (comment) #4050 (comment))

Docs Changes:

None

Release Note:

None

@socket_manager_server ->
@socket_manager_path

Signed-off-by: abetomo <[email protected]>
Files created in `/tmp` are deleted.

Signed-off-by: abetomo <[email protected]>
test/plugin/test_in_tcp.rb Outdated Show resolved Hide resolved
Backup files are generated in `/tmp` and remain there.
Set `root_dir` to generate backup files in the specified directory.

Signed-off-by: abetomo <[email protected]>
@abetomo abetomo force-pushed the clean-up-after-unit-tests branch from 0e647ed to a6b02da Compare February 13, 2023 13:08
@abetomo
Copy link
Contributor Author

abetomo commented Feb 13, 2023

With the committed changes and the following changes, all files were cleaned.

diff --git a/test/command/test_fluentd.rb b/test/command/test_fluentd.rb
index 461e19ef..9d62cd40 100644
--- a/test/command/test_fluentd.rb
+++ b/test/command/test_fluentd.rb
@@ -85,12 +85,12 @@ class TestFluentdCommand < ::Test::Unit::TestCase
         yield pid, io
         # p(here: "execute command", pid: pid, worker_pids: @worker_pids)
       ensure
-        Process.kill(:KILL, pid) rescue nil
+        Process.kill(:TERM, pid) rescue nil
         if @supervisor_pid
-          Process.kill(:KILL, @supervisor_pid) rescue nil
+          Process.kill(:TERM, @supervisor_pid) rescue nil
         end
         @worker_pids.each do |cpid|
-          Process.kill(:KILL, cpid) rescue nil
+          Process.kill(:TERM, cpid) rescue nil
         end
         # p(here: "execute command", pid: pid, exist: process_exist?(pid), worker_pids: @worker_pids, exists: @worker_pids.map{|i| process_exist?(i) })
         Timeout.timeout(10){ sleep 0.1 while process_exist?(pid) }

When you exit with SIGTERM, the shutdown process runs and the files are deleted.
However, the test was not stable because the process could not terminate in some cases.

`instance_shutdown` in `ensure` block.

Signed-off-by: abetomo <[email protected]>
@ashie
Copy link
Member

ashie commented Feb 15, 2023

How about trying SIGTERM first then SIGKILL after timeout?
Although we should make it stable but not needed to do it in this PR.

@abetomo
Copy link
Contributor Author

abetomo commented Feb 15, 2023

@ashie Thanks!
I’ll give it a try.

With SIGTERM, the shutdown process runs.
`SERVERENGINE_SOCKETMANAGER_*` etc. will be removed in the shutdown process.

When it cannot be terminated by SIGTERM, it is terminated by SIGKILL.

Signed-off-by: abetomo <[email protected]>
@abetomo abetomo marked this pull request as ready for review February 16, 2023 14:51
Copy link
Member

@ashie ashie left a comment

Choose a reason for hiding this comment

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

LGTM

@ashie
Copy link
Member

ashie commented Feb 17, 2023

Retrying some unstable tests not concerned with this PR...

Copy link
Contributor

@daipom daipom 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 the fix!

@ashie ashie merged commit 07e3ad1 into fluent:master Feb 17, 2023
@ashie
Copy link
Member

ashie commented Feb 17, 2023

Thanks!

@abetomo abetomo deleted the clean-up-after-unit-tests branch February 17, 2023 23:53
@abetomo
Copy link
Contributor Author

abetomo commented Feb 17, 2023

Thanks for your review!

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.

Files used in unit tests remain in /tmp
3 participants