Add verifying double features from rspec-fire. #378

xaviershay · 2013-07-24T04:03:45Z

This is intended to be both API compatible with rspec-fire, and to completely obsolete it. It does not yet implement any of the suggested changes in #227, such as transferring nested constants by default or an alternate constant stubbing interface.

It adds the following behaviours:

verifying doubles
  class doubles
    when doubled class is not loaded
      allows any method to be stubbed
    when doubled class is loaded
      can transfer nested constants to the double
      can replace existing constants for the duration of the test
      only allows class methods that exist to be expected
      only allows class methods that exist to be stubbed
      allows class to be specified by constant
  instance doubles
    when doubled class is not loaded
      allows any instance method to be stubbed
    when doubled class is loaded
      only allows instance methods that exist to be expected
      only allows instance methods that exist to be stubbed
      checks the arity of stubbed methods
      allows class to be specified by constant
  when verify_constant_names config option is set
    prevents creation of instance doubles for unloaded constants
    prevents creation of class doubles for unloaded constants

It adds a perhaps unexpected method to the public API: ArityMatcher.match!. This seemed useful enough on its own to make public, but I would consider keeping it private if anyone exists.

Questions for reviewers

I'm not familiar with yardoc style. Does this conform?
What is the preferred way to split features over the README and actual cucumber files?
It is unfortunate that I need to use a parallel class hierarchy for the verifying counterparts. Is there a better way to do this?

@myronmarston @JonRowe

Fixes #227.

coveralls · 2013-07-24T04:05:37Z

Coverage increased (+0%) when pulling 42d0860 on xaviershay:issue-227 into e6d1980 on rspec:master.

coveralls · 2013-07-24T04:10:06Z

Coverage increased (+0%) when pulling 927ae29 on xaviershay:issue-227 into e6d1980 on rspec:master.

myronmarston · 2013-07-24T05:19:14Z

features/verifying_doubles/README.md

+When using verifying doubles, RSpec will check that the methods being stubbed
+are actually present on the underlying object if it is available.
+
+No checking will happen when running the spec in isolation, but when run in the


I think the "when running the spec in isolation" part is a bit misleading: it's not about if I run rspec path/to/one/spec.rb vs rspec spec but about whether or not the named constant is loaded or not. I think of rspec path/to/one/spec.rb as "running a spec in isolation", but it will still verify the double if the named constant is loaded.

How about

No checking will happen if the object definition isn't loaded, but when run with the object loaded (either as a full spec run or by explicitly preloading collaborators) a failure will be triggered if an invalid method is being stubbed.

myronmarston · 2013-07-24T05:45:49Z

Looking good so far. I need to go fold laundry but I'll come back later (or perhaps tomorrow) to read through and comment on the rest.

JonRowe · 2013-07-24T11:40:50Z

Currently this is failing for all our 1.8.7 compatible builds.

JonRowe · 2013-07-24T11:59:59Z

features/verifying_doubles/README.md

+
+    # once you do this, you can no longer access MyCoolGem::Widget in your
+    # example...
+    class_double("MyCoolGem")


Am I the only one that finds class_double confusing when used on a Module? :)

xaviershay · 2013-07-24T15:04:52Z

Thanks for the great feedback! Will address it all in the next couple of days.

coveralls · 2013-07-25T03:16:48Z

Coverage increased (+0%) when pulling 9a8310c on xaviershay:issue-227 into e6d1980 on rspec:master.

xaviershay · 2013-07-25T03:17:59Z

New commits are hiding an open discussion about naming of verify_constant_names option. Other suggestions (copied from old comments):

verify_instance_and_class_double_names
verify_verifying_double_names
check_verifying_double_names
verify_misspelled_constant_names
verify_doubled_constant_names
verify_doubled_class_names

coveralls · 2013-07-25T03:56:49Z

Coverage increased (+0%) when pulling b177ba4 on xaviershay:issue-227 into 474e643 on rspec:master.

coveralls · 2013-07-25T04:01:06Z

Coverage increased (+0%) when pulling 6ca3078 on xaviershay:issue-227 into 474e643 on rspec:master.

