The constraint looks good. ar_proxy_of(...) for this case? Or is
your constraint specified as making a copy?
Your patch seems to be narrow enough that that is also workable. As
you say, it is Rails that is causing the surprise.
Michael
On Sep 28, 2008, at 11:52 AM, David Chelimsky wrote:
On Sun, Sep 28, 2008 at 1:44 PM, Michael Latta <[EMAIL PROTECTED]> wrote:
Is your patch AR proxy specific? If it is for any collection, it
prevents
two collections from being compared for equality. I have had many
examples
of collections that are not simple containers, and only comparing the
contents would be equally invalid as the simple equality on AR
proxies. If
you added an AR specific method / test that would have less impact
on other
cases, but of course be less clean. That is why I suggested separate
matchers, since I know of cases where your patch would be incorrect.
The patch is only in the rspec-rails plugin, and checks to see if it's
a proxy before doing anything, so it won't affect any other sort of
collection.
As for matching the collection, the comparison is done using == so it
passes or fails if the collections are considered equal according to
ruby's semantics for ==.
What you're proposing could be resolved with an argument constraint
that's been discussed in some other threads on this list - something
like:
lambda {...}.should change{...}.to(array_consisting_of(...))
I'd prefer this as it lets us keep the single matcher, but allows us
to make it more flexible by adding different constraints. We already
have hash_including. This would expand that sort of capability.
WDYT?
Michael
On Sep 28, 2008, at 11:35 AM, David Chelimsky wrote:
On Sun, Sep 28, 2008 at 1:18 PM, Michael Latta <[EMAIL PROTECTED]>
wrote:
David,
It seems to me that the root of the problem is that the
specification is
incorrect. Since Rails returns association proxies the
specification
fails
because it does not specify what the behavior should be. I would
suggest
that instead of patching the change matcher, that you should add a
change_contents matcher that matches the contents of a collection
vs. the
contents of a collection. That way the framework is not guessing
what
was
meant, but relying on the specification to be correct. Since
that is
really
what you want to specify (the contents have changed). I think
this is
cleaner.
I think what you propose would be objectively more explicit, but
"cleanliness" is a very subjective thing.
This is tricky because AR is simultaneously violating the
principle of
least surprise by giving you a proxy instead of the real collection
AND adhering to principles of encapsulation and duck-typing. The
fact
that team.players is a proxy should not matter to its consumer one
way
or the other as long as it can treat it just like the collection of
players.
Now, in the case of what rspec is doing in the change matcher, I
would
say that rspec is getting bitten by the violation of the principle
of
least surprise because the matcher is doing something that AR is not
expecting - further delaying evaluation of AR::A::AP's delayed
evaluation. But I think rspec can deal w/ that and alleviate the
burden on the rspec-rails user to know two different matchers and
when
to use them.
I've already implemented this so it's all transparent for the user:
http://rspec.lighthouseapp.com/projects/5645-rspec/tickets/545.
I'm open to re-opening this but I'd like some feedback from other
people before I do. Anybody else have any opinions?
David
Michael
On Sep 28, 2008, at 11:13 AM, David Chelimsky wrote:
On Sun, Sep 28, 2008 at 11:01 AM, David Chelimsky <[EMAIL PROTECTED]
>
wrote:
On Sun, Sep 28, 2008 at 10:43 AM, David Chelimsky
<[EMAIL PROTECTED]>
wrote:
On Sun, Sep 28, 2008 at 9:47 AM, Ashley Moran
<[EMAIL PROTECTED]> wrote:
Hi
Just had a surprising result:
it "should not appear in the Story.unposted list" do
@story.save
lambda {
@story.post_to_twitter(@twitter_client)
}.should change { Story.unposted }.from([EMAIL PROTECTED]).to([])
end
'Story#post_to_twitter should not appear in the
Story.unposted list'
FAILED
result should have been changed to [], but is now []
Anyone know why this fails? I've looked in change.rb but I
can't
figure it
out.
Whenever I've seen output like "should have been foo, but was
foo" it
has boiled down to AR Assocation Proxies, which don't align in
their
response to == and inspect.
I'm looking at seeing if there's a way we can make "should
change"
work in spite.
Wow.
OK - here's what I figured out. Talk about insidious bugs! This
is
actually quite a bit different from what I thought.
There are two lambdas involved here:
lambda {
1st lambda: expression that should cause the change
}.should change{
2nd lambda: expression that returns the object that should change
}.to(expected value)
The matcher executes the 1st lambda and stores the result as
@before.
In your example this is a Rails association proxy for the
Story.unposted collection.
The matcher then executes the 2nd lambda.
The matcher then executes the 1st lambda again and stores the
result
as @after. In your example, this is, again, a Rails association
proxy
for the Story.unposted collection.
At this point, @before and @after point to the same object -
the same
Rails association proxy!!!!!!
The matcher passes if @before != @after and fails if @before ==
@after. Because they are actually the same association proxy, the
example fails.
Now rspec asks the matcher to print out the reason why it failed,
which does this:
"#{result} should have been changed to [EMAIL PROTECTED], but is now
[EMAIL PROTECTED]"
@to is the expected value []
@after is the association proxy, which proxies to an empty
collection.
Now, when the matcher calls @after.inspect, is the first time
that the
proxy is actually evaluated!!!!
Does this make sense?
I was able to get a similar example to pass by doing this
immediately
after storing the proxy in the @before variable:
@before = @before.collect{|item|item} if @before.respond_to?
(:collect)
Ugly, ugly, ugly. But perhaps necessary to deal w/ this problem.
I think I'll restructure things so the the change matcher
handles this
in rails, but not in core rspec.
Thoughts?
FYI - ticket added and problem resolved:
http://rspec.lighthouseapp.com/projects/5645-rspec/tickets/545
I can make it work with:
should change { Story.unposted.length }.from(1).to(0)
But that's a weaker test.
Thanks
Ashley
--
http://www.patchspace.co.uk/
http://aviewfromafar.net/
_______________________________________________
rspec-users mailing list
rspec-users@rubyforge.org
http://rubyforge.org/mailman/listinfo/rspec-users
_______________________________________________
rspec-users mailing list
rspec-users@rubyforge.org
http://rubyforge.org/mailman/listinfo/rspec-users
_______________________________________________
rspec-users mailing list
rspec-users@rubyforge.org
http://rubyforge.org/mailman/listinfo/rspec-users
_______________________________________________
rspec-users mailing list
rspec-users@rubyforge.org
http://rubyforge.org/mailman/listinfo/rspec-users
_______________________________________________
rspec-users mailing list
rspec-users@rubyforge.org
http://rubyforge.org/mailman/listinfo/rspec-users
_______________________________________________
rspec-users mailing list
rspec-users@rubyforge.org
http://rubyforge.org/mailman/listinfo/rspec-users
