icy-arctic-fox / spectator

Feature-rich testing framework for Crystal inspired by RSpec.
https://gitlab.com/arctic-fox/spectator
MIT License
101 stars 4 forks source link

Code is untestable if method name is `description` #13

Closed UrsaDK closed 3 years ago

UrsaDK commented 3 years ago

A good example is worth 1000 words, so here is how to recreate the problem:

main.cr:

module A
  def description
    "test method"
  end
end

main_spec.cr:

require "spectator"
require "../src/*"

Spectator.describe A do
  it { is_expected.to respond_to(:description) }
end

Running crystal spec with spectator 0.9.24 throws the following error:

Showing last frame. Use --error-trace for full trace.

In lib/spectator/src/spectator/matchers/respond_matcher.cr:22:25

 22 | FailedMatchData.new(description, "#{actual.label} does not respond to #{label}", **values(snapshot))
                      ^--
Error: no overload matches 'Spectator::Matchers::FailedMatchData.new' with types String, String, description: String

Overloads are:
 - Spectator::Matchers::FailedMatchData.new(description, failure_message, values)
 - Spectator::Matchers::FailedMatchData.new(description, failure_message, **values)

Based on a very brief look, I think the problem is in failed_match_data.cr#L23:

def initialize(description, @failure_message, **values)

From what I can gather, when we test a method that happens to be called description then the key supplied as part of **values overrides identically named method parameter. Unfortunately, I'm not familiar enough with the codebase to propose a solution or help with a PR.

icy-arctic-fox commented 3 years ago

It appears this affected the respond_to, have_attributes, be_xxx, and have_xxx matchers. Those have all been fixed. The issue was that description was the name of another argument in the parameter list, so when it got splatted, it looked like this:

FailedMatchData.new(description, "foo", description: true)

The splat operation has been removed. This will be in 0.9.25, pushing out now. You happened to catch me while I was working on Spectator. :smile:

Also, I don't know if this is just from making a code snippet, but the code:

expect(A).to respond_to(:description)

will fail the test case because A#description is an instance method. So technically A won't respond to description, but anything that includes it will. To make the code pass the test, A should be:

module A
  extend self # <--- Add this

  # ...
end
UrsaDK commented 3 years ago

Thank you so much for fixing this so quickly!

And yes, you're absolutely right about the code snippet. I made it on the fly, based on my existing code and simply forgot to include extend self. My priority was to highlight the issue. Having a passing test would have been nice, but I'm glad that a failing one demonstrated the issue just as well. :)