solnic/virtus

Specs are failing

jpmoral opened this issue · 7 comments

Several specs are failing with
undefined method 'first' for #<Virtus::Attribute>

This seems to be because of this change in the equalizer gem:
old:

def ==(other)
  other = coerce(other) if respond_to?(:coerce, true)
  other.is_a?(self.class) && cmp?(__method__, other)
end

new:

def ==(other)
  other = coerce(other).first if respond_to?(:coerce, true) # added call to 'first'
  other.is_a?(self.class) && cmp?(__method__, other)
end

The spec trips up on trying to compare Virtus::Attributes, which don't have a first method defined.
I'd be happy to attempt a fix if I knew more about each project.

We need our own Attribute#== method. Equalizer calls #coerce internally which in case of virtus attributes doesn't do the same thing as, let's say, Float#coerce. This should fix the specs and the behavior of equality method too.

Is it as simple as going back to not using the equalizer gem?

On a side note, changing how the specs are written from:

expect { subject }.to change { object.reset.to_a }.from(attributes).to([ attribute ])

to:

expect(object.reset.to_a).to eq attributes
subject
expect(object.reset.to_a).to eq [attribute]

gets them passing. It's much more difficult to understand though.

We're discussing how to solve this on the Equalizer side, see related issue.

So I tried to look into it and it seems like we can implement a simple own matcher for comparing Virtus::Attribute models (for Attribute model type and option fields matching), but I thought I would wait for some movement on Equalizer side. Are you working on it already? If not, I would love to discuss it and help.
Actually, I implemented Attribute#== method already - probably I can open a PR and we can merge if you like it, at least specs will be passing now (and some related bugs will be fixed I guess, since there should be some unreported ones related to that, if specs are failing). After that we can either wait for Equalizer fix or start using Virtus's own matchers everywhere (this will also remove the dependency, but that's a pretty good amount of potentially unnecessary work). Tell me your thoughts please, and nice project there!

@novikserg you may want to check out this thread: dkubb/equalizer#18 I'm not sure how to proceed with this, currently leaning towards going back to the built-in equalizer that was deprecated in 1.0.0 but now for pragmatic reasons I feel like it could be brought back (esp. that it would remove the need for values .. end block in value_object extension.

@solnic Alright, thanks for the info, I will send a PR in the following week or two.

Specs are fixed, closing.