oesmith / puffing-billy

A rewriting web proxy for testing interactions between your browser and external sites. Works with ruby + rspec.
MIT License
656 stars 170 forks source link

RSpec puffing billy/capybara ordering issue #226

Closed AlanFoster closed 6 years ago

AlanFoster commented 6 years ago

I noticed that for RSpec the default puffing billy reset hook happens before Capybara calls a reset.

For instance this is the hook order that I can see being generated for our test set up:

capybara-screenshot-1.0.17/lib/capybara-screenshot/rspec.rb:80
puffing-billy-0.12.0/lib/billy/init/rspec.rb:12       <---
vcr-2.9.3/lib/vcr/test_frameworks/rspec.rb:38
vcr-2.9.3/lib/vcr/test_frameworks/rspec.rb:25
vcr-2.9.3/lib/vcr/test_frameworks/rspec.rb:25
webmock-3.1.0/lib/webmock/rspec.rb:30
capybara-2.13.0/lib/capybara/rspec.rb:19              <---

From the above, we can see that puffing billy's reset happens before capybara's reset implementation:

https://github.com/teamcapybara/capybara/blob/b67059a57a87aeb7b2ca957f19805b4ac30b8197/lib/capybara/rspec.rb#L16-L21

That means there's a period of time where the browser could be making requests to puffing billy that are not stubbed correctly.

I wonder if the implementation should instead be:

RSpec.configure do |config|
  config.include(Billy::RspecHelper)

-  config.after(:each) do
+  config.prepend_after(:each) do
    proxy.reset
  end
end

This will place Puffing Billy's reset mechanism after Capybara's reset and avoid any race conditions:

capybara-screenshot-1.0.17/lib/capybara-screenshot/rspec.rb:80
vcr-2.9.3/lib/vcr/test_frameworks/rspec.rb:38
vcr-2.9.3/lib/vcr/test_frameworks/rspec.rb:25
vcr-2.9.3/lib/vcr/test_frameworks/rspec.rb:25
webmock-3.1.0/lib/webmock/rspec.rb:30
capybara-2.13.0/lib/capybara/rspec.rb:19              <---
puffing-billy-0.12.0/lib/billy/init/rspec.rb:12       <---

Note: I was able to naively grab the hook ordering information with:

RSpec.configuration.hooks
  .instance_variable_get(:@after_example_hooks)
  .items_and_filters
  .map { |(item, metadata)| item.block.source_location }
ronwsmith commented 6 years ago

Now that's an edge case! Your suggestion seems like it'll work just fine. Please submit a PR.

AlanFoster commented 6 years ago

@ronwsmith Will do :+1: