Feature #3219: assert now passes non-boolean result

• Status: Rejected
• Assignee: sorah (Sorah Fukumori)
• Target version: 2.6

[nobu (Nobuyoshi Nakada)]

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., [ruby-core:29861]) 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

[nobu (Nobuyoshi Nakada)]

• 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

[nobu (Nobuyoshi Nakada)]

• Category set to lib
• Status changed from Closed to Assigned
• Assignee set to yugui (Yuki Sonoda)
• Priority changed from 3 to Normal

=begin

=end

[nobu (Nobuyoshi Nakada)]

=begin
Hi,

At Thu, 29 Apr 2010 13:58:24 +0900,
Aaron Patterson wrote in [ruby-core:29872]:

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

[coatl (caleb clausen)]

=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

[nobu (Nobuyoshi Nakada)]

=begin
Hi,

At Fri, 30 Apr 2010 06:15:34 +0900,
Roger Pack wrote in [ruby-core:29890]:

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

[drbrain (Eric Hodel)]

=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 test_assert
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

[nobu (Nobuyoshi Nakada)]

=begin
Hi,

At Sat, 1 May 2010 14:30:19 +0900,
Eric Hodel wrote in [ruby-core:29910]:

class TestAssert < MiniTest::Unit::TestCase
def test_assert
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

[yugui (Yuki Sonoda)]

• Status changed from Assigned to Closed

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

[nobu (Nobuyoshi Nakada)]

=begin
Hi,

At Mon, 3 May 2010 11:13:50 +0900,
Yuki Sonoda wrote in [ruby-core:29945]:

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

[mame (Yusuke Endoh)]

• 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 [e-mail]
=end

[zenspider (Ryan Davis)]

=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 [ruby-core:29910]:

class TestAssert < MiniTest::Unit::TestCase
def test_assert
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

[shyouhei (Shyouhei Urabe)]

• Status changed from Open to Assigned

=begin

=end

[mame (Yusuke Endoh)]

• Description updated (diff)
• Assignee changed from yugui (Yuki Sonoda) to sorah (Sorah Fukumori)

Sorah-san, you can decide this.

--
Yusuke Endoh [e-mail]

[fxn (Xavier Noria)]

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?

[nobu (Nobuyoshi Nakada)]

=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 test_fail; assert_send([3, :even?]); end; end'
Run options:

Running tests:

[1/1] X#test_fail

1. Failure:
   test_fail(X) [-e:1]:
   Expected 3.even? to return true.
   =end

[fxn (Xavier Noria)]

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.)

[fxn (Xavier Noria)]

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.

[mame (Yusuke Endoh)]

• Target version set to 2.6

[nobu (Nobuyoshi Nakada)]

• Description updated (diff)

[mame (Yusuke Endoh)]

• Status changed from Assigned to Rejected

I don't remember this ticket, but I guess this request should be handled in the upstream of test/unit. (Or, if you want to change test/lib/minitest, please change freely.)