xaviershay · 2013-07-25T04:01:58Z

1.8.7 is passing now.

myronmarston · 2013-07-25T05:12:36Z

verify_instance_and_class_double_names

This is OK.

verify_verifying_double_names

I dislike. "verify verifying" is awkwardly awkward.

check_verifying_double_names

It's OK.

verify_misspelled_constant_names

It's OK.

verify_doubled_constant_names

I think I like this one best.

verify_doubled_class_names

I like this one OK, too.

myronmarston · 2013-07-25T05:14:59Z

lib/rspec/mocks/arity_matcher.rb

+          method.name,
+          arity_description,
+          actual_arity
+        ]


Any reason you prefer this over "#{string} #{interpolation}"? I find string interpolation easier to read.

(BTW, I'm not writing that to tell you you must change it--I'm genuinely curious why you chose to go that route!)

I find this version easier to read, since I see the "shape" of the sentence much clearer, and there's not so many that it isn't obvious which value interpolates where.

It fits much nicer in 80-chars.

myronmarston · 2013-08-04T23:48:17Z

features/verifying_doubles/class_doubles.feature

@@ -0,0 +1,52 @@
+Feature: Using a class double
+
+  `class_double` is provided as a complement to `instance_double`, with the


Given that this references instance_double, but comes first alphabetically, it would be good to list the order we want this in the relish nav file, so that this file can come later.

Addressed in new commit.

xaviershay · 2013-08-05T00:29:00Z

All other comments make sense, will revise.

myronmarston · 2013-08-05T00:37:57Z

features/verifying_doubles/introduction.feature

+  underlying object if it is available. Prefer using veryifing doubles over
+  normal doubles.
+
+  No checking will happen if the object definition is not loaded, but when run


No checking will happen if the object definition is not loaded

This is a bit misleading/inaccurate: it's not about whether or not the object definition is loaded, it's about whether or not the named class constant is defined. I've actually run into a subtle edge case where the class constant was defined but the object definition wasn't loaded, and rspec-fire told me the named class didn't implement a method that it actually did (at least it did when fully loaded). The specific edge case I ran into was in our use of a github-sourced gem in our gemfile that used the common idiom (as generated by bundle gem) whereby the .gemspec file has a require 'mygem/version', which caused MyGem::VERSION to get loaded (which implies that MyGem was a defined constant) when Bundler.setup ran (since it eval'd the gemspec file), but the actual definition of the MyGem class wasn't loaded. Note that this isn't a problem with normal released gems...the .gemspec file gets compiled to a static YAML file during that process, and thus bundler can read the gemspec w/o it loading the version constant.

While this is a bit of an edge case other users will hopefully never run into, I think it's good to reduce potential future confusion by accurately stating that the checking happens if the class constant is defined, regardless of whether or not the class definition is actually loaded.

xaviershay · 2013-08-05T01:05:39Z

Don't have a strong opinion on the .name issue. I reckon let's role with current behaviour and if it trips anyone up we can revisit.

myronmarston · 2013-08-05T04:05:00Z

As far as I'm concerned, this needs a changelog entry, and to be rebased against master (or merged against it, if you prefer), and it needs a green travis build, but is ready to be merged otherwise.

This is intended to be both API compatible with rspec-fire, and to completely obsolete it. Fixes rspec#227.

xaviershay · 2013-08-06T04:18:26Z

@myronmarston rebased, Changelog entry added, CI is green.

coveralls · 2013-08-06T04:19:43Z

Coverage increased (+0%) when pulling 03f51d9 on xaviershay:issue-227 into cee433c on rspec:master.

Add verifying double features from rspec-fire.

myronmarston · 2013-08-06T04:57:04Z

Beautiful :). Thanks for doing such a great job with this!

myronmarston · 2013-08-06T06:08:05Z

BTW, I opened up issues for the other features and changes we discussed so that we don't forget them:

Do you want to take a stab at any of these, @xaviershay? If so, please comment on them so someone else doesn't also work on them and cause wasted effort.

