Feature #3219

assert now passes non-boolean result

Added by Nobuyoshi Nakada almost 4 years ago. Updated 2 months ago.

[ruby-core:29868]
Status:Assigned
Priority:Normal
Assignee:Shota Fukumori
Category:-
Target version:next minor

Description

Hi,

Test::Unit::Assertions#assert now passes non-boolean values
(neither true nor false).

It is not only an incompatibility against former TestUnit, also
makes wrong tests (e.g., ) passing.

diff --git a/lib/test/unit/assertions.rb b/lib/test/unit/assertions.rb
index 821faf5..52d5201 100644
--- a/lib/test/unit/assertions.rb
+++ b/lib/test/unit/assertions.rb
@@ -10,6 +10,16 @@ module Test
         obj.pretty_inspect.chomp
       end

+      def assert(result, *args, &b)
+        super(result == true || result == false, "assertion result must be true or false")
+        super
+      end
+
+      def refute(result, *args, &b)
+        super(result == true || result == false, "assertion result must be true or false")
+        super
+      end
+
       def assert_raise(*args, &b)
         assert_raises(*args, &b)
       end

Nobu Nakada

History

#1 Updated by Nobuyoshi Nakada almost 4 years ago

  • Status changed from Open to Closed
  • % Done changed from 0 to 100

=begin
This issue was solved with changeset r27541.
Nobuyoshi, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.

=end

#2 Updated by Nobuyoshi Nakada almost 4 years ago

  • Category set to lib
  • Status changed from Closed to Assigned
  • Assignee set to Yuki Sonoda
  • Priority changed from Low to Normal

=begin

=end

#3 Updated by Nobuyoshi Nakada almost 4 years ago

=begin
Hi,

At Thu, 29 Apr 2010 13:58:24 +0900,
Aaron Patterson wrote in :

I don't think this is right. test/unit on ruby 1.8 allows non-boolean
values to it's assert method. The following test will pass on 1.8:

Exactly. I reverted the commit, but found some tests use
nonsense assertions. Although fixed in test/ruby already, I
suspect there might be more mistakes in other tests.

--
Nobu Nakada

=end

#4 Updated by caleb clausen almost 4 years ago

=begin
It is all too easy to write assert(foo, bar) when you meant to write assert_equal(foo, bar). I have made the same mistake myself a number of times. Usually, no error will result because assert allows an optional second parameter (an alternate String to be printed when the assertion fails).

I'd suggest that a better way to detect this problem is for assert to fail if a non-String is passed as the second parameter. This won't detect all cases of using assert when you meant assert_equal, but it should catch at lest 90%.
=end

#5 Updated by Nobuyoshi Nakada almost 4 years ago

=begin
Hi,

At Fri, 30 Apr 2010 06:15:34 +0900,
Roger Pack wrote in :

I'd suggest that a better way to detect this problem is for
assert to fail if a non-String is passed as the second
parameter. This won't detect all cases of using assert when
you meant assert_equal, but it should catch at lest 90%.

Yeah something like that could work well. When would someone want to
output something that's not a string?

How do you want to show non-string message? Anyway, you can
use a proc too.

--
Nobu Nakada

=end

#6 Updated by Eric Hodel almost 4 years ago

=begin
On Apr 29, 2010, at 09:12, caleb clausen wrote:

Issue #3219 has been updated by caleb clausen.

It is all too easy to write assert(foo, bar) when you meant to write assert_equal(foo, bar). I have made the same mistake myself a number of times. Usually, no error will result because assert allows an optional second parameter (an alternate String to be printed when the assertion fails).

I don't see how you could have this problem if you're following a red, green, refactor type discipline. If the test passed when you forgot to add _equal you should immediately know you did it wrong because it should have failed. How can you trust any of your tests if you haven't ensured they fail when something is wrong?

I'd suggest that a better way to detect this problem is for assert to fail if a non-String is passed as the second parameter. This won't detect all cases of using assert when you meant assert_equal, but it should catch at lest 90%.

Why should I have to call #to_s manually when I can pass an object that will print something useful for me?

$ cat t.rb
require 'minitest/autorun'

class TestAssert < MiniTest::Unit::TestCase

def testassert
o = Object.new
def o.to
s() "something useful" end

 assert false, o

end

end
$ ruby19 t.rb
Loaded suite t
Started
F
Finished in 0.000645 seconds.

1) Failure:
test_assert(TestAssert) [t.rb:9]:
something useful

