Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add #unstub method, replace deprecated watir-webdriver with watir #212

Merged
merged 2 commits into from
Mar 5, 2018

Conversation

bwilczek
Copy link
Contributor

@bwilczek bwilczek commented Jan 11, 2018

In some test cases, it is required to dynamically decide which requests should be stubbed, and which not. Using proxy.reset is not the best way to it since it provides no control over which stubs are to be removed.

This PR adds a new method: proxy.unstub, which acts as a reverse to proxy.stub, and removes only a single stub.

See spec/lib/billy/handlers/stub_handler_spec.rb:66 for examples.

Additionally, this PR removes dependency watir-webdriver. That gem was outdated, deprecated and in fact already has already been removed from GitHub. It is now a part of watir gem, and this one has been added instead.

watir-webdriver was throwing a ton of warnings (Fixnum) and an exception on just require 'watir-webdriver' making work on this change impossible.

@ronwsmith
Copy link
Collaborator

@bwilczek thanks for the contribution! Looks like the watir upgrade requires us to finally drop support for ruby 1.93. Mind updating the travis config to remove that and add in the more recent ruby versions?

@ronwsmith
Copy link
Collaborator

@bwilczek can you fix up the conflicts here? I just merged a couple smaller PRs in preparation for this breaking change.

 * Add unstub method that acts as opposite of stub
 * Replace unsupported watir-webdriver with watir
 * Drop support for Ruby 1.9.3, add newer versions instead
@bwilczek
Copy link
Contributor Author

bwilczek commented Feb 9, 2018

@ronwsmith
done: fork freshly rebased onto master, updated list of supported ruby versions.

@@ -34,6 +34,11 @@ def stub(url, options = {})
new_stub
end

def unstub(url, options = {:method => :get})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should unstub be given a previously created stub, rather than providing the values to search for one? 🤔

stub = proxy.stub(...)
proxy.unstub(stub)

@AlanFoster
Copy link
Contributor

@bwilczek I wonder if it's easier to conditionally add stubs to the proxy, rather than conditionally remove stubs? I'd be interested to see an example scenario of this new api being applied :)

@bwilczek
Copy link
Contributor Author

bwilczek commented Feb 19, 2018

@AlanFoster Here's an example :)

Let's consider a suite of WebDriver tests running in the same browser.
The application under test consists of JS-heavy frontend, and few backend services.
One of them is search, which is slow and heavy, and for most of the scenarios
requests to it are stubbed out with Puffing Billy.

But, there are some tests (tagged search) which verify integration with the real service:

def stub_seach
  Billy.proxy.stub("#{SEARCH_HOST}/query/...").and_return(
    {some: 'fake objects'}
  )
end

def unstub_search
  Billy.proxy.unstub("#{SEARCH_HOST}/query/...")
end

RSpec.configure do |c|
  c.before(:suite) do
    stub_seach
  end

  c.around(:each, search: true) do |example|
    unstub_search
    example.run
    stub_search
  end
end

Not sure if the API suggested in one of the previous comments would me more elegant though:

def stub_seach
  @search_stub = Billy.proxy.stub("#{SEARCH_HOST}/query/...").and_return(
    {some: 'fake objects'}
  )
end

def unstub_search
  Billy.proxy.unstub(@search_stub)
end

RSpec.configure do |c|
  c.before(:suite) do
    stub_seach
  end

  c.around(:each, search: true) do |example|
    unstub_search
    example.run
    stub_search
  end
end

Hope the example is clear.

@AlanFoster
Copy link
Contributor

AlanFoster commented Feb 19, 2018

@bwilczek Interesting! Won't Puffing Billy's default rspec configuration cause you issues?

RSpec.configure do |config|
config.include(Billy::RspecHelper)
config.after(:each) do
proxy.reset
end
end

Although - I guess you're not making use of that, if you're referencing Billy.proxy directly? 🤔

@bwilczek
Copy link
Contributor Author

@AlanFoster
Yes, that's my case: we're explicitly setting Billy as the HTTP(S) proxy for the browser in our test suite.

And regarding the API:

stub = proxy.stub(...)
proxy.unstub(stub)

the more I think about it the more I like this approach, I'll test it and implement it if practical.

@AlanFoster
Copy link
Contributor

AlanFoster commented Feb 19, 2018

@bwilczek For that scenario what are your thoughts on something like:

require 'billy/capybara/rspec'

def stub_seach
  Billy.proxy.stub("#{SEARCH_HOST}/query/...").and_return(...)
end

RSpec.configure do |config|
  config.before(:each, type: :feature) do |example|
    stub_search unless example.metadata.key? :with_real_search_functionality
  end
end

That would give you the functionality you're after at the minute I think 🤔

@bwilczek
Copy link
Contributor Author

@AlanFoster Your example would work as long as all stubs are reset after each scenario. Given that we use Billy as a regular browser proxy once the stubs are set they will remain there and affect the consecutive tests. In fact, we use cucumber for this, I used rspec here as an example because it's much nicer :)

BTW: I've updated the unstub method to accept the stub (not URL/method anymore) as a parameter.

@ronwsmith
Copy link
Collaborator

@bwilczek @AlanFoster I haven't been following your back and forth closely. Is this PR ready for review, or are you still working through some ideas?

@bwilczek
Copy link
Contributor Author

bwilczek commented Mar 1, 2018

@ronwsmith It is done from my perspective, so as long as @AlanFoster is OK with the changes the PR is ready for review/merge.

@AlanFoster
Copy link
Contributor

:shipit:

@ronwsmith ronwsmith merged commit 15ebb90 into oesmith:master Mar 5, 2018
@ronwsmith
Copy link
Collaborator

Released with 1.0.0

gem.add_runtime_dependency 'eventmachine', '1.2.0.1'
gem.add_development_dependency 'watir', '~> 6.10.0'
gem.add_runtime_dependency 'addressable', '~> 2.5'
gem.add_runtime_dependency 'eventmachine', '~> 1.0.4'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ronwsmith / @bwilczek Was this version change intentional? 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe watir required it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I believe once I changed watir-webdriver to watir I had to change versions of certain other gems - it wouldn't work otherwise. But I don't remember the exact details now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants