Name collision - how would you handle this?

Hey all,

It turns out that if you have

  • Rails (2 or 3)
  • Ruby-1.9
  • a model named Message
  • let(:message) or def message in an example group
  • a Rails assertion in an example in that group
    • note that rspec-rails’ matchers delegate to Rails’ assertions

You’ll get an error saying “wrong number of arguments (1 for 0)”

This is because the rails assertion, which, when running with Ruby-1.9,
delegates to Minitest::Assertions#assert_block, which delegates to a
message() method that it defines. So the message() method defined by
let() overrides the message() method in the Assertions module, and
results in unexpected and undesirable outcomes.

So - what should we do? I don’t think changing Minitest is really an
option, as too many assertion libraries already wrap Minitest
assertions. I don’t think RSpec should be in the business of monitoring
methods end-users define to make sure they’re not overriding
pre-existing methods (what if you override a method intentionally?). The
only thing I’m left with is document this particular case and hope for
the best, but that feels unsatisfactory as well.

Recommendations? Words of wisdom?

Cheers,
David

On 7 Aug 2010, at 22:10, David C. wrote:

So - what should we do? I don’t think changing Minitest is really an option, as too many assertion libraries already wrap Minitest assertions. I don’t think RSpec should be in the business of monitoring methods end-users define to make sure they’re not overriding pre-existing methods (what if you override a method intentionally?). The only thing I’m left with is document this particular case and hope for the best, but that feels unsatisfactory as well.

While I fully agree if you def a method that already exists, you
should be expected to deal with it yourself (that’s just the way things
are in Ruby), does the same apply to let? I can actually see an
argument that you should only be able to let a method that doesn’t
already exist, and also only do it once (which is just a consequence of
not being able to override a method, given the current implementation).

Can you think of any downsides of preventing RSpec users from overriding
existing methods with let? Are there any popular names already taken?
Or other problems?

To me, let is magic. I don’t think of it, first and foremost, of
defining a method. I see the things it creates as more like local
variables, and just remind myself that they’re methods if I wonder why
it works.

I’m not sold either way on this, but I think it’s one worth a debate.

Ash


http://www.patchspace.co.uk/
http://www.linkedin.com/in/ashleymoran

On Aug 7, 2010, at 4:37 PM, Ashley M. wrote:

On 7 Aug 2010, at 22:10, David C. wrote:

So - what should we do? I don’t think changing Minitest is really an option, as too many assertion libraries already wrap Minitest assertions. I don’t think RSpec should be in the business of monitoring methods end-users define to make sure they’re not overriding pre-existing methods (what if you override a method intentionally?). The only thing I’m left with is document this particular case and hope for the best, but that feels unsatisfactory as well.

While I fully agree if you def a method that already exists, you should be expected to deal with it yourself (that’s just the way things are in Ruby), does the same apply to let? I can actually see an argument that you should only be able to let a method that doesn’t already exist, and also only do it once (which is just a consequence of not being able to override a method, given the current implementation).

Can you think of any downsides of preventing RSpec users from overriding existing methods with let?

Yes. Let’s say I write a shared example group with this:

def foo
raise “you need to define a foo method in the block passed to
it_should_behave_like”
end

And you override it using let(:foo), which would be a perfectly
reasonable way to handle it. In fact, it would be the way I would handle
in instinctively, because now I don’t have to wrote my own memoization
handling into the method.

Are there any popular names already taken? Or other problems?

To me, let is magic. I don’t think of it, first and foremost, of defining a method.

From the RDoc: Generates a method whose return value is memoized after the first call.

I see the things it creates as more like local variables, and just remind myself that they’re methods if I wonder why it works.

I’m not sold either way on this, but I think it’s one worth a debate.

The debate is on! Any other opinions out there?

On 7 Aug 2010, at 23:18, David C. wrote:

Yes. Let’s say I write a shared example group with this:

def foo
raise “you need to define a foo method in the block passed to it_should_behave_like”
end

And you override it using let(:foo), which would be a perfectly reasonable way to handle it. In fact, it would be the way I would handle in instinctively, because now I don’t have to wrote my own memoization handling into the method.

I instinctively agree with ashley, but I see your point too David.

