Make be_predicate fail upon calling a private method #207

JonRowe · 2013-02-13T03:39:11Z

A potential solution to #206, this makes be_predicate fail with a nice warning when calling a private methods.

Potentially this might want to be configurable, so as not to break existing test suites? Is this a decision we want to make for the person writing the test suite? Is #206 the desired behaviour or do we want to let users make the decision about calling private methods...

myronmarston · 2013-02-13T04:26:34Z

@JonRowe -- thanks for taking a stab at this. I've been meaning to respond to #206. I share many of the questions you mentioned above!

Anyhow, let's take the conversation back to #206 where we can figure out the overall strategy to solve this, and then, if it lines up with what you've done here, we can return to this PR to discuss any remaining implementation concerns.

dchelimsky · 2013-02-15T10:41:15Z

features/built_in_matchers/predicates.feature

+       """
+     When I run `rspec attempting_to_match_private_method_spec.rb`
+     Then the output should contain "1 example, 1 failure"
+     And the output should contain "expected secret? to return true, but it's a private method"


I think this message should clarify that private methods are not accessible in expectations. It might seem obvious to some, but it reads (to me) like "I was trying to make a left turn, but I was thirsty" (the "but" being somewhat orthogonal to the expectation).

In normal use, calling privates from the outside results in an error e.g.

NoMethodError: private method `print' called for "this":String

Why not treat these as errors as well, e.g.

NoMethodError: private method `print' called for "this":String (expectations on private methods are not supported)

Or even take it a step further:

UnsupportedOperationError: expectation set on private method `print' for "this":String

JonRowe · 2013-02-15T12:02:16Z

I've taken your feedback to heart, and am raising an error with a message, which actually cleans up the resulting failure messages again, however on 1.8.7 I'm now getting weird Too many open files (Errno::EMFILE) errors, but not on the changed feature, any hints as to what's wrong there?

dchelimsky · 2013-02-15T13:26:18Z

@JonRowe no idea :( I've never understood why that happens.

JonRowe · 2013-02-17T10:50:45Z

Just re-run today and they work, from a bit of googling seems this is a "weird" Heisenbug which is platform dependant... So this passes on 1.8.7 and 1.9.3

fables-tales · 2013-04-15T15:33:02Z

So my feeling here is this might be a travis weirdness, can we force it to rerun the build?

JonRowe · 2013-04-15T23:54:04Z

We can do, but I have experienced the failure locally, it's a weird Ruby heisenbug...

JonRowe · 2013-04-16T08:49:07Z

I rebased this, so let's see what happens to the build.

JonRowe · 2013-04-16T12:54:33Z

Woo, passes

fables-tales · 2013-04-16T17:32:57Z

@JonRowe I think this needs a changelog entry, but is otherwise pretty awesome. Could you add one? 👍 💚 :

myronmarston · 2013-04-16T17:34:52Z

This can't be merged until 3.0, because it'll be a breaking change. It's definitely a good change, though!

fables-tales · 2013-04-16T17:39:28Z

That too :)

myronmarston · 2013-04-16T17:48:34Z

lib/rspec/matchers/built_in/be.rb

@@ -135,6 +135,9 @@ def initialize(*args, &block)

        def matches?(actual)
          @actual = actual
+
+          raise "UnsupportedOperationError: expectation set on private method `#{predicate}`" if actual.private_methods.include?(predicate.to_sym) || actual.private_methods.include?(predicate.to_s)


This line is super long, particularly for having a trailing if. I'd prefer to see the non-trailing if for a case like this.

Rather than including handling for both 1.8 and 1.9 in a single line, I'd prefer to see something like:

if methods.first.is_a?(String) def private_method_on?(object) object.private_methods.include?(predicate.to_s) end else def private_method_on?(object) object.private_methods.include?(predicate.to_sym) end end

I've refactored this, take a look when you have a sec.

fables-tales · 2013-05-28T23:24:40Z

@JonRowe want to merge this in now we're doing 3.0 on master?

dchelimsky · 2013-05-28T23:28:10Z

@JonRowe want to merge this in now we're doing 3.0 on master?

I thought that was after the 2.14.0 final release. Correct? No?

JonRowe · 2013-05-28T23:29:28Z

That's what I was pushing for on the mailing list. I think master should be 2.14 until we actually ship 2.14 and not just an rc. /cc @myronmarston @soulcutter

fables-tales · 2013-05-28T23:29:55Z

@dchelimsky 2.14 dev is on the maintenance branch now. @myronmarston @alindeman and myself were chatting about this last night on irc. The 2.99 maintenance branch also exists.

dchelimsky · 2013-05-28T23:32:04Z

No sense going back :)

On Tue, May 28, 2013 at 6:29 PM, Sam Phippen notifications@github.comwrote:

@dchelimsky https://github.com/dchelimsky 2.14 dev is on the
maintenance branch now. @myronmarston https://github.com/myronmarston
@alindeman https://github.com/alindeman and myself were chatting about
this last night on irc. The 2.99 maintenance branch also exists.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/207#issuecomment-18587035
.

JonRowe · 2013-05-30T09:11:20Z

I'm going to look at this and merge it tonight

dchelimsky · 2013-05-30T11:22:42Z

lib/rspec/matchers/built_in/be.rb

@@ -135,6 +135,9 @@ def initialize(*args, &block)

        def matches?(actual)
          @actual = actual
+
+          raise "UnsupportedOperationError: expectation set on private method `#{predicate}`" if is_private_on?( @actual )


How about making an UnsupportedOperationError class? Raising a RuntimeError with a message that suggests it is a different kind of error feels odd to me.

fables-tales · 2013-05-30T12:58:10Z

I like this. 🐑 it.

dchelimsky · 2013-05-30T13:04:59Z

spec/rspec/matchers/be_spec.rb

@@ -31,6 +31,18 @@
    }.to raise_error(NameError, /happy\?/)
  end

+  it 'fails when :predicate? is private' do
+    klass = Class.new do


Just for grins: I'd name this class privately_happy and have happy? return true :)

On your head be it... ;) I totally did not indulge my own humour in the commit message...

…vate Make be_predicate fail upon calling a private method

JonRowe mentioned this pull request Feb 13, 2013

Matchers should fail when trying to spec against private methods of the object under test. #206

Closed

dchelimsky reviewed Feb 15, 2013
View reviewed changes

myronmarston reviewed Apr 16, 2013
View reviewed changes

dchelimsky reviewed May 30, 2013
View reviewed changes

JonRowe added 5 commits May 30, 2013 22:20

describe behaviour

bf15a4d

fail with warning when calling private messages

518f6a9

handle 1.8.7 syntax

71d1262

raising an error on private method calls

d51ddf7

refactor 187 support

ea78020

JonRowe added 4 commits May 30, 2013 22:24

make the feature more concise

148cc99

performance tweak

f519301

refactor spec layout

23c96a6

throw a proper error rather than runtime error

dc32c7b

dchelimsky reviewed May 30, 2013
View reviewed changes

for lulz

ff9ec78

JonRowe added a commit that referenced this pull request May 30, 2013

Merge pull request #207 from JonRowe/expect_private_methods_to_be_pri…

0409804

…vate Make be_predicate fail upon calling a private method

JonRowe merged commit 0409804 into rspec:master May 30, 2013

JonRowe deleted the expect_private_methods_to_be_private branch May 30, 2013 13:30

JonRowe restored the expect_private_methods_to_be_private branch June 1, 2013 01:31

JonRowe deleted the expect_private_methods_to_be_private branch June 1, 2013 01:44

JonRowe mentioned this pull request Jun 1, 2013

Deprecate calling private methods as predicates #261

Merged

myronmarston mentioned this pull request Apr 17, 2014

Dynamic have predicate matchers should fail when mapped to a private method #529

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make be_predicate fail upon calling a private method #207

Make be_predicate fail upon calling a private method #207

JonRowe commented Feb 13, 2013

myronmarston commented Feb 13, 2013

dchelimsky Feb 15, 2013

JonRowe commented Feb 15, 2013

dchelimsky commented Feb 15, 2013

JonRowe commented Feb 17, 2013

fables-tales commented Apr 15, 2013

JonRowe commented Apr 15, 2013

JonRowe commented Apr 16, 2013

JonRowe commented Apr 16, 2013

fables-tales commented Apr 16, 2013

myronmarston commented Apr 16, 2013

fables-tales commented Apr 16, 2013

myronmarston Apr 16, 2013

JonRowe Apr 17, 2013

fables-tales commented May 28, 2013

dchelimsky commented May 28, 2013

JonRowe commented May 28, 2013

fables-tales commented May 28, 2013

dchelimsky commented May 28, 2013

JonRowe commented May 30, 2013

dchelimsky May 30, 2013

fables-tales May 30, 2013

fables-tales commented May 30, 2013

dchelimsky May 30, 2013

JonRowe May 30, 2013

dchelimsky May 30, 2013

Make be_predicate fail upon calling a private method #207

Make be_predicate fail upon calling a private method #207

Conversation

JonRowe commented Feb 13, 2013

myronmarston commented Feb 13, 2013

Choose a reason for hiding this comment

JonRowe commented Feb 15, 2013

dchelimsky commented Feb 15, 2013

JonRowe commented Feb 17, 2013

fables-tales commented Apr 15, 2013

JonRowe commented Apr 15, 2013

JonRowe commented Apr 16, 2013

JonRowe commented Apr 16, 2013

fables-tales commented Apr 16, 2013

myronmarston commented Apr 16, 2013

fables-tales commented Apr 16, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fables-tales commented May 28, 2013

dchelimsky commented May 28, 2013

JonRowe commented May 28, 2013

fables-tales commented May 28, 2013

dchelimsky commented May 28, 2013

JonRowe commented May 30, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fables-tales commented May 30, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment