From 892e38c78e03c11afaa5f01d995e3a21bd92b415 Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono Date: Fri, 21 Dec 2018 02:44:01 +0900 Subject: Enable `Style/RedundantBegin` cop to avoid newly adding redundant begin block Currently we sometimes find a redundant begin block in code review (e.g. https://github.com/rails/rails/pull/33604#discussion_r209784205). I'd like to enable `Style/RedundantBegin` cop to avoid that, since rescue/else/ensure are allowed inside do/end blocks in Ruby 2.5 (https://bugs.ruby-lang.org/issues/12906), so we'd probably meets with that situation than before. --- .../cache/behaviors/connection_pool_behavior.rb | 56 ++++++----- activesupport/test/core_ext/load_error_test.rb | 7 +- activesupport/test/json/encoding_test.rb | 24 +++-- activesupport/test/share_lock_test.rb | 102 ++++++++++----------- activesupport/test/time_travel_test.rb | 74 +++++++-------- 5 files changed, 123 insertions(+), 140 deletions(-) (limited to 'activesupport/test') diff --git a/activesupport/test/cache/behaviors/connection_pool_behavior.rb b/activesupport/test/cache/behaviors/connection_pool_behavior.rb index 4d1901a173..5e66e0b202 100644 --- a/activesupport/test/cache/behaviors/connection_pool_behavior.rb +++ b/activesupport/test/cache/behaviors/connection_pool_behavior.rb @@ -7,24 +7,22 @@ module ConnectionPoolBehavior threads = [] emulating_latency do - begin - cache = ActiveSupport::Cache.lookup_store(store, { pool_size: 2, pool_timeout: 1 }.merge(store_options)) - cache.clear - - assert_raises Timeout::Error do - # One of the three threads will fail in 1 second because our pool size - # is only two. - 3.times do - threads << Thread.new do - cache.read("latency") - end + cache = ActiveSupport::Cache.lookup_store(store, { pool_size: 2, pool_timeout: 1 }.merge(store_options)) + cache.clear + + assert_raises Timeout::Error do + # One of the three threads will fail in 1 second because our pool size + # is only two. + 3.times do + threads << Thread.new do + cache.read("latency") end - - threads.each(&:join) end - ensure - threads.each(&:kill) + + threads.each(&:join) end + ensure + threads.each(&:kill) end ensure Thread.report_on_exception = original_report_on_exception @@ -34,24 +32,22 @@ module ConnectionPoolBehavior threads = [] emulating_latency do - begin - cache = ActiveSupport::Cache.lookup_store(store, store_options) - cache.clear - - assert_nothing_raised do - # Default connection pool size is 5, assuming 10 will make sure that - # the connection pool isn't used at all. - 10.times do - threads << Thread.new do - cache.read("latency") - end + cache = ActiveSupport::Cache.lookup_store(store, store_options) + cache.clear + + assert_nothing_raised do + # Default connection pool size is 5, assuming 10 will make sure that + # the connection pool isn't used at all. + 10.times do + threads << Thread.new do + cache.read("latency") end - - threads.each(&:join) end - ensure - threads.each(&:kill) + + threads.each(&:join) end + ensure + threads.each(&:kill) end end diff --git a/activesupport/test/core_ext/load_error_test.rb b/activesupport/test/core_ext/load_error_test.rb index 126aa51cb4..8afff4fb9c 100644 --- a/activesupport/test/core_ext/load_error_test.rb +++ b/activesupport/test/core_ext/load_error_test.rb @@ -13,10 +13,9 @@ class TestLoadError < ActiveSupport::TestCase end def test_path - begin load "nor/this/one.rb" - rescue LoadError => e - assert_equal "nor/this/one.rb", e.path - end + load "nor/this/one.rb" + rescue LoadError => e + assert_equal "nor/this/one.rb", e.path end def test_is_missing_with_nil_path diff --git a/activesupport/test/json/encoding_test.rb b/activesupport/test/json/encoding_test.rb index 8062873386..7589ffd0ea 100644 --- a/activesupport/test/json/encoding_test.rb +++ b/activesupport/test/json/encoding_test.rb @@ -21,20 +21,18 @@ class TestJSONEncoding < ActiveSupport::TestCase JSONTest::EncodingTestCases.constants.each do |class_tests| define_method("test_#{class_tests[0..-6].underscore}") do - begin - prev = ActiveSupport.use_standard_json_time_format - - standard_class_tests = /Standard/.match?(class_tests) - - ActiveSupport.escape_html_entities_in_json = !standard_class_tests - ActiveSupport.use_standard_json_time_format = standard_class_tests - JSONTest::EncodingTestCases.const_get(class_tests).each do |pair| - assert_equal pair.last, sorted_json(ActiveSupport::JSON.encode(pair.first)) - end - ensure - ActiveSupport.escape_html_entities_in_json = false - ActiveSupport.use_standard_json_time_format = prev + prev = ActiveSupport.use_standard_json_time_format + + standard_class_tests = /Standard/.match?(class_tests) + + ActiveSupport.escape_html_entities_in_json = !standard_class_tests + ActiveSupport.use_standard_json_time_format = standard_class_tests + JSONTest::EncodingTestCases.const_get(class_tests).each do |pair| + assert_equal pair.last, sorted_json(ActiveSupport::JSON.encode(pair.first)) end + ensure + ActiveSupport.escape_html_entities_in_json = false + ActiveSupport.use_standard_json_time_format = prev end end diff --git a/activesupport/test/share_lock_test.rb b/activesupport/test/share_lock_test.rb index 34479020e1..30a1ddad3f 100644 --- a/activesupport/test/share_lock_test.rb +++ b/activesupport/test/share_lock_test.rb @@ -115,68 +115,66 @@ class ShareLockTest < ActiveSupport::TestCase def test_exclusive_conflicting_purpose [true, false].each do |use_upgrading| with_thread_waiting_in_lock_section(:sharing) do |sharing_thread_release_latch| - begin - together = Concurrent::CyclicBarrier.new(2) - conflicting_exclusive_threads = [ - Thread.new do - @lock.send(use_upgrading ? :sharing : :tap) do - together.wait - @lock.exclusive(purpose: :red, compatible: [:green, :purple]) { } - end - end, - Thread.new do - @lock.send(use_upgrading ? :sharing : :tap) do - together.wait - @lock.exclusive(purpose: :blue, compatible: [:green]) { } - end + together = Concurrent::CyclicBarrier.new(2) + conflicting_exclusive_threads = [ + Thread.new do + @lock.send(use_upgrading ? :sharing : :tap) do + together.wait + @lock.exclusive(purpose: :red, compatible: [:green, :purple]) { } + end + end, + Thread.new do + @lock.send(use_upgrading ? :sharing : :tap) do + together.wait + @lock.exclusive(purpose: :blue, compatible: [:green]) { } end - ] - - assert_threads_stuck conflicting_exclusive_threads # wait for threads to get into their respective `exclusive {}` blocks - - # This thread will be stuck as long as any other thread is in - # a sharing block. While it's blocked, it holds no lock, so it - # doesn't interfere with any other attempts. - no_purpose_thread = Thread.new do - @lock.exclusive { } end - assert_threads_stuck no_purpose_thread + ] - # This thread is compatible with both of the "primary" - # attempts above. It's initially stuck on the outer share - # lock, but as soon as that's released, it can run -- - # regardless of whether those threads hold share locks. - compatible_thread = Thread.new do - @lock.exclusive(purpose: :green, compatible: []) { } - end - assert_threads_stuck compatible_thread + assert_threads_stuck conflicting_exclusive_threads # wait for threads to get into their respective `exclusive {}` blocks - assert_threads_stuck conflicting_exclusive_threads + # This thread will be stuck as long as any other thread is in + # a sharing block. While it's blocked, it holds no lock, so it + # doesn't interfere with any other attempts. + no_purpose_thread = Thread.new do + @lock.exclusive { } + end + assert_threads_stuck no_purpose_thread + + # This thread is compatible with both of the "primary" + # attempts above. It's initially stuck on the outer share + # lock, but as soon as that's released, it can run -- + # regardless of whether those threads hold share locks. + compatible_thread = Thread.new do + @lock.exclusive(purpose: :green, compatible: []) { } + end + assert_threads_stuck compatible_thread - sharing_thread_release_latch.count_down + assert_threads_stuck conflicting_exclusive_threads - assert_threads_not_stuck compatible_thread # compatible thread is now able to squeak through + sharing_thread_release_latch.count_down - if use_upgrading - # The "primary" threads both each hold a share lock, and are - # mutually incompatible; they're still stuck. - assert_threads_stuck conflicting_exclusive_threads + assert_threads_not_stuck compatible_thread # compatible thread is now able to squeak through - # The thread without a specified purpose is also stuck; it's - # not compatible with anything. - assert_threads_stuck no_purpose_thread - else - # As the primaries didn't hold a share lock, as soon as the - # outer one was released, all the exclusive locks are free - # to be acquired in turn. + if use_upgrading + # The "primary" threads both each hold a share lock, and are + # mutually incompatible; they're still stuck. + assert_threads_stuck conflicting_exclusive_threads - assert_threads_not_stuck conflicting_exclusive_threads - assert_threads_not_stuck no_purpose_thread - end - ensure - conflicting_exclusive_threads.each(&:kill) - no_purpose_thread.kill + # The thread without a specified purpose is also stuck; it's + # not compatible with anything. + assert_threads_stuck no_purpose_thread + else + # As the primaries didn't hold a share lock, as soon as the + # outer one was released, all the exclusive locks are free + # to be acquired in turn. + + assert_threads_not_stuck conflicting_exclusive_threads + assert_threads_not_stuck no_purpose_thread end + ensure + conflicting_exclusive_threads.each(&:kill) + no_purpose_thread.kill end end end diff --git a/activesupport/test/time_travel_test.rb b/activesupport/test/time_travel_test.rb index 8c47f2cdc7..4dec0dc334 100644 --- a/activesupport/test/time_travel_test.rb +++ b/activesupport/test/time_travel_test.rb @@ -11,16 +11,14 @@ class TimeTravelTest < ActiveSupport::TestCase def test_time_helper_travel Time.stub(:now, Time.now) do - begin - expected_time = Time.now + 1.day - travel 1.day + expected_time = Time.now + 1.day + travel 1.day - assert_equal expected_time.to_s(:db), Time.now.to_s(:db) - assert_equal expected_time.to_date, Date.today - assert_equal expected_time.to_datetime.to_s(:db), DateTime.now.to_s(:db) - ensure - travel_back - end + assert_equal expected_time.to_s(:db), Time.now.to_s(:db) + assert_equal expected_time.to_date, Date.today + assert_equal expected_time.to_datetime.to_s(:db), DateTime.now.to_s(:db) + ensure + travel_back end end @@ -42,16 +40,14 @@ class TimeTravelTest < ActiveSupport::TestCase def test_time_helper_travel_to Time.stub(:now, Time.now) do - begin - expected_time = Time.new(2004, 11, 24, 01, 04, 44) - travel_to expected_time + expected_time = Time.new(2004, 11, 24, 01, 04, 44) + travel_to expected_time - assert_equal expected_time, Time.now - assert_equal Date.new(2004, 11, 24), Date.today - assert_equal expected_time.to_datetime, DateTime.now - ensure - travel_back - end + assert_equal expected_time, Time.now + assert_equal Date.new(2004, 11, 24), Date.today + assert_equal expected_time.to_datetime, DateTime.now + ensure + travel_back end end @@ -73,21 +69,19 @@ class TimeTravelTest < ActiveSupport::TestCase def test_time_helper_travel_back Time.stub(:now, Time.now) do - begin - expected_time = Time.new(2004, 11, 24, 01, 04, 44) + expected_time = Time.new(2004, 11, 24, 01, 04, 44) - travel_to expected_time - assert_equal expected_time, Time.now - assert_equal Date.new(2004, 11, 24), Date.today - assert_equal expected_time.to_datetime, DateTime.now - travel_back + travel_to expected_time + assert_equal expected_time, Time.now + assert_equal Date.new(2004, 11, 24), Date.today + assert_equal expected_time.to_datetime, DateTime.now + travel_back - assert_not_equal expected_time, Time.now - assert_not_equal Date.new(2004, 11, 24), Date.today - assert_not_equal expected_time.to_datetime, DateTime.now - ensure - travel_back - end + assert_not_equal expected_time, Time.now + assert_not_equal Date.new(2004, 11, 24), Date.today + assert_not_equal expected_time.to_datetime, DateTime.now + ensure + travel_back end end @@ -122,20 +116,18 @@ class TimeTravelTest < ActiveSupport::TestCase def test_time_helper_travel_to_with_subsequent_calls Time.stub(:now, Time.now) do - begin - initial_expected_time = Time.new(2004, 11, 24, 01, 04, 44) - subsequent_expected_time = Time.new(2004, 10, 24, 01, 04, 44) - assert_nothing_raised do - travel_to initial_expected_time - travel_to subsequent_expected_time + initial_expected_time = Time.new(2004, 11, 24, 01, 04, 44) + subsequent_expected_time = Time.new(2004, 10, 24, 01, 04, 44) + assert_nothing_raised do + travel_to initial_expected_time + travel_to subsequent_expected_time - assert_equal subsequent_expected_time, Time.now + assert_equal subsequent_expected_time, Time.now - travel_back - end - ensure travel_back end + ensure + travel_back end end -- cgit v1.2.3