Skip to content
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

Add IncludesExactly matcher #674

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mohammednasser-32
Copy link

fix #673

add new matcher #includes_exactly which works in a similar way like #includes..however each param should be matched once
ie: checks if two arrays are identical regardless of their order, but it also works with Hash, String and matchers similar to #includes
example:

    matcher = includes_exactly(:x, :y,)
    matcher.matches?([[:y, :x]]) #true
    matcher.matches?([[:y, :x, :z]]) #false
    matcher.matches?([[:y, :y, :z]]) #false

@floehopper
Copy link
Member

@mohammednasser-32 Many thanks for submitting this - at first glance it looks like you've been very thorough with the tests and documentation, so thank you for that! I'll try to find some time soon to review it properly and get it merged and released.

@mohammednasser-32
Copy link
Author

@floehopper Thank you so much 🙏 once the approach is validated I can do the same for has_exact_entries

@floehopper
Copy link
Member

@mohammednasser-32

Thanks again for your work on this. I wanted to let you know that I have started to review the PR. However, I noticed that the implementation of IncludesExactly#matches? does not use the #include? predicate method, which seems a bit surprising to me. Was that intentional or just a natural result of trying to get all the tests passing?

Also I'm wondering why we can't configure the existing Includes matcher with an optional exact argument like we do for HasEntries:

def initialize(entries, exact: false)

In fact I have had an initial attempt at using this approach locally and my implementation is passing all but two of the unit tests. The failing tests have made me wonder a bit about the semantics of this matcher and what the least surprising behaviour would be. I apologise that I hadn't thought this through further before I gave you the green light to work on it.

Unfortunately I've run out of time to look at it today, but I'll have a think about it overnight and take another look tomorrow. I haven't yet had a close look at the examples you've included in the YARD docs for the new matcher, so perhaps they will clarify some things for me.

@mohammednasser-32
Copy link
Author

@floehopper
Thank you so much for your precise review 🙏
It was intentional not to use #include? as an attempt to save an extra loop.

The goal of the matcher is to check if two objects that respond to include are identical regardless of the order, so my first approach to achieve that was:

@items.each do |item| #loop 1
  if parameters.include?(item) #loop 2
    item_index = parameters.index(item) #loop 3
    parameters.delete_at(item_index) #I am deleting by index here so I only delete the first occurrence
  end
end

 parameters.empty? #if the two arrays are identical, we should have deleted all occurances

so as an attempt to improve that I changed it to

  @items.each do |item| #loop1
      matched_index = parameters.each_index.find { |i| item.to_matcher.matches?([parameters[i]]) } #loop2 
      #now we are checking inclusion and getting index in the same loop
      return false unless matched_index
  
      parameters.delete_at(matched_index)
  end

  parameters.empty?

I share the same doubts of the precise expectations of the matcher, if it is supposed to support only arrays/strings/hashes (which was my first impression)..we could just sort them and check for equality..however I tried to make it as close as possible to Includes matcher by supporting matchers and regex too, and that`s why I went with that approach.

@floehopper
Copy link
Member

@mohammednasser-32

Thanks for explaining how you came up with your solution - it was very helpful.

Having thought about it afresh this morning and discussed it with a colleague, I'm not convinced that an exact matcher relating to the #includes? predicate makes complete sense - mainly because of the String case.

So for now, I don't think I want to accept this PR. I apologise again for not fully thinking it all through before encouraging you to do the work. I need to give it some more thought and unfortunately I don't have loads of time at the moment...

Thinking back to your original motivation in #673, did you know you can pass a block to Expectation#with to effectively make a custom matcher? I know this isn't perfect, but it might unblock your current problem...

require "bundler/setup"
require "minitest/autorun"
require "mocha/minitest"

class FooTest < Minitest::Test
  def test_matches
    object = mock("object")
    object.expects(:foo).with(&matches_set([1,2]))
    object.foo([2,1])
  end

  def test_does_not_match
    object = mock("object")
    object.expects(:foo).with(&matches_set([1,2]))
    object.foo([2,1,3])
  end

  private

  def matches_set(expected)
    -> actual { expected.to_set == actual.to_set }
  end
end

@mohammednasser-32
Copy link
Author

Hey @floehopper , sure no worries..and thanks for your suggestion! 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

introduce #has_all parameter matcher
2 participants