Skip to content

Commit

Permalink
Remove configuration to control what we do with tests without assertions
Browse files Browse the repository at this point in the history
This is too much complexity for something low value. Let's just always
warn when a test doesn't have any assertions.

If people want to raise an error or ignore them, they can do so by
overriding `Warning.warn`.
  • Loading branch information
rafaelfranca committed Apr 30, 2024
1 parent 64ab211 commit 6a6c7e6
Show file tree
Hide file tree
Showing 10 changed files with 52 additions and 162 deletions.
8 changes: 2 additions & 6 deletions activesupport/CHANGELOG.md
@@ -1,12 +1,8 @@
* Allow assertionless tests to be reported.
* Warn on tests without assertions.

`ActiveSupport::TestCase` can be configured to report tests that do not run any assertions.
`ActiveSupport::TestCase` now warns when tests do not run any assertions.
This is helpful in detecting broken tests that do not perform intended assertions.

```ruby
config.active_support.assertionless_tests_behavior = :raise
```

*fatkodima*

* Support `hexBinary` type in `ActiveSupport::XmlMini`.
Expand Down
4 changes: 0 additions & 4 deletions activesupport/lib/active_support/railtie.rb
Expand Up @@ -124,10 +124,6 @@ class Railtie < Rails::Railtie # :nodoc:
ActiveSupport.deprecator.warn("config.active_support.remove_deprecated_time_with_zone_name is deprecated and will be removed in Rails 7.2.")
elsif k == :use_rfc4122_namespaced_uuids
ActiveSupport.deprecator.warn("config.active_support.use_rfc4122_namespaced_uuids is deprecated and will be removed in Rails 7.2.")
elsif k == :assertionless_tests_behavior
ActiveSupport.on_load(:active_support_test_case) do
ActiveSupport::TestCase.assertionless_tests_behavior = v
end
else
k = "#{k}="
ActiveSupport.public_send(k, v) if ActiveSupport.respond_to? k
Expand Down
4 changes: 2 additions & 2 deletions activesupport/lib/active_support/test_case.rb
Expand Up @@ -3,7 +3,7 @@
require "minitest"
require "active_support/testing/tagged_logging"
require "active_support/testing/setup_and_teardown"
require "active_support/testing/assertionless_tests"
require "active_support/testing/tests_without_assertions"
require "active_support/testing/assertions"
require "active_support/testing/error_reporter_assertions"
require "active_support/testing/deprecation"
Expand Down Expand Up @@ -143,7 +143,7 @@ def parallelize_teardown(&block)

include ActiveSupport::Testing::TaggedLogging
prepend ActiveSupport::Testing::SetupAndTeardown
prepend ActiveSupport::Testing::AssertionlessTests
prepend ActiveSupport::Testing::TestsWithoutAssertions
include ActiveSupport::Testing::Assertions
include ActiveSupport::Testing::ErrorReporterAssertions
include ActiveSupport::Testing::Deprecation
Expand Down
57 changes: 0 additions & 57 deletions activesupport/lib/active_support/testing/assertionless_tests.rb

This file was deleted.

@@ -0,0 +1,22 @@
# frozen_string_literal: true

module ActiveSupport
module Testing
# Warns when a test case does not perform any assertions.
#
# This is helpful in detecting broken tests that do not perform intended assertions.
module TestsWithoutAssertions # :nodoc:
def after_teardown
super

return if skipped? || error?

if assertions == 0
file, line = method(name).source_location
message = "Test is missing assertions: `#{name}` #{file}:#{line}"
warn message

This comment has been minimized.

Copy link
@dhh

dhh Apr 30, 2024

Member

No need to assign message to a local var here, imo.

This comment has been minimized.

Copy link
@rafaelfranca

rafaelfranca Apr 30, 2024

Author Member

Yes. Let me clean that up

This comment has been minimized.

Copy link
@dhh

dhh Apr 30, 2024

Member

I'd also say I hate multiple exit wounds. I'd go with:

if assertions.zero? && !skipped? && !error?
  file, line = method(name).source_location
  warn "Test is missing assertions: `#{name}` #{file}:#{line}"
end

This comment has been minimized.

Copy link
@rafaelfranca

rafaelfranca Apr 30, 2024

Author Member

Fixed that as well

end
end
end
end
end
66 changes: 0 additions & 66 deletions activesupport/test/testing/assertionless_tests_test.rb

This file was deleted.

26 changes: 26 additions & 0 deletions activesupport/test/testing/test_without_assertions_test.rb
@@ -0,0 +1,26 @@
# frozen_string_literal: true

require_relative "../abstract_unit"
require "active_support/core_ext/object/with"

module TestsWithoutAssertionsTest
module Tests
def test_without_assertions
end
end

class TestsWithoutAssertionsWarnTest < ActiveSupport::TestCase
module AfterTeardown
def after_teardown
_out, err = capture_io do
super
end

assert_match(/Test is missing assertions: `test_without_assertions` .+test_without_assertions_test\.rb:\d+/, err)
end
end

include Tests
prepend AfterTeardown
end
end
13 changes: 0 additions & 13 deletions guides/source/configuring.md
Expand Up @@ -64,7 +64,6 @@ Below are the default values associated with each target version. In cases of co
- [`config.active_record.automatically_invert_plural_associations`](#config-active-record-automatically-invert-plural-associations): `true`
- [`config.active_record.validate_migration_timestamps`](#config-active-record-validate-migration-timestamps): `true`
- [`config.active_storage.web_image_content_types`](#config-active-storage-web-image-content-types): `%w[image/png image/jpeg image/gif image/webp]`
- [`config.active_support.assertionless_tests_behavior`](#config-active-support-assertionless-tests-behavior): `:log`

#### Default Values for Target Version 7.1

Expand Down Expand Up @@ -2657,18 +2656,6 @@ The default value depends on the `config.load_defaults` target version:
| (original) | `false` |
| 7.0 | `true` |

#### `config.active_support.assertionless_tests_behavior`

Controls the behavior when a test do not run any assertions. The following options are available:

* `:ignore` - Assertionless tests will be ignored. This is the default.

* `:log` - Assertionless tests will be logged via `Rails.logger` at the `:warn` level.

* `:raise` - Assertionless tests will be considered as failed and raise an error.

* Custom proc - A custom proc can be provided. It should accept an error message.

#### `ActiveSupport::Logger.silencer`

Is set to `false` to disable the ability to silence logging in a block. The default is `true`.
Expand Down
6 changes: 0 additions & 6 deletions railties/lib/rails/application/configuration.rb
Expand Up @@ -334,12 +334,6 @@ def load_defaults(target_version)
active_record.validate_migration_timestamps = true
active_record.automatically_invert_plural_associations = true
end

if respond_to?(:active_support)
if Rails.env.local?
active_support.assertionless_tests_behavior = :log
end
end
else
raise "Unknown version #{target_version.to_s.inspect}"
end
Expand Down
Expand Up @@ -66,11 +66,3 @@
# on `Post`. With this option enabled, it will also look for a `:comments` association.
#++
# Rails.application.config.active_record.automatically_invert_plural_associations = true

###
# Controls whether Active Support should log tests without assertions.
# This is helpful in detecting broken tests that do not perform intended assertions.
# To disable this behavior, set the value to `:ignore` inside the `config/application.rb` (NOT this file).
#
#++
# Rails.application.config.active_support.assertionless_tests_behavior = :log

0 comments on commit 6a6c7e6

Please sign in to comment.