Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Add assertion for checking the contents of a collection, ignoring element order #93

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
4 participants

tkareine commented Feb 9, 2012

No description provided.

tkareine added some commits Feb 9, 2012

Add assert_includes_all
This assertion is commonly needed when testing the contents of a
collection, but not caring about the order of the elements in the
collection.
Owner

zenspider commented Feb 10, 2012

I like the idea (with caveats, see below), but it looks like it only works with arrays, not collections in general. If it is just arrays, then this looks like it would suffice:

assert_empty a - b
assert_empty b - a

caveats: I'm not sure how much clarity this provides.

I have a number of tests that sort before doing an assert_equal, but they're pretty self-descriptive. Eg:

    deps = spec.dependencies.sort_by { |dep| dep.name }

    expected = [
      ["hoe",  :development, "~> #{Hoe::VERSION.sub(/\.\d+$/, '')}"],
      ["rdoc", :development, "~> 3.10"],
    ]

    assert_equal expected, deps.map { |dep|
      [dep.name, dep.type, dep.requirement.to_s]
    }

vs:

    deps = spec.dependencies

    expected = [
      ["hoe",  :development, "~> #{Hoe::VERSION.sub(/\.\d+$/, '')}"],
      ["rdoc", :development, "~> 3.10"],
    ]

    assert_equal_unordered expected, deps.map { |dep|
      [dep.name, dep.type, dep.requirement.to_s]
    }

Which one is clearer? They seem almost exactly the same to me.

Also, I don't like the name. assert_include_all is stunted at the very least. assert_include_all_of seems a bit better, but both of these names suggest the possibility of a superset, not that they're strictly the same except for order. That's why I named it assert_equal_unordered in my example above.

@ghost ghost assigned zenspider Feb 10, 2012

Member

phiggins commented Feb 10, 2012

Also, I don't like the name.

FWIW, shoulda has this called assert_same_elements. I always thought that was a succinct name for this behavior.

I agree that assert_includes_all is not a good name. I don't think assert_same_elements is good either, because minitest already has assert_same, which would imply relationship. I like @zenspider's suggestion (assert_equal_unordered) best so far. I renamed the assertion.

assert_equal_unordered should work with any Enumerable. The assertion converts the collection to an Array for internal operation. Enumerable is guaranteed to convert the collection to Array with to_a. The call to dup is needed to avoid mutating the original collection, if the assertion is called with an Array as the first argument.

The elements of the collection need not implement <=>. I believe this is the most useful feature of the assertion, because a collection with custom elements does not need to be sorted externally before asserting its contents.

Owner

zenspider commented Feb 10, 2012

On Feb 10, 2012, at 00:12 , Tuomas Kareinen wrote:

I agree that assert_includes_all is not a good name. I don't think assert_same_elements is good either, because minitest already has assert_same, which would imply relationship. I like @zenspider's suggestion (assert_equal_unordered) best so far. I renamed the assertion.

assert_equal_unordered should work with any Enumerable. The assertion converts the collection to an Array for internal operation. Enumerable is guaranteed to convert the collection to Array with to_a. The call to dup is needed to avoid mutating the original collection, if the assertion is called with an Array as the first argument.

The elements of the collection need not implement <=>. I believe this is the most useful feature of the assertion, because a collection with custom elements does not need to be sorted externally before asserting its contents.

OK. I overlooked the dup.to_a and misunderstood what this was up to.

(NOTE: I'm tired so this isn't entirely thought out)

I think it'd be better if this section were non-destructive:

     remaining = collection.dup.to_a
     assert remaining.size == elements.size, msg
     elements.each do |e|
       index = remaining.index e
       remaining.delete_at index if index
     end

I think we can do it with just the size check and then ensuring that all elements from one side are in the other:

    assert a.size == b.size, msg
    assert a.all? { |e| b.include? e }

No... OK... After poking at this more and looking at your (great!!) tests I see a lot more of your rationale... Still... I don't like the destructive nature of it (and can't explain why right now). Try this:

      assert_kind_of Enumerable, a
      assert_kind_of Enumerable, b

      c = Hash.new { |h,k| h[k] = 0 }; a.each do |e| c[e] += 1 end
      d = Hash.new { |h,k| h[k] = 0 }; b.each do |e| d[e] += 1 end

      assert c == d, msg