Would it be awful to make let even more magic, and do something with
#caller to forward the message to MiniTest if it didn’t come from the
code in your example block? Maybe the method defined by let could even
have a __hidden name, and then RSpec can forward the message to that
__hidden method if the message was sent from within the example block.

Sounds pretty horrible, doesn’t it?

The debate is on! Any other opinions out there?

Ash


rspec-users mailing list
[email protected]
http://rubyforge.org/mailman/listinfo/rspec-users

cheers,
Matt

http://blog.mattwynne.net
+44(0)7974 430184

On 8 Aug 2010, at 12:05, Matt W. wrote:

And you override it using let(:foo), which would be a perfectly reasonable way to handle it. In fact, it would be the way I would handle in instinctively, because now I don’t have to wrote my own memoization handling into the method.

I instinctively agree with ashley, but I see your point too David.

Would it be awful to make let even more magic, and do something with #caller to forward the message to MiniTest if it didn’t come from the code in your example block? Maybe the method defined by let could even have a __hidden name, and then RSpec can forward the message to that __hidden method if the message was sent from within the example block.

Sounds pretty horrible, doesn’t it?

Hmmm, sounds like it could create a pretty nasty coupling to MiniTest.
Maybe there’s a general solution like:

define_example_method do # or maybe “define_helper” ?
# some stuff that only gets called from examples
end

I’m not sure I’d be keen on let embedding this magic, but maybe as a
general concept it makes more sense, as a way of isolating helper code
in example groups.

There’s another side to the debate, which is that in shared example
groups, I prefer the precondition-check style to the templatemethod-fail
style, ie rather than:

def foo
raise “you need to define a foo method …”
end

I’d prefer to write:

unless respond_to?(:foo)
raise “you need to define a foo method …”
end

But that would involve evaluating the configuration block first >:)

Ash


http://www.patchspace.co.uk/
http://www.linkedin.com/in/ashleymoran

On Aug 7, 2010, at 4:10 PM, David C. wrote:

You’ll get an error saying “wrong number of arguments (1 for 0)”

This is because the rails assertion, which, when running with Ruby-1.9, delegates to Minitest::Assertions#assert_block, which delegates to a message() method that it defines. So the message() method defined by let() overrides the message() method in the Assertions module, and results in unexpected and undesirable outcomes.

So - what should we do? I don’t think changing Minitest is really an option, as too many assertion libraries already wrap Minitest assertions. I don’t think RSpec should be in the business of monitoring methods end-users define to make sure they’re not overriding pre-existing methods (what if you override a method intentionally?). The only thing I’m left with is document this particular case and hope for the best, but that feels unsatisfactory as well.

Recommendations? Words of wisdom?

FYI - here’s the issue that spawned this thread:
http://github.com/rspec/rspec-rails/issues/152

On 8 Aug 2010, at 16:38, David C. wrote:

  • a Rails assertion in an example in that group
    FYI - here’s the issue that spawned this thread: http://github.com/rspec/rspec-rails/issues/152
    Can you use the Assertions module some other way than mixing it into the
    example (thereby polluting it with the Assertions module’s methods?)

rspec-users mailing list
[email protected]
http://rubyforge.org/mailman/listinfo/rspec-users

cheers,
Matt

http://blog.mattwynne.net
+44(0)7974 430184

On Aug 8, 2010, at 8:05 AM, Ashley M. wrote:

On 8 Aug 2010, at 12:05, Matt W. wrote:

And you override it using let(:foo), which would be a perfectly reasonable way to handle it. In fact, it would be the way I would handle in instinctively, because now I don’t have to wrote my own memoization handling into the method.

I instinctively agree with ashley, but I see your point too David.

Would it be awful to make let even more magic, and do something with #caller to forward the message to MiniTest if it didn’t come from the code in your example block? Maybe the method defined by let could even have a __hidden name, and then RSpec can forward the message to that __hidden method if the message was sent from within the example block.

Sounds pretty horrible, doesn’t it?

Yes! It seems crazy to me to put this burden on RSpec, or any downstream
library. The problem here stems from the fact that Minitest consumed a
non-testing-domain-specific name here. Test::Unit uses build_message,
which is still sort of generic, but at least it’s a verb. I submitted a
request to change message to build_message [1], but even if Ryan agrees
to do it, we wouldn’t likely see it in ruby-1.9.2, so we’d still have
the conflict for some time.

