search

KnapsackPro / knapsack_pro-ruby

Knapsack Pro gem splits tests across parallel CI nodes and makes sure that tests run in optimal time
https://knapsackpro.com
MIT License
132 stars 27 forks source link

Remove manual steps to configure WebMock (and VCR) #251

Closed 3v0k4 closed 5 months ago

3v0k4 commented 6 months ago

Story

Ticket https://trello.com/c/nInb3gxY/359-knapsackpro-gem-dx-remove-manual-steps-for-webmock-vcr

Currently, setting up Knapsack Pro with WebMock/VCR requires additional steps because WebMock replaces Net::HTTP with its own client that (also) blocks requests to the Knapsack Pro API.

Some codebases configure WM in a way that doesn't play well with Knapsack Pro, even when the manual configuration steps are followed.

For example, each call to WebMock.disable_net_connect! without passing allow: ['api.knapsackpro.com'] overrides the previous WebMock.disable_net_connect!(allow: ['api.knapsackpro.com']) call preventing requests to api.knapsackpro.com.

Users could have multiple calls to WebMock that "reset" the configuration including in before/after hooks. So it's difficult to solve the issue by using hooks to undo the reset.

(Aside: WebMock.reset! does not touch WebMock::Config.instance.allow so that does not pose a problem, only disable_net_connection! does.)

This PR, introduces a mechanism to always call the original Net::HTTP for requests to the Knapsack Pro API preventing WM from interfering.

What follows are the internal details on why this PR works. It may be overwhelming to understand it, so feel free to ignore. But I'm leaving notes for reference.

Test

I created a webmock branch (same name as the branch for this PR) in rails-app-with-knapsack-pro with:

diff --git a/spec/support/vcr.rb b/spec/support/vcr.rb
index 72e2dbd..89dce79 100644
--- a/spec/support/vcr.rb
+++ b/spec/support/vcr.rb
@@ -2,26 +2,9 @@ require 'vcr'

 VCR.configure do |config|
   config.cassette_library_dir = "spec/fixtures/vcr_cassettes"
-  config.hook_into :webmock # or :fakeweb
-
-  config.ignore_hosts(
-    'localhost',
-    '127.0.0.1',
-    '0.0.0.0',
-    'api.knapsackpro.com',
-    'api-fake.knapsackpro.com',
-    'api-staging.knapsackpro.com',
-    'api.knapsackpro.test',
-    'api-disabled-for-fork.knapsackpro.com',
-  )
+  config.hook_into :webmock
+  config.ignore_localhost = true
 end

 require 'webmock/rspec'
-
-WebMock.disable_net_connect!(allow: [
-  'api.knapsackpro.com',
-  'api-fake.knapsackpro.com',
-  'api-staging.knapsackpro.com',
-  'api.knapsackpro.test',
-  'api-disabled-for-fork.knapsackpro.com',
-]) if defined?(WebMock)
+WebMock.disable_net_connect!

Since the branch names are the same, CI will use that for the E2E tests of this PR.

Here are:

I also built the API with this PR but without removing the custom WM/VCR config:

gem 'knapsack_pro', github: 'KnapsackPro/knapsack_pro-ruby', branch: 'webmock'

And built without the custom WM/VCR config:

diff --git a/spec/support/config/vcr.rb b/spec/support/config/vcr.rb
index 66b6b75a..9de2e61a 100644
--- a/spec/support/config/vcr.rb
+++ b/spec/support/config/vcr.rb
@@ -2,20 +2,9 @@ require 'vcr'

 VCR.configure do |config|
   config.cassette_library_dir = 'spec/fixtures/vcr_cassettes'
-  config.hook_into :webmock # or :fakeweb
+  config.hook_into :webmock
   config.allow_http_connections_when_no_cassette = true
-  config.ignore_hosts(
-    'localhost',
-    '127.0.0.1',
-    '0.0.0.0',
-    'api.knapsackpro.com',
-    'api-staging.knapsackpro.com',
-    'api.knapsackpro.dev',
-  )
+  config.ignore_localhost = true
 end

-WebMock.disable_net_connect!(allow: [
-  'api.knapsackpro.com',
-  'api-staging.knapsackpro.com',
-  'api.knapsackpro.dev',
-])
+WebMock.disable_net_connect!

Details for WebMock

WebMock enables itself at the beginning of a test run. Here's how it's done for RSpec:

  config.before(:suite) do
    WebMock.enable!
  end

https://github.com/bblimke/webmock/blob/fc6a2ab897a069d861adbc1c88e51b2cf8aa88ac/lib/webmock/rspec.rb#L30-L32

enable! ends up calling WebMock::HttpLibAdapters::NetHttpAdapter.enable!:

      def self.enable!
        Net.send(:remove_const, :HTTP)
        Net.send(:remove_const, :HTTPSession)
        Net.send(:const_set, :HTTP, @webMockNetHTTP)
        Net.send(:const_set, :HTTPSession, @webMockNetHTTP)
      end

https://github.com/bblimke/webmock/blob/fc6a2ab897a069d861adbc1c88e51b2cf8aa88ac/lib/webmock/http_lib_adapters/net_http.rb#L16-L21

which replaces Net::HTTP with @webMockNetHTTP:

@webMockNetHTTP = Class.new(Net::HTTP) do