I'll leave it to others to decide which one has more appeal... I'm going to bed.

2012/2/10 Ryan Davis
reply@reply.github.com:

[..] Still... I don't like the destructive nature of it (and can't explain why right now). Try this:

     assert_kind_of Enumerable, a
     assert_kind_of Enumerable, b

     c = Hash.new { |h,k| h[k] = 0 }; a.each do |e| c[e] += 1 end
     d = Hash.new { |h,k| h[k] = 0 }; b.each do |e| d[e] += 1 end

     assert c == d, msg

That works, but it introduces subtle requirement: elements should have
similar behavior for == and eql?. Hash uses eql? to check
whether two keys are equal. That might lead to a surprise in some
cases:

irb(main):004:0> 1 == 1.0
=> true
irb(main):005:0> 1.eql?(1.0)
=> false
Owner

tenderlove commented Feb 10, 2012

This assertion seems really application specific. I can't imagine a common situation where I would need this. Is it really something that should be in minitest?

Could you provide real world examples where this would simplify and clarify the test cases? Maybe there is a different abstraction we need.

Member

phiggins commented Feb 10, 2012

@tenderlove assert_equal expected, actual.sort is a pretty common idiom.

Ruby:

$ grep -R "assert_equal.*\.sort" test/ | wc -l
     150

Jruby (likely some overlap with ruby):

 grep -R "assert_equal.*\.sort" test/ | wc -l
     253

Rails:

$ grep -R "assert_equal.*\.sort" **/test/ | wc -l
     113
Owner

tenderlove commented Feb 10, 2012

@phiggins sure, but is adding a new assertion to minitest worth saving a call to sort?

[aaron@higgins rails (master)]$ grep -R "assert_equal.*\.sort" **/test/ | wc -l
     114
[aaron@higgins rails (master)]$ grep -R "assert_equal" **/test/ | wc -l
   11658
[aaron@higgins rails (master)]$

Less than 1%, and that's only counting calls to assert_equal. I'm still dubious that this is a "common idiom".

EDIT: I forgot to mention that I think adding the explicit sort may make the test more clear. I'm not sure this is common, nor am I sure it makes the tests read more clearly.

Member

phiggins commented Feb 10, 2012

is adding a new assertion to minitest worth saving a call to sort?
Less than 1%

Probably not, when you put it that way.

Owner

tenderlove commented Feb 10, 2012

Oh @phiggins, you cave too easily. :trollface:

Member

phiggins commented Feb 10, 2012

I don't really have horse in this race, just trying to help out. 🍰

Member

phiggins commented Feb 10, 2012

Serious Business

@tenderlove, I think needing to sort a collection for asserting its contents is noise for test readability. Depending on how you get the expected elements, you might have to sort them as well.

It is even worse if the elements cannot be compared with <=>, like the booleans. Then you have to figure out a scheme for calculating a key for each element for ordering. And that scheme depends on the situation.

Owner

zenspider commented Feb 11, 2012

I put a very typical comparative example in my first comment asking how one was clearer than the other and haven't heard a word on it. They're pretty much the same imo. I don't think 1% is a typical average on a project, but it is going to be low... but as far as what clarity this assertion lends? That's still up for debate.

@zenspider, the example in your first comment doesn't get much better with assert_equal_unordered, and I don't think that can be improved.

However, let's presume I have a test class where I want to have the main expectation in one place, and use selected parts of that in the actual assertions. In particular, I want to write the expectation in the order what is shown to the user of the application. This helps maintaining the test. I have modified your original example to reflect this:

require "minitest/autorun"

