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

Revert "Apply timeout-scale to test_thr_kill." #7838

Merged

Conversation

junaruga
Copy link
Member

This reverts commit bbbec4b.
Because the commit is a bug. The bug was reported on the commit link bbbec4b. The apply_timeout_scale to scale the timeout is called 2 times in the process.

The test/ruby/test_thread_queue.rb#test_thr_kill is calling EnvUtil.apply_timeout_scale, and
calling tool/lib/core_assertions.rb#assert_normal_exit. calling tool/lib/envutil.rb#invoke_ruby (EnvUtil.invoke_ruby) calling the apply_timeout_scale.

I confirmed the issues by adding the debug log after each EnvUtil.apply_timeout_scale.

diff --git a/test/ruby/test_thread_queue.rb b/test/ruby/test_thread_queue.rb
index 3d2d1b23e9..09c33d1561 100644
--- a/test/ruby/test_thread_queue.rb
+++ b/test/ruby/test_thread_queue.rb
@@ -209,6 +209,7 @@ def test_thr_kill
     bug5343 = '[ruby-core:39634]'
     Dir.mktmpdir {|d|
       timeout = EnvUtil.apply_timeout_scale(60)
+      puts "[DEBUG] test/ruby/test_thread_queue.rb#test_thr_kill.: timeout: #{timeout}"
       total_count = 250
       begin
         assert_normal_exit(<<-"_eom", bug5343, timeout: timeout, chdir: d)
diff --git a/tool/lib/envutil.rb b/tool/lib/envutil.rb
index 728ca7059b..a4c4c7304f 100644
--- a/tool/lib/envutil.rb
+++ b/tool/lib/envutil.rb
@@ -130,6 +130,7 @@ def invoke_ruby(args, stdin_data = "", capture_stdout = false, capture_stderr =
                   rubybin: EnvUtil.rubybin, precommand: nil,
                   **opt)
     timeout = apply_timeout_scale(timeout)
+    puts "[DEBUG] tool/lib/envutil.rb#invoke_ruby: timeout: #{timeout}"
 
     in_c, in_p = IO.pipe
     out_p, out_c = IO.pipe if capture_stdout

The timeout is scaling from 60 to 120 and to 240 seconds.

$ make test-all V=1 TESTS="-v test/ruby/test_thread_queue.rb -n TestThreadQueue#test_thr_kill --timeout-scale=2"
[1/0] TestThreadQueue#test_thr_kill[DEBUG] test/ruby/test_thread_queue.rb#test_thr_kill.: timeout: 120.0
[DEBUG] tool/lib/envutil.rb#invoke_ruby: timeout: 240.0
 = 0.07 s
Finished tests in 0.074814s, 13.3665 tests/s, 66.8323 assertions/s.
1 tests, 5 assertions, 0 failures, 0 errors, 0 skips

This reverts commit bbbec4b.
Because the commit is a bug. The `apply_timeout_scale` to scale the timeout is
called 2 times in the process.

The `test/ruby/test_thread_queue.rb#test_thr_kill` is calling
`EnvUtil.apply_timeout_scale`, and
calling `tool/lib/core_assertions.rb#assert_normal_exit`.
calling `tool/lib/envutil.rb#invoke_ruby` (`EnvUtil.invoke_ruby`)
calling the `apply_timeout_scale`.

```
$ make test-all V=1 TESTS="-v test/ruby/test_thread_queue.rb -n TestThreadQueue#test_thr_kill --timeout-scale=2"
```
@junaruga junaruga merged commit b0a25c9 into ruby:master May 22, 2023
91 of 95 checks passed
@junaruga junaruga deleted the wip/revert-test_thr_kill-timeout-scale branch May 22, 2023 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
1 participant