Also, let’s say he does agree - now people who like to prefix their
test-data-building-helper-methods with build_xxx will be screwed too :slight_smile:

I wonder if the right answer is for RSpec to implement its own base
assertion library, matching the Test::Unit/Minitest assertion APIs. That
would have an interesting side effect in that domain-specific
assertion-libraries could be used in RSpec without having to depend on
Test::Unit or Minitest. It would also allow folks who like RSpec’s
structure (example groups and examples) but prefer assert_xxx over
RSpec’s matcher to use RSpec the way they’d like without the additional
dependency.

Thoughts on this? Really just an idea. I’m fairly convinced it’s a bad
one, but my head is sort of spinning over this issue right now (or maybe
it’s the cocktails at http://thedrchicago.com/), so I figured I’d throw
it against the wall and see if it sticks (think spaghetti, or Jackson
Pollock).

Hmmm, sounds like it could create a pretty nasty coupling to MiniTest. Maybe there’s a general solution like:

define_example_method do # or maybe “define_helper” ?

some stuff that only gets called from examples

end

I’m not sure I’d be keen on let embedding this magic, but maybe as a general concept it makes more sense, as a way of isolating helper code in example groups.

This suggests, to me, “don’t use Ruby if you want things to work right.”

end

But that would involve evaluating the configuration block first >:)

Keep in mind that it_should_behave_like generates a nested group, so
shared group authors can do this:

shared_examples_for “bar” do
unless respond_to?(:foo)
raise “gotta have foo”
end
end

and host group authors can do this:

describe “thing” do
let(:foo) { Foo.new }
it_should_behave_like “bar”
end

That’s part of the beauty of eval’ing the conditions block last - we get
the best of both worlds.

Cheers,
David

[1]
http://rubyforge.org/tracker/index.php?func=detail&aid=28457&group_id=1040&atid=4097

On 8 Aug 2010, at 16:53, David C. wrote:

  • a model named Message
    Recommendations? Words of wisdom?

FYI - here’s the issue that spawned this thread: http://github.com/rspec/rspec-rails/issues/152

Can you use the Assertions module some other way than mixing it into the example (thereby polluting it with the Assertions module’s methods?)

I like the idea in the abstract, but most of the rails assertions rely on some state that is local to the example (@response, @controller, @request, etc, etc). RSpec could gather up all those instance variables and pass them into an assertion-wrapper object, but then it would be highly coupled to that implementation and would lead us down a familiar and unfriendly path of forcing rspec-rails releases for every rails release. That’s a world I hope to leave behind with Rails 3 :slight_smile:

So leave the rails assertions mixed into the example, but forward all
the calls to the MiniTest::Assertions methods to some other object that
has them mixed in. Won’t that work?


rspec-users mailing list
[email protected]
http://rubyforge.org/mailman/listinfo/rspec-users

cheers,
Matt

http://blog.mattwynne.net
+44(0)7974 430184

On Aug 8, 2010, at 10:40 AM, Matt W. wrote:

  • let(:message) or def message in an example group

FYI - here’s the issue that spawned this thread: http://github.com/rspec/rspec-rails/issues/152

Can you use the Assertions module some other way than mixing it into the example (thereby polluting it with the Assertions module’s methods?)

I like the idea in the abstract, but most of the rails assertions rely
on some state that is local to the example (@response, @controller,
@request, etc, etc). RSpec could gather up all those instance
variables and pass them into an assertion-wrapper object, but then it
would be highly coupled to that implementation and would lead us down a
familiar and unfriendly path of forcing rspec-rails releases for every
rails release. That’s a world I hope to leave behind with Rails 3 :slight_smile:

It would also eliminate the option to use the Rails assertions directly
in examples.

Oh, well :slight_smile:

On Aug 8, 2010, at 11:13 AM, Matt W. wrote:

So leave the rails assertions mixed into the example, but forward all the calls to the MiniTest::Assertions methods to some other object that has them mixed in. Won’t that work?
Here’s a prototype implementation:
http://github.com/rspec/rspec-rails/commit/0cd384536cf532435ec8f290a9c357b60872acd7