class AssertEqualUnorderedJustification < MiniTest::Unit::TestCase
  # specified in the order as shown to the user of the application
  EXPECTED_DEPS = [
    ["hoe",  :runtime,     "~> 2.13.1"],
    ["rdoc", :runtime,     "= 3.9"],
    ["rdoc", :runtime,     "= 3.8"],
    ["hoe",  :development, "~> 2.13.3"],
    ["hoe",  :development, "~> 2.13.2"],
    ["hoe",  :development, "~> 2.13.0"],
    ["rdoc", :development, "= 3.12"]
  ]

  def EXPECTED_DEPS.type_of(type); self.select { |_, t| t == type }; end

  def setup
    # read from IO, order not guaranteed
    @deps = [
      ["rdoc", :runtime,     "= 3.9"],
      ["hoe",  :development, "~> 2.13.2"],
      ["hoe",  :development, "~> 2.13.3"],
      ["rdoc", :runtime,     "= 3.8"],
      ["hoe",  :runtime,     "~> 2.13.1"],
      ["hoe",  :development, "~> 2.13.0"],
      ["rdoc", :development, "= 3.12"],
    ]

    def @deps.type_of(type); self.select { |_, t| t == type }; end
  end

  def test_development_dependencies_with_assert_equal
    # requires both calls to #sort
    assert_equal @deps.type_of(:development).sort, EXPECTED_DEPS.type_of(:development).sort
  end

  def test_development_dependencies_with_assert_equal_unordered
    assert_equal_unordered @deps.type_of(:development), EXPECTED_DEPS.type_of(:development)
  end

  def test_runtime_dependencies_with_assert_equal
    # requires both calls to #sort
    assert_equal @deps.type_of(:runtime).sort, EXPECTED_DEPS.type_of(:runtime).sort
  end

  def test_runtime_dependencies_with_assert_equal_unordered
    assert_equal_unordered @deps.type_of(:runtime), EXPECTED_DEPS.type_of(:runtime)
  end
end

Granted, this does not happen often and you can work around it. But I have come across it every now and then, so that I dislike copying the assertion method to every project where I need it.

Any thoughts on this additional assertion and my arguments for it? In short, I think it allows writing tests for checking the contents of a collection easier, without requiring the collection (the actual or the excepted) to be sorted or the elements in it to be Comparable.

Most of the time, you can work around this by simply sorting both collections and using assert_equals. I like minitest to be small, so I understand if the addition is not worth it. What do you think?

Owner

zenspider commented Mar 9, 2012


  def test_assert_equal_unordered_keeps_equality_contract
    @assertion_count = 3

    @tc.assert_equal           [1.0, 2, 3], [1, 2, 3], "WOO"
    @tc.assert_equal_unordered [1.0, 2, 3], [1, 2, 3], "BOO"
  end

I don't like the idea of assert_equal being the contract maker. assert_equal is saying that the two objects respond true to #==. assert_equal_unordered is not saying that and is saying something completely different. This is the only test that fails on my non-destructive version of assert_equal_unordered and at this point I'm not sure whether I should punt on this test or my impl....

Open the debate...

I'd like that assert_equals_unordered would not surprise the user. To me, the name of the assertion relates it to assert_equals, giving the user impression that both assertions behave in a similar way. That's why I'm favoring my original destructive implementation here, because it relies on #== for equality.

That said, I like @zenspider's non-destructive implementation as well. If you choose that, you should document that equality is in terms of #eql?, not #==.

Owner

zenspider commented Mar 13, 2012

On Mar 10, 2012, at 22:56 , Tuomas Kareinen wrote:

I'd like that assert_equals_unordered would not surprise the user. To me, the name of the assertion relates it to assert_equals, giving the user impression that both assertions behave in a similar way.

assert_equal Set.new([1]), Set.new([1.0])

Is the user surprised by this? Maybe... but once they learn how sets work, they shouldn't be. Maybe the method should be renamed... I dunno.

Owner

zenspider commented Mar 13, 2012

OK. After much discussion over here, we've decided to address the "surprise" issue with more documentation or to pull the feature all-together (none of us are convinced that it'll clean up our tests very much). The doco I just finished is:

+    # Fails unless +a+ contains the same contents as +b+, regardless
+    # of order.
+    #
+    #    assert_equal_unordered %w[a a b c], %w[a b c a] # pass
+    #
+    # NOTE: This uses Hash#== to determine collection equivalence, as
+    # such, do not expect it to behave the same as +assert_equal+.
+    #
+    #    assert_equal [1], [1.0]                         # pass
+    #    assert_equal({ 1 => true }, { 1.0 => true })    # fail
+    #    assert_equal_unordered [1], [1.0]               # fail

That's great, thank you! I'm fine with either option you take.

Owner

zenspider commented Mar 20, 2012

I just added seattlerb/minitest-unordered. Please poke at it. In particular, I didn't touch README.txt at all.

@zenspider zenspider closed this Mar 20, 2012

Thanks. I already sent a pull request for updating README.txt.

@zenspider zenspider locked and limited conversation to collaborators May 17, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.