chain in a matcher keeps instance variables on other asserts #104

Closed
lunks opened this Issue Jan 6, 2012 · 22 comments

Comments

Projects
None yet
6 participants

lunks commented Jan 6, 2012

I've got a matcher for enumerations using enumerate it like this:

module ActiveRecordMatchers

  RSpec::Matchers.define :have_enumeration_for do |enum|

    chain :with do |custom_class|
      @custom_class = custom_class
    end

    match do |subject|
      if @custom_class
        @enum_class = @custom_class
      else
        @enum_class = enum.to_s.camelize.constantize
      end
      subject.class.enumerations[enum.to_sym].should == @enum_class
    end

    failure_message_for_should do |subject|
      "#{subject.class} should have an enumeration on #{enum}. Enumerations: #{subject.class.enumerations.inspect}"
    end

    description do |enum|
      "have an enum for #{enum}"
    end
  end
end

On 2.8.0.rc1 and before, this work like intended:

  it { should have_enumeration_for(:capacity).with(ChipCapacity) } # @enum_class is ChipCapacity
  it { should have_enumeration_for(:state) } #@enum_class is State

After 2.8.0.rc1, the behavior has changed:

  it { should have_enumeration_for(:capacity).with(ChipCapacity) } # @enum_class is ChipCapacity
  it { should have_enumeration_for(:state) } #@enum_class is ChipCapacity

Is this a bug? Is this intended?

Owner

dchelimsky commented Jan 6, 2012

It's a bug introduced by d3e5310. Corey and I actually talked about this issue but it fell off the radar. Basically, we need to clear out any user-set ivars within the matcher.

lunks commented Jan 7, 2012

Thanks for the quick update!

Contributor

alindeman commented Jan 9, 2012

Yah, seeing this one as well :/

lunks commented Jan 9, 2012

If you want the latest version possible, use 2.8.0.rc1, which is not
affected by this bug.

On Mon, Jan 9, 2012 at 01:24, Andy Lindeman <
reply@reply.github.com

wrote:

Yah, seeing this one as well :/


Reply to this email directly or view it on GitHub:
#104 (comment)

Contributor

alindeman commented Jan 9, 2012

Basically, we need to clear out any user-set ivars within the matcher.

Like matcher.instance_variables.each { |ivar| matcher.remove_instance_variable(ivar) } ?

nbibler commented Jan 10, 2012

Hrm.. looks like this is still an issue in 2.8.0 proper.

RSpec::Matchers.define(:my_special_matcher) do |...|
  # ...
  chain :with_title do |title|
    @title = title
  end
end

The @title is still maintained between separate specs, thus failing randomly. While executing a single line of the spec passes without issue.

Owner

dchelimsky commented Jan 10, 2012

@nbibler yes - it was reported after we had already released 2.8.

nbibler commented Jan 10, 2012

Ahh .. got it. I was a little confused by @lunks comment about 2.8.0.rc1 not being affected. I'm guessing that means in rc1 and prior it's not an issue. Thanks for the clarification!

Owner

dchelimsky commented Jan 12, 2012

The issue is really with any user-defined instance variables, not just those defined in chain. Fix coming shortly.

Any news when this fix will be released?

thanks

Can this not be released immediately in 2.8.1, rather than waiting for the next feature release? It is only a bugfix, after-all.

Owner

dchelimsky commented Mar 13, 2012

@benlangfeld I released 2.9.0.rc2 yesterday, which includes this fix.

I'd prefer not to depend on a pre-release.

Enviado via iPad

Em 13 Mar 2012, às 23:16, David Chelimsky
reply@reply.github.com
escreveu:

@benlangfeld I released 2.9.0.rc2 yesterday, which includes this fix.


Reply to this email directly or view it on GitHub:
#104 (comment)

lunks commented Mar 14, 2012

Your best bet is to clone the repository and cherry-pick the commit that
fix this for 2.8.0.

On Wed, Mar 14, 2012 at 05:40, Ben Langfeld <
reply@reply.github.com

wrote:

I'd prefer not to depend on a pre-release.

Enviado via iPad

Em 13 Mar 2012, às 23:16, David Chelimsky
reply@reply.github.com
escreveu:

@benlangfeld I released 2.9.0.rc2 yesterday, which includes this fix.


Reply to this email directly or view it on GitHub:

#104 (comment)


Reply to this email directly or view it on GitHub:
#104 (comment)

So now a fix for a bug that has existed for 4 months being included in the next feature release is semver compliant, and the recommendation is to roll a forked release? That's a very poor approach to quality, especially for such a major bug. I must say, I cannot understand the logic.

Owner

dchelimsky commented Mar 14, 2012

@benlangfeld while I appreciate where you're coming from a) I have limited time b) nobody in the community is stepping up to offer help in managing releases c) it's open source, so your hands are not tied. Going forward I will try to adopt a policy of getting bug fix releases out faster, but there is a cost to this and, in this case, in light of the fact that 2.9.0.rc2 is already out I'm not going to take the time to get a 2.8.1 out unless it looks like a 2.9 final release is going to get pushed back beyond a week or two.

lunks commented Mar 14, 2012

As David said, it's open source. There is nothing wrong with forking it and
adding what you need to it.

On Wed, Mar 14, 2012 at 09:36, David Chelimsky <
reply@reply.github.com

wrote:

@benlangfeld while I appreciate where you're coming from a) I have limited
time b) nobody in the community is stepping up to offer help in managing
releases c) it's open source, so your hands are not tied. Going forward I
will try to adopt a policy of getting bug fix releases out faster, but
there is a cost to this and, in this case, in light of the fact that
2.9.0.rc2 is already out I'm not going to take the time to get a 2.8.1 out
unless it looks like a 2.9 final release is going to get pushed back beyond
a week or two.


Reply to this email directly or view it on GitHub:
#104 (comment)

If assistance with rolling releases is what's required, I'm more than happy to contribute. Let me know what's needed.

Owner

dchelimsky commented Mar 14, 2012

@benlangfeld now you're talking. I'll follow up with you privately.

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