fables-tales · 2013-08-06T11:38:11Z

@xaviershay thanks so much, this is a really cool feature.

soulcutter · 2013-08-06T14:11:34Z

@xaviershay I'm very excited about your work on this! Thanks a ton.

xaviershay mentioned this pull request Jul 24, 2013

WIP: Sketch of what an rspec-fire port would look like. #372

Closed

myronmarston reviewed Jul 24, 2013
View reviewed changes

JonRowe reviewed Jul 24, 2013
View reviewed changes

myronmarston reviewed Jul 25, 2013
View reviewed changes

myronmarston reviewed Aug 4, 2013
View reviewed changes

myronmarston reviewed Aug 5, 2013
View reviewed changes

Add verifying double features from rspec-fire.

03f51d9

This is intended to be both API compatible with rspec-fire, and to completely obsolete it. Fixes rspec#227.

myronmarston added a commit that referenced this pull request Aug 6, 2013

Merge pull request #378 from xaviershay/issue-227

f9f0e5b

Add verifying double features from rspec-fire.

myronmarston merged commit f9f0e5b into rspec:master Aug 6, 2013

This was referenced Aug 6, 2013

Add object_double #391

Closed

Verify method arity of verifying doubles when they receive messages #393

Closed

myronmarston mentioned this pull request Aug 30, 2013

Constrain null-object verifying doubles according to their defined interface #392

Closed

se3000 mentioned this pull request Oct 30, 2013

Wrong number of arguments using better_stub with any_instance in RSpec 2.14.6 se3000/better_receive#5

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add verifying double features from rspec-fire. #378

Add verifying double features from rspec-fire. #378

xaviershay commented Jul 24, 2013

coveralls commented Jul 24, 2013

coveralls commented Jul 24, 2013

myronmarston Jul 24, 2013

JonRowe Jul 24, 2013

myronmarston commented Jul 24, 2013

JonRowe commented Jul 24, 2013

JonRowe Jul 24, 2013

xaviershay commented Jul 24, 2013

coveralls commented Jul 25, 2013

xaviershay commented Jul 25, 2013

coveralls commented Jul 25, 2013

coveralls commented Jul 25, 2013

xaviershay commented Jul 25, 2013

myronmarston commented Jul 25, 2013

myronmarston Jul 25, 2013

myronmarston Jul 25, 2013

xaviershay Jul 26, 2013

myronmarston Aug 4, 2013

xaviershay Aug 5, 2013

xaviershay commented Aug 5, 2013

myronmarston Aug 5, 2013

xaviershay commented Aug 5, 2013

myronmarston commented Aug 5, 2013

xaviershay commented Aug 6, 2013

coveralls commented Aug 6, 2013

myronmarston commented Aug 6, 2013

myronmarston commented Aug 6, 2013

fables-tales commented Aug 6, 2013

soulcutter commented Aug 6, 2013

		@@ -0,0 +1,52 @@
		Feature: Using a class double

		`class_double` is provided as a complement to `instance_double`, with the

Add verifying double features from rspec-fire. #378

Add verifying double features from rspec-fire. #378

Conversation

xaviershay commented Jul 24, 2013

Suggested reading order

Questions for reviewers

coveralls commented Jul 24, 2013

coveralls commented Jul 24, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

myronmarston commented Jul 24, 2013

JonRowe commented Jul 24, 2013

Choose a reason for hiding this comment

xaviershay commented Jul 24, 2013

coveralls commented Jul 25, 2013

xaviershay commented Jul 25, 2013

coveralls commented Jul 25, 2013

coveralls commented Jul 25, 2013

xaviershay commented Jul 25, 2013

myronmarston commented Jul 25, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xaviershay commented Aug 5, 2013

Choose a reason for hiding this comment

xaviershay commented Aug 5, 2013

myronmarston commented Aug 5, 2013

xaviershay commented Aug 6, 2013

coveralls commented Aug 6, 2013

myronmarston commented Aug 6, 2013

myronmarston commented Aug 6, 2013

fables-tales commented Aug 6, 2013

soulcutter commented Aug 6, 2013