Figure out best practices for sorbet + RSpec mocks

[xaviershay]

Currently, RSpec mocks are disallowed by the Sorbet runtime system.

A relevant design justification is:

Mocha mocks (stub in tests) will not pass any type checks by default. This is deliberate and considered a feature; bare mocks make tests brittle and tend to cause problems when refactoring code, regardless of type checking.

I agree with "bare mocks make tests brittle", but that's exactly the issue that verifying doubles try to address also! I'm not convinced yet that sorbet shouldn't support verifying doubles.

With verifying doubles we are effectively doing many (but not all) of the kinds of checks that sorbet does - it should be possible for some compatibility.

This is a placeholder issue to try and draw search traffic of people experiencing this issue and who have suggestions for how to fix. I'm not actively pushing forward a solution at the moment.

(TODO: Provide minimal example here)

[xaviershay]

I kind of like solution #2 presented at https://stackoverflow.com/a/56905241/379639 - effectively disable runtime type checking when mock is used. (I don't like how it depends on error messages though, feels brittle.)

Not perfect, but definitely achievable and so might pass as an "official" recommendation for time being.

[JonRowe]

I'm up for working with Sorbet to improve our verifying double compatibility here, I think 2 is workable and we could help them detect us / provide a way for users to decide what they want to do?

[jaredbeck]

What about using union types? A naive and incorrect implementation would look something like the following:

diff --git a/gems/sorbet-runtime/lib/types/private/methods/decl_builder.rb b/gems/sorbet-runtime/lib/types/private/methods/decl_builder.rb
index 0850c419c..eb5bd6dfe 100644
--- a/gems/sorbet-runtime/lib/types/private/methods/decl_builder.rb
+++ b/gems/sorbet-runtime/lib/types/private/methods/decl_builder.rb
@@ -32,13 +32,23 @@ module T::Private::Methods
       )
     end
 
+    def transform_params_by_supporting_rspec_mocks(params)
+      params.each_with_object({}) { |(k, v), a|
+        a[k] = ::T.any(v, ::RSpec::Mocks::InstanceVerifyingDouble)
+      }
+    end
+
     def params(params)
       check_live!
       if !decl.params.equal?(ARG_NOT_PROVIDED)
         raise BuilderError.new("You can't call .params twice")
       end
 
-      decl.params = params
+      if defined?(::RSpec::Mocks)
+        decl.params = transform_params_by_supporting_rspec_mocks(params)
+      else
+        decl.params = params
+      end
 
       self
     end

A correct implementation would have to also: 1) support T.let 2) be able to transform more sophisticated params.

This might be a terrible idea (too complicated) but what do you think? :grin:

[xaviershay]

@jaredbeck interesting! Assuming this is technically feasible, I think either a) the sorbet team would need to sign up for supporting it (since would introduce a dep between sorbet + rspec in sorbet code), or b) it would need to be possible via plugin API. I am not familiar enough to know how relatively likely those options are.

[zanella]

This gem circumvents the issue, seems to be a better written workaround as the one done in stack overflow: https://github.com/tricycle/rspec-sorbet

[bessey]

EDIT: As my thoughts are not RSpec specific I am moving the discussion to the Sorbet repo, if you have comment pertaining to this comment, please keep them there https://github.com/sorbet/sorbet/issues/5683

---

Sorry to dig up an old thread but this seemed to related to start a new one...

Background: I am experimenting with introducing Sorbet into a large codebase. We follow a pattern in this codebase where the codebase is subdivided into components, and those components are only allowed to communicate via a slim "front door" API exposed as class methods on one class per component.

The primary value I saw in Sorbet initially (when typed: true / strict cases are few and far between) was typing these class methods. What I expected was we would mock the return values in tests that would otherwise depend on a certain component, and sorbet-runtime would enforce that the mocks remained up to date. Something like this (forgive contrived example but wanted it to be minimal):

# typed: true

class Blog
  def initialize(id)
    @id = id
  end
  def comment_ids
    DiscussionDomain.comment_ids_for_blog(id)
  end
end

class DiscussionDomain
  extend T::Sig

  sig {params(blog_id: Integer).returns(T::Array[Integer])}
  def self.comment_ids_for_blog(blog_id)
    [123, 456]
  end
end

RSpec.describe Blog do
  it "delegates comment_ids to DiscussionDomain" do
    blog = Blog.new("1")
    # I want this to raise, via sorbet-runtime
    # Parameter 'blog_id': Expected type Integer, got type String with value "1"
    expect(DiscussionDomain).to receive(:comment_ids_for_blog).with("1").and_return([2])
    blog.comment_ids
  end

  it "delegates comment_ids to DiscussionDomain 2" do    
    blog = Blog.new(1)
    # I want this to raise, via sorbet-runtime
    # Return value: Expected type Array[Integer], got type Array[String] with value ["2"]
    expect(DiscussionDomain).to receive(:comment_ids_for_blog).with(1).and_return(["2"])
    blog.comment_ids
  end
end

In reality of course, the moment I call expect(...).to receive(...) RSpec Mocks replaces the method, so Sorbet Runtime's signature never gets called.

# Before Mock
[1] pry(#<RSpec::...>)> DiscussionDomain.method(:comment_ids_for_blog)
=> #<Method: DiscussionDomain.comment_ids_for_blog(*args, &block) /usr/local/bundle/gems/rspec-mocks-3.10.3/lib/rspec/mocks/method_double.rb:63>

# After Mock
[1] pry(#<RSpec::...>)> DiscussionDomain.method(:comment_ids_for_blog)
=> #<Method: DiscussionDomain.comment_ids_for_blog(*args, &blk) /usr/local/bundle/gems/sorbet-runtime-0.5.6539/lib/types/private/methods/_methods.rb:248>

This seems like a tremendous missed opportunity to me, would it not be possible for some kind of integration between RSpec Mocks and Sorbet Runtime that instruments partial test doubles with the same type signature as the method being mocked, and therefore maintains type enforcement even in the event that the original implementation is never called? Conceptually I guess it's an extension of the existing verify_partial_doubles configuration, specific to Sorbet.

Given that Sorbet has been kicking for several years now I imagine I am missing something and this is for some reason a massive anti-pattern :laughing:. To me though this seems like having your cake and eating it too: automatic enforcement that partial test doubles are honouring the API specified.

Has anyone thought about this? I'm no RSpec Guru but I'll try and find the time to knock up a prototype if not...

[bessey]

Just to follow up, a coworker and I have with the help of the Sorbet team developed a quick demo of how this can be implemented in RSpec:

https://github.com/ImmersiveLabsUkOrg/rspec-mocks/pull/2

It needs no public API changes to Sorbet, so at this point this is wholely a question of: how do we resolve the remaining bugs and integrate into RSpec in a way that can actually be merged (most likely configurable from the outside, rather than making RSpec dependent on Sorbet?)

[multiplegeorges]

Bumping for visibility, it looks like the above is a workable solution. The current issues lead us to either remove sigs (bad!) or not fully test (also bad!).

[JonRowe]

Prehaps the above can be integrated into rspec-sorbet, theres no way we (RSpec) are adding a dependency on Sorbet, and I'm also reluctant to bring it in as a "soft dependency" we'd then have to support, if people working on an integration gem needed an API point to hook into we could look to provide that, we already have a before_verifying_doubles hook for ActiveRecord for example, it might be possible to use this already.

[multiplegeorges]

Yeah, that makes sense, totally understand your position. Thanks for the input!

[JonRowe]

Closing as a duplicate of rspec/rspec#3 during the monorepo migration