-
Notifications
You must be signed in to change notification settings - Fork 21.5k
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
Allow assertionless tests to be reported #51625
Conversation
This seems pretty cool, I tried this out and it found a few cases. One annoyance when using
This makes it hard to find the actual test location when using something like |
Thanks for the feedback and I am glad you found it helpful too! I will look into this problem more closely 👍 . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
I'm a little biased but I do find this change useful and I would love to see it in Rails ❤️
return if skipped? || error? | ||
|
||
if assertions == 0 | ||
ActiveSupport::TestCase.assertionless_tests_behavior&.call |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think people find it helpful if we could pass name
(if that's a correct getter for the test name) so we could include it in both the error message or the log message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a name and also a file/line combination to be easier detectable, as in the example for spec style given by @Earlopain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is pretty great. Minitest prints relative paths by default in recent versions though, I have created a patch that does that here as well. It uses a custom assertion class to control the location:
diff --git a/activesupport/lib/active_support/testing/assertionless_tests.rb b/activesupport/lib/active_support/testing/assertionless_tests.rb
index 862b0f3d45..25d1d07c32 100644
--- a/activesupport/lib/active_support/testing/assertionless_tests.rb
+++ b/activesupport/lib/active_support/testing/assertionless_tests.rb
@@ -11,6 +11,15 @@ def self.prepended(klass)
klass.assertionless_tests_behavior = :ignore
end
+ class NoAssertions < Minitest::Assertion
+ attr_reader :location
+
+ def initialize(message, test_location)
+ @location = test_location
+ super(message)
+ end
+ end
+
module ClassMethods
def assertionless_tests_behavior=(behavior)
@assertionless_tests_behavior =
@@ -26,9 +35,9 @@ def assertionless_tests_behavior=(behavior)
ActiveSupport::Logger.new($stderr)
end
- ->(message) { logger.warn(message) }
+ ->(message, test_location, test_name) { logger.warn("#{message}: `#{test_name}` #{test_location}") }
when :raise
- ->(message) { raise Minitest::Assertion, message }
+ ->(message, test_location, *) { raise NoAssertions.new(message, test_location) }
else
raise ArgumentError, "assertionless_tests_behavior must be one of :ignore, :log, or :raise."
end
@@ -42,8 +51,8 @@ def after_teardown
if assertions == 0 && (behavior = ActiveSupport::TestCase.assertionless_tests_behavior)
file, line = self.class.instance_method(name).source_location
- message = "Test is missing assertions: `#{name}` #{file}:#{line}"
- behavior.call(message)
+ test_location = "#{file}:#{line}"
+ behavior.call("Test is missing assertions", test_location, name)
end
end
end
diff --git a/activesupport/test/testing/assertionless_tests_test.rb b/activesupport/test/testing/assertionless_tests_test.rb
index eb7b21285a..90cfbf99df 100644
--- a/activesupport/test/testing/assertionless_tests_test.rb
+++ b/activesupport/test/testing/assertionless_tests_test.rb
@@ -51,10 +51,12 @@ class AssertionlessTestsRaise < ActiveSupport::TestCase
module AfterTeardown
def after_teardown
with_assertionless_tests_behavior(:raise) do
- assert_raise(Minitest::Assertion,
- match: /Test is missing assertions: `test_without_assertions` .+assertionless_tests_test\.rb:\d+/) do
+ e = assert_raise(Minitest::Assertion, match: "Test is missing assertions") do
super
end
+ report = Minitest::Result.from(self)
+ report.failures << e
+ assert_match(/\[test\/testing\/assertionless_tests_test.rb:\d+]/, report.to_s)
end
end
end
As far as I can tell this is all done through public api. It's pretty much only the location
method, which is documented. Sample output:
Failure:
AssertionlessTestsTest::AssertionlessTestsRaise#test_without_assertions [test/testing/assertionless_tests_test.rb:7]:
Test is missing assertions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion but I am not sure these complications are needed and I am not comfortable overriding location
method, even though it is a public method.
Let's see what others will say about this.
I'm generally sympathetic to the idea, I'll just have to take time to review this (this week hopefully), and also think about what the upgrade story need to be. Also even though it's 3rd party project, we'd need to double check whether it breaks |
f6a1fc8
to
1fe871c
Compare
I did not get why this PR is relevant to |
1fe871c
to
39db54c
Compare
I'm not sure about this specific case. But IIRC rspec-rails uses ActiveSupport::TestCase under the hood to have access to various test helpers, and in the past similar changes broke it. PS: I wanted to review this but for some weird reason GitHub isn't accessible from my hotel wifi... but I hope to have a bit of time this week end. |
Made a simple testing with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good, but we probably ought to have this disabled by default for upgrading apps.
So this mean adding the usual load_default 7.2
and new_framework_defaults
, things.
if defined?(Rails.logger) && Rails.logger | ||
Rails.logger |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a big fan of checking for Rails existence here. Might make more sense to introduce ActiveSupport.logger
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is how it is done in another place too
rails/activesupport/lib/active_support/deprecation/behaviors.rb
Lines 25 to 33 in f04c59b
log: ->(message, callstack, deprecator) do | |
logger = | |
if defined?(Rails.logger) && Rails.logger | |
Rails.logger | |
else | |
require "active_support/logger" | |
ActiveSupport::Logger.new($stderr) | |
end | |
logger.warn message |
Maybe introduce that change separately?
else | ||
raise ArgumentError, "assertionless_tests_behavior must be one of :ignore, :log, or :raise." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably YAGNI
, but we could accept any callable here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
I noticed, that .with
helper is working only with objects, and not with modules. So something like
ActiveSupport::TestCase.with(assertionless_tests_behavior: :ignore, &block)
is not working. Was that intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just confirmed it does, I suspect that core_ext just instead loaded?
39db54c
to
3c74e16
Compare
So this mean adding the usual It was disabled by default. I changed it to be |
# This is helpful in detecting broken tests that do not perform intended assertions. | ||
module AssertionlessTests # :nodoc: | ||
def self.prepended(klass) | ||
klass.singleton_class.attr_reader :assertionless_tests_behavior |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should make this a class_attribute
, so that you can change the behavior on a per child class basis. (don't forget not pass instance_accessor: false, instance_predicate: false
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like class_attribute
is not working with overridden setter method, as in our case? 🤔 Am I doing something wrong or there are other ways?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, you can't just override the setter, best is to use class_attirbute :_attr_name
, then from you override set self._attr_name
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, that worked. Ready for review again.
3c74e16
to
73d6d52
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. But the CI neeed some fixing.
73d6d52
to
76966f9
Compare
FYI for anyone who came across this and wondered where the config went: 6a6c7e6 |
I found a few of these in the railties suite, there maybe more elsewhere:
|
Interesting why these were not detected when originally the error was raised. Not related, but I am also seeing |
🤔 That message comes from the Line 9 in 965b8c3
We generate the JUnit test reports for every job and upload them to artifacts: I don't think we're actually using that data to my knowledge, but in theory we could use it with Buildkite's Test Analytics features. For example, we can use that data to find slow or flaky tests and track their performance overtime. |
Inspired by https://railsatscale.com/2024-01-25-catching-assertionless-tests/.
It is very easy to write a "false positive" broken test (as can be seen from the linked article) that actually tests nothing (or can become such in the future). A simple example:
We have some assertions used in the test case, so, looks good? But if we change the scope and it will now return 0 records, this test will still pass and none of the assertions will be run. For this test to be more robust, we need at least to test that
active_users
contains any records.This PR now allows to easily detect such tests.
With this configuration, assertionless tests we will now be marked as failed and not silently pass.
Currently, this feature is configured to
:ignore
such tests by default, but we can rollout it to eventually:log
and then:raise
in the future.Some of such problematic tests were already fixed before, like in #48065.
As a follow up to this PR, I would like to enable
:raise
behavior for all the frameworks. Currently there are a few offences fixing which will make this PR bigger than is needed.cc @nvasilevski @byroot (interested in your thoughts about this)