refactor: encapsulate minitest assertion matchers

[G-Rath]

This aims to move all of the specific code for each minitest assertion matcher into the assertion class, so that the outer cop becomes responsible for calling a specific assertion matcher class and reporting, and the inner classes become responsible for handling the actual matching.

Opening as a draft for now to get confirmation that people are ok with the direction of this - I'll squash and do any remaining cleanup afterwards.

---

Before submitting the PR make sure the following are checked:

• Feature branch is up-to-date with master (if not - rebase it).
• Squashed related commits together.
• Added tests.
• Updated documentation.
• Added an entry to the CHANGELOG.md if the new code introduces user-observable changes.
• The build (bundle exec rake) passes (be sure to run this locally, since it may produce updated documentation that you will need to commit).

If you have created a new cop:

• Added the new cop to config/default.yml.
• The cop is configured as Enabled: pending in config/default.yml.
• The cop is configured as Enabled: true in .rubocop.yml.
• The cop documents examples of good and bad code.
• The tests assert both that bad code is reported and that good code is not reported.
• Set VersionAdded: "<<next>>" in default/config.yml.

If you have modified an existing cop's configuration options:

• Set VersionChanged: "<<next>>" in config/default.yml.

[bquorning]

If you add extend NodePattern::Macros to BasicAssertion, it should be possible to add the node matchers in the BasicAssertion subclasses themselves. I’m not 100% sure if it works, but we’d get rid of this loop defining the node patterns, and #on_send would also become a lot simpler, e.g.

def on_send(node)
  ASSERTION_MATCHERS.each do |matcher|
    assertion = matcher.new(node).match
    return if assertion.nil?

    on_assertion(node, assertion)
  end
end

[G-Rath]

ah cool - @pirj made a similar comment over in #1791 but I'd not explored further since I thought it'd require a more complicated change to the constructor, but maybe not.

I'll give that a try but it if turns out too complicated I would prefer to tackle it in a follow-up PR if that's ok

[G-Rath]

hmm yeah this is turning ugly a bit too ugly for my taste: since BasicAssertion is handling a lot of the instance variables, we effectively want an artificial second constructor, and the node matcher method takes a block which makes it messier to do a return (unless I've missed something).

Would you mind sticking with this current layout and I can explore this further as a follow up?

[bquorning]

Sure, we can stick with the current layout.

I tried finding a suitable implementation, and it’s tricky. I ended up with something like this in the subclasses:

            def initialize(node)
              pattern(node) do |expected, actual, failure_message|
                super(expected, actual, failure_message.first)
              end
            end

The “only sometimes call super” smells a bit, and I had to add this method to the BasicAssertion:

            def match?
              !!@expected || !!@actual
            end

to be called instead of next if assertion.nil?:

          def on_send(node)
            ASSERTION_MATCHERS.each do |m|
              assertion = m.new(node)
              on_assertion(node, assertion) if assertion.match?
            end
          end

[G-Rath]

Yeah it's annoying how ugly it gets and I'm pretty sure it's all because of node_matcher not being on the class... 😅

I've got an idea or two on how to approach it though which I'll explore after this is landed.

[pirj]

Thanks!
I hope it will turn out to be possible to eliminate the accumulated meta-programming.

[ydah]

Looks great!