It’s on a branch
(http://github.com/rspec/rspec-rails/tree/assertion-delegate) because
I’m not convinced this is the right way to go yet, but I’d like some
feedback from anyone who wishes to peruse and comment.

Thanks,
David

Good error messages are important to a library’s usability. Could we
find away to give the user a good error message when they override a
“reserved method”?

I’m thinking this could be accomplished with 2 simple pieces:

  1. A method_added hook in Rspec-core that gives a warning or error
    when a reserved method is overridden.
  2. An API that allows libraries to add to the reserved methods list.

That way, rspec doesn’t have to have knowledge of other libraries; it
would be the responsibility of other libraries to add their reserved
methods to the list.

Myron

On Aug 8, 2010, at 11:58 AM, Myron M. wrote:

That way, rspec doesn’t have to have knowledge of other libraries; it
would be the responsibility of other libraries to add their reserved
methods to the list.

Yehuda K. made a similar suggestion to me, referencing some code from
merb:
http://github.com/merb/merb/blob/master/merb-core/lib/merb-core/controller/merb_controller.rb#L508

Merb also has an override! method that end users can use to override the
registered reserved methods, which I agree would be a necessary part of
this. The idea being that any user that does that does so explicitly and
knowingly.

The blacklist comment probably wouldn’t work for upstream libs like
Rails, Test::Unit or MiniUnit. It would be up to RSpec to define those
lists. But maybe that’s an acceptable tradeoff. WDYT?

On 9 Aug 2010, at 01:54, David C. wrote:

Hey all,
You’ll get an error saying “wrong number of arguments (1 for 0)”

I like the idea in the abstract, but most of the rails assertions rely on some state that is local to the example (@response, @controller, @request, etc, etc). RSpec could gather up all those instance variables and pass them into an assertion-wrapper object, but then it would be highly coupled to that implementation and would lead us down a familiar and unfriendly path of forcing rspec-rails releases for every rails release. That’s a world I hope to leave behind with Rails 3 :slight_smile:

So leave the rails assertions mixed into the example, but forward all the calls to the MiniTest::Assertions methods to some other object that has them mixed in. Won’t that work?

Here’s a prototype implementation: http://github.com/rspec/rspec-rails/commit/0cd384536cf532435ec8f290a9c357b60872acd7

It’s on a branch (http://github.com/rspec/rspec-rails/tree/assertion-delegate) because I’m not convinced this is the right way to go yet, but I’d like some feedback from anyone who wishes to peruse and comment.

Yeah, that’s what I was talking about. Couple of thoughts / questions:

I’m still not clear why you need to copy the instance variable over
though - do the rails assertions get monkey-patched into the
Test::Unit::Assertions module then?
Also, how come there’s nothing in the specs about the #message method
that caused all this?

[email protected]
http://rubyforge.org/mailman/listinfo/rspec-users

cheers,
Matt

http://blog.mattwynne.net
+44(0)7974 430184

On Aug 9, 2010, at 6:37 AM, Matt W. wrote:

On 8 Aug 2010, at 16:38, David C. wrote:

Yeah, that’s what I was talking about. Couple of thoughts / questions:

I’m still not clear why you need to copy the instance variable over though - do the rails assertions get monkey-patched into the Test::Unit::Assertions module then?

No - holdover from exploratory session.

Also, how come there’s nothing in the specs about the #message method that caused all this?

Good point.

New patch:
http://github.com/rspec/rspec-rails/commit/86600313462638e7becc726e53f1bc67af108667

On 9 Aug 2010, at 13:04, David C. wrote:

On 8 Aug 2010, at 16:53, David C. wrote:

  • Ruby-1.9

Here’s a prototype implementation: http://github.com/rspec/rspec-rails/commit/0cd384536cf532435ec8f290a9c357b60872acd7

Good point.

New patch: http://github.com/rspec/rspec-rails/commit/86600313462638e7becc726e53f1bc67af108667

This is like an extremely sluggish kind of pair programming!

What do you think of it now? Is it growing on you? What worries you
about it?

[email protected]
[email protected]
http://rubyforge.org/mailman/listinfo/rspec-users


rspec-users mailing list
[email protected]
http://rubyforge.org/mailman/listinfo/rspec-users

cheers,
Matt

http://blog.mattwynne.net
+44(0)7974 430184

On Aug 9, 2010, at 7:42 AM, Matt W. wrote:

On Aug 8, 2010, at 11:13 AM, Matt W. wrote:

It turns out that if you have
This is because the rails assertion, which, when running with Ruby-1.9, delegates to Minitest::Assertions#assert_block, which delegates to a message() method that it defines. So the message() method defined by let() overrides the message() method in the Assertions module, and results in unexpected and undesirable outcomes.

No - holdover from exploratory session.

Also, how come there’s nothing in the specs about the #message method that caused all this?

Good point.

New patch: http://github.com/rspec/rspec-rails/commit/86600313462638e7becc726e53f1bc67af108667

This is like an extremely sluggish kind of pair programming!

What do you think of it now? Is it growing on you? What worries you about it?

Not sure yet. On one hand it feels sort of unnecessary, but on the
other, it does solve this particular anomaly. I’m going to sit on it for
a day or two and see if any specific arguments against come to mind. In
the mean time, other opinions would be welcome.

Cheers,
David

Sorry it’s taken me so long to respond–I have considerably less time
on weekdays than the weekend to take care of things like this.

Yehuda K. made a similar suggestion to me, referencing some code from merb:http://github.com/merb/merb/blob/master/merb-core/lib/merb-core/contr

Merb also has an override! method that end users can use to override the registered reserved methods, which I agree would be a necessary part of this. The idea being that any user that does that does so explicitly and knowingly.

Merb’s implementation is very similar to what I had in mind. It’s
nice to see I’m not in left field with my idea :).

I agree that having something like override! is important, although I
think I slightly prefer an API like this:

allow_reserved_overrides do
def reserved_method
end
end

Or maybe I like blocks too much…

The blacklist comment probably wouldn’t work for upstream libs like Rails, Test::Unit or MiniUnit. It would be up to RSpec to define those lists. But maybe that’s an acceptable tradeoff. WDYT?

RSpec is pretty high-profile in the Ruby community, so we could
hopefully get most libraries to add their reserved methods using
something like:

if defined?(RSpec::Core.add_reserved_methods)
RSpec::Core.add_reserved_methods :foo, :bar, :bazz
end

As far as Rails goes, rspec-rails seems like a natural point to
register these reserved methods. For libraries that are distributed
with ruby like Test::Unit, I think it’s acceptable to register their
reserved methods in rspec-core itself.

What do others think of this idea? I’m willing to take a stab at
implementing this if there is support for it.

Myron

On 12 Aug 2010, at 04:30, David C. wrote:

I think they should all be registered in the same place, in rspec-core. Or are you saying that rspec-rails would take the responsibility of registering the names for rspec-rails, rails, test/unit and minitest? That makes sense to me, as long as they’re all using RSpec::Core::register_reserved_name (or whatever).

What do others think of this idea? I’m willing to take a stab at
implementing this if there is support for it.

I’m still not sold on this idea yet. Seems like a lot of complexity for not much benefit. I’ve already taken care of the message issue by encapsulating the assertion libs in a separate scope.

Anybody else have opinions on this?

The idea sounds interesting, but sounds like it could lead to a nasty
dependency down the line. Is there any way of doing the core of this as
an external gem, that all interested projects could use?

I’m only following the edge of this thread, but by dependency radar went
off - it’s a bit oversensitive.


http://www.patchspace.co.uk/
http://www.linkedin.com/in/ashleymoran

On Aug 11, 2010, at 3:05 PM, Myron M. wrote:

I agree that having something like override! is important, although I

with ruby like Test::Unit, I think it’s acceptable to register their
reserved methods in rspec-core itself.

I think they should all be registered in the same place, in rspec-core.
Or are you saying that rspec-rails would take the responsibility of
registering the names for rspec-rails, rails, test/unit and minitest?
That makes sense to me, as long as they’re all using
RSpec::Core::register_reserved_name (or whatever).

What do others think of this idea? I’m willing to take a stab at
implementing this if there is support for it.

I’m still not sold on this idea yet. Seems like a lot of complexity for
not much benefit. I’ve already taken care of the message issue by
encapsulating the assertion libs in a separate scope.

Anybody else have opinions on this?

This forum is not affiliated to the Ruby language, Ruby on Rails framework, nor any Ruby applications discussed here.

| Privacy Policy | Terms of Service | Remote Ruby Jobs