1 tests, 1 assertions, 1 failures, 0 errors, 0 skips

Test run options: --seed 42229

=end

#7 Updated by Nobuyoshi Nakada almost 4 years ago

=begin
Hi,

At Sat, 1 May 2010 14:30:19 +0900,
Eric Hodel wrote in :

class TestAssert < MiniTest::Unit::TestCase
def testassert
o = Object.new
def o.to
s() "something useful" end
assert false, o
end
end

I can't imagine how it is useful in what situation. Could you
elaborate?

--
Nobu Nakada

=end

#8 Updated by Yuki Sonoda almost 4 years ago

  • Status changed from Assigned to Closed

=begin
I don't want to merge this to ruby 1.9.1
=end

#9 Updated by Nobuyoshi Nakada almost 4 years ago

=begin
Hi,

At Mon, 3 May 2010 11:13:50 +0900,
Yuki Sonoda wrote in :

I don't want to merge this to ruby 1.9.1

Agreed. But I think the fixes for wrong assertions should be
merged.

--
Nobu Nakada

=end

#10 Updated by Yusuke Endoh almost 4 years ago

  • Status changed from Closed to Open

=begin
Hi,

I really agree with nobu. I love this feature.
But this is a feature request. This change should not be included
in 1.9.2, IMO. I move this ticket to Feature tracker.

I prefer warning to failing for compatibility reason.

I know minitest aims to be simple, but it would be good to be more
useful with subtle fix. See Feature #2643 :-)

--
Yusuke Endoh mame@tsg.ne.jp
=end

#11 Updated by Ryan Davis almost 4 years ago

=begin

On Apr 30, 2010, at 23:16 , Nobuyoshi Nakada wrote:

Hi,

At Sat, 1 May 2010 14:30:19 +0900,
Eric Hodel wrote in :

class TestAssert < MiniTest::Unit::TestCase
def testassert
o = Object.new
def o.to
s() "something useful" end
assert false, o
end
end

I can't imagine how it is useful in what situation. Could you
elaborate?

Here is one I just wrote:

root.childNodes.each do |child|
  assert child.path, child
end

I want to know which child object doesn't have a valid path. I almost never use assertion messages, but when I do, they're almost never Strings.

=end

#12 Updated by Shyouhei Urabe over 3 years ago

  • Status changed from Open to Assigned

=begin

=end

#13 Updated by Yusuke Endoh almost 2 years ago

  • Description updated (diff)
  • Assignee changed from Yuki Sonoda to Shota Fukumori

Sorah-san, you can decide this.

Yusuke Endoh mame@tsg.ne.jp

#14 Updated by Xavier Noria almost 2 years ago

Not sure I understand this ticket.

If I write a generic predicate foo? whose return value is undocumented and irrelevant, I should be able to test it like this?

assert obj.foo?

#15 Updated by Nobuyoshi Nakada almost 2 years ago

=begin
fxn, you can use (({assert_send})) instead and will see better message if it fails.

$ ruby -rtest/unit -e 'class X<Test::Unit::TestCase;def testfail; assertsend([3, :even?]); end; end'
Run options:

# Running tests:

[1/1] X#testfail
1) Failure:
test
fail(X) :
Expected 3.even? to return true.
=end

#16 Updated by Xavier Noria almost 2 years ago

Hmmm... if I have

# Returns true if the user has first and last names.
def name_complete?
  @first_name && @last_name
end

I'd like to be able to continue testing that as

assert user.name_complete?

the alternative

assert_send([user, :name_complete?])

seems very obscure to me for testing such an ordinary method. (And since the contract says nothing about the return value, there's no point in testing that if true you get the last name.)

#17 Updated by Xavier Noria almost 2 years ago

In my view those are a kind of predicates that deserve to be tested using a straightforward assertion call. They are common, for example

# Says whether there's a user in the session.
def logged_in?
  @current_user
end

or

# Says whether the request is Ajax.
def xml_http_request?
  @env['HTTP_X_REQUESTED_WITH'] =~ /XMLHttpRequest/i
end

etc.

If true, the former returns a User object, and the latter an integer. But that does not belong to their interfaces and you don't test it. You only test them as predicates.

#18 Updated by Yusuke Endoh over 1 year ago

  • Target version set to next minor

#19 Updated by Nobuyoshi Nakada 2 months ago

  • Description updated (diff)

Also available in: Atom PDF