https://github.com/bblimke/webmock/blob/fc6a2ab897a069d861adbc1c88e51b2cf8aa88ac/lib/webmock/http_lib_adapters/net_http.rb#L40

Notice it also saves the original Net::HTTP in WebMock::HttpLibAdapters::NetHttpAdapter::OriginalNetHTTP:

OriginalNetHTTP = Net::HTTP unless const_defined?(:OriginalNetHTTP)

https://github.com/bblimke/webmock/blob/fc6a2ab897a069d861adbc1c88e51b2cf8aa88ac/lib/webmock/http_lib_adapters/net_http.rb#L14

Details for WebMock and VCR

When WM is used in tandem with VCR, the situation gets a bit more complicated.

When VCR starts it enables WebMock and monkey patches WM's net_connect_allowed? to:

VCR.turned_on? ? true : net_connect_allowed_without_vcr?(*args)

https://github.com/vcr/vcr/blob/cde89346aebcbe5280299ba10aedb3d97860c557/lib/vcr/library_hooks/webmock.rb#L160-L171

which means if VCR is on it does not allow WM to block anything, otherwise it delegates to WM because net_connect_allowed_without_vcr? is the original WM’s net_connect_allowed?.

If VCR is off, we are in the WM-only situation explained above.

Otherwise, VCR interacts with WM’s @webMockNetHTTP as follows.

VCR hooks into WM's global stubs:

::WebMock.globally_stub_request do |req|
  global_hook_disabled?(req) ? nil : RequestHandler.new(req).handle
end

https://github.com/vcr/vcr/blob/cde89346aebcbe5280299ba10aedb3d97860c557/lib/vcr/library_hooks/webmock.rb#L134-L136

In turn, WM in @webMockNetHTTP:

        def request(request, body = nil, &block)
          # ...
          if webmock_response = WebMock::StubRegistry.instance.response_for_request(request_signature)
            # use stub
          elsif WebMock.net_connect_allowed?(request_signature.uri)
            # perform request
          else
            raise WebMock::NetConnectNotAllowedError.new(request_signature)
          end
        end

https://github.com/bblimke/webmock/blob/fc6a2ab897a069d861adbc1c88e51b2cf8aa88ac/lib/webmock/http_lib_adapters/net_http.rb#L71-L108

So request invokes WebMock::StubRegistry.instance.response_for_request(request_signature), which ends up calling VCR's RequestHandler.new(req).handle.

WebMock::StubRegistry.instance.response_for_request(request_signature) may return a stubbed response when:

or it may return a fals-y value (nil) and let WM perform the request. Remember that net_connect_allowed? is always true when using VCR.

Here's how handle does it:

    def handle
      # ...
      req_type = request_type(:consume_stub)
      # ...
      send "on_#{req_type}_request"
    end

https://github.com/vcr/vcr/blob/cde89346aebcbe5280299ba10aedb3d97860c557/lib/vcr/request_handler.rb#L6-L25

Where

    def request_type(consume_stub = false)
      case
        when externally_stubbed?                then :externally_stubbed # if stubbed by WM, let WM return the stubbed response
        when should_ignore?                     then :ignored # if ignored by VCR, let WM perform the request and record the cassette
        when has_response_stub?(consume_stub)   then :stubbed_by_vcr # let WM use the VCR cassette
        when VCR.real_http_connections_allowed? then :recordable # let WM perform the request and record the cassette
        else                                         :unhandled # raise
      end
    end

https://github.com/vcr/vcr/blob/cde89346aebcbe5280299ba10aedb3d97860c557/lib/vcr/request_handler.rb#L33-L41

        def externally_stubbed? # WM stubbed the request?
          # prevent infinite recursion...
          VCR::LibraryHooks::WebMock.with_global_hook_disabled(request) do
            ::WebMock.registered_request?(request)
          end
        end

https://github.com/vcr/vcr/blob/cde89346aebcbe5280299ba10aedb3d97860c557/lib/vcr/library_hooks/webmock.rb#L97-L102

    def should_ignore?
      disabled? || VCR.request_ignorer.ignore?(vcr_request)
    end

    def disabled?
      VCR.library_hooks.disabled?(library_name) # always false when using WM
    end

https://github.com/vcr/vcr/blob/cde89346aebcbe5280299ba10aedb3d97860c557/lib/vcr/request_handler.rb#L58-L64

        def on_externally_stubbed_request
          # nil allows WebMock to handle the request
          nil
        end

        def on_unhandled_request
          invoke_after_request_hook(nil)
          super # raise unhandled
        end

        def on_stubbed_by_vcr_request
          {
            :body    => stubbed_response.body.dup,
            :status  => [stubbed_response.status.code.to_i, stubbed_response.status.message],
            :headers => stubbed_response.headers
          }
        end

https://github.com/vcr/vcr/blob/cde89346aebcbe5280299ba10aedb3d97860c557/lib/vcr/library_hooks/webmock.rb#L113-L129

    def on_ignored_request
      # nil allows WebMock to handle the request
      nil
    end

    def on_recordable_request
      # nil allows WebMock to handle the request
      nil
    end

https://github.com/vcr/vcr/blob/cde89346aebcbe5280299ba10aedb3d97860c557/lib/vcr/request_handler.rb#L87-L94

Checklist reminder