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

Weirdness when using apartment with Capybara JS feature specs #444

Closed
tomdracz opened this Issue Jun 27, 2017 · 13 comments

Comments

Projects
None yet
5 participants
@tomdracz

tomdracz commented Jun 27, 2017

I'm having massive issues with apartment gem while testing js enabled feature specs with Capybara and Poltergeist (or Selenium for that matter).
Basically, in the spec, every time visit page is triggered, the schema search path resets to public. My tenant schema is "test_site" in the specs. Switch to that tenant is set in before hook in rails_helper.rb
See my spec file below with comments

require "rails_helper"

feature "Images", js: true do
  before(:each) do
    pw = "pw"
    @user = create(:user, password: pw, password_confirmation: pw)
    login_as(@user)  #Warden test helper, doesn't trigger schema change. It does trigger if trying to navigate to login page and filling in the data there though
  end

  scenario "lists images" do
    create(:image)
    create(:image)
    puts Image.connection.schema_search_path #returns "test_site", "shared_extensions"
    visit(admin_images_path)
    puts Image.connection.schema_search_path #returns "public", "shared_extensions"
    Image.all.count  #returns 0
    expect(page).to have_selector("img", count: 2)  #passes
  end
end

If I put debugger in the page rendered by admin_images_path, the schema search path there correctly shows "test_site", records I'm looking for are there put since spec changes to public schema, the count shows at 0.

No issues while using Capybara without javascript driver though.

First I thought issue might be devise, but using that just suffers from the same problem, any page visits resets schema_search_path. Tried using my elevator middleware in different order of the stack but nothing seems to work.

Anyone experienced anything similar?

@mikecmpbll

This comment has been minimized.

Collaborator

mikecmpbll commented Jun 27, 2017

i'm not clear what you're describing here. what middleware are you using, what subdomain/host is capybara using? how are you expecting tenant switching to occur in test?

@tomdracz

This comment has been minimized.

tomdracz commented Jun 27, 2017

@mikecmpbll Sorry for being unclear. Hope the below helps and that you don't mind all the info there, I've spent last few days trying to unpick that!
I'm using custom elevator like

class TestElevator < Apartment::Elevators::Generic
  # @return {String} - The tenant to switch to
  def parse_tenant_name(request)
    # request is an instance of Rack::Request

    domain = request.host
    tenant_name = Site.find_by(domain: domain)&.parameterized_name

    tenant_name
  end
end

This is inserted in the middleware before Warden::Manager
Rails.application.config.middleware.insert_before Warden::Manager, ::TestElevator

In the rspec helper I create new Site (and tenant if after_create callback) and switch to it in before(:each) block (domain used is lvh.me, although I tried to set mock one in /etc/hosts but results were the same)

  config.before(:suite) do
    DatabaseCleaner.strategy = :transaction
    DatabaseCleaner.clean_with :truncation

    @site = FactoryGirl.create(:site)
    Capybara.app_host = "http://" + @site.domain + ":33333"
  end

  config.before(:each) do |example|
    DatabaseCleaner.start
    Apartment::Tenant.switch!("test_site")
  end

  config.after(:each) do
    Apartment::Tenant.reset
    DatabaseCleaner.clean
    Warden.test_reset!
  end

As for the middleware stack:

use Rack::Sendfile
use ActionDispatch::Static
use ActionDispatch::Executor
use ActiveSupport::Cache::Strategy::LocalCache::Middleware
use Rack::Runtime
use Rack::MethodOverride
use ActionDispatch::RequestId
use ActionDispatch::RemoteIp
use Rails::Rack::Logger
use ActionDispatch::ShowExceptions
use ActionDispatch::DebugExceptions
use ActionDispatch::Callbacks
use ActionDispatch::Cookies
use ActionDispatch::Session::CookieStore
use ActionDispatch::Flash
use Rack::Head
use Rack::ConditionalGet
use Rack::ETag
use ForestPageElevator
use Warden::Manager
run Dummy::Application.routes

As mentioned, this setup DOES work in non-js specs which do not fire up a real server. With js specs, after each visit in the spec, schema search path reverts to public, but in the view rendered the search path is correct. This has to do with the switch block apartment uses to reset back to previous tenant.

Doing some digging, scanning through backtraces and such I've identified following info:

  • Tenant switching in Capybara used to work when generic elevator was still using switch without block, before change introduced in cfe2bd9 If I try and use that approach, everything works as expected. This made me think problem is in middleware stack somewhere
  • I've started looking at the switch block and what comes back as previous tenant that will be switched back to after request is processed https://github.com/influitive/apartment/blob/development/lib/apartment/adapters/abstract_adapter.rb#L81-L90 In non js specs, the previous tenant assigned is correctly "test_site" while using Capybara js driver, it ends up being "public"
  • From above it seems that Capybara running real server is never aware of the tenant switch. Since it is in the stack before Elevator, that obviously makes sense. This however makes the js feature specs useless if additional database queries are used in assertions (unless I force tenant switch before every possible query, not great)

Since Capybara testing seems to be fairly ubiquitous in Rails world, I was wondering if anyone has hit similar problems before and managed to solve them. Right know, I'm not sure if the problem lies in Capybara starting new thread or actual middleware position in the stack.

Right now the two workaround for the issues I've encountered are:

  • override call method in custom elevator to stop switching back to previous tenant (not perfect, we're switching it back for a reason)
  • force switch tenants before any db queries in the spec done after page visit

I appreciate any thoughts here!

Edit: Hacking elevator as a band-aid until better way is found:

class Elevator < Apartment::Elevators::Generic
  # @return {String} - The tenant to switch to

  def call(env)
    if Rails.env == "test" && env["HTTP_USER_AGENT"]&.match?("PhantomJS")
      request = Rack::Request.new(env)
      database = @processor.call(request)
      Apartment::Tenant.switch! database if database
      @app.call(env)
    else
      super
    end
  end

  def parse_tenant_name(request)
    domain = request.host
    tenant_name = Site.find_by(domain: domain)&.parameterized_name

    tenant_name
  end
end

@mikecmpbll

This comment has been minimized.

Collaborator

mikecmpbll commented Jun 27, 2017

hmm. which database are you using and which switching strategy (config.use_schema = false?). also what is the value of @site.domain in your app_host?

i don't use apartment at all in my application tests so not something i've personally come across.

@tomdracz

This comment has been minimized.

tomdracz commented Jun 27, 2017

config.use_schemas = true - using pg database even in test env
@site.domain returns lvh.me

As mentioned, setup works live and in tests using Rack::Test so the problem lies with Capybara. Backtrace I've been raising shows that even if elevator is inserted before everything else in the stack (config.middleware.insert_before 0, ::Elevator), Capybara specific stuff is there between elevator and Webrick server. This must be causing singular visits to always identify public schema as the current one (or it is due to threading but if my band aid fallback on old switching works, it doesn't seem like it. Can't quite remember now if it's definitely the case, but I think clicking links in tests also resets schema to public.

Above might not necessarily be Apartment issue (although everything worked before cfe2bd9) and might hit Capybara team about that one, it is probably worth mentioning this edge case in readme somewhere. I did try several Capybara drivers with same results.

Happy to provide PR if we don't reach anything else, might save someone the hours it took me to pinpoint this madness!

@mikecmpbll

This comment has been minimized.

Collaborator

mikecmpbll commented Jun 27, 2017

are you using any "connection sharing" code / monkey patch that forces capybara to use the same database connection as your tests, in order, for example, to use transactional fixtures in your capybara tests? also which version of rails?

@tomdracz

This comment has been minimized.

tomdracz commented Jun 28, 2017

I am, yes. With or without it, the results with the schema changes are the same. Rails 5.1

From Apartment standpoint it just seems like the dependence on the order of middleware now messes up the Capybara feature specs and it's time to start looking there for some help.
If I can't hook elevator before Capybara it does its thing, the only way to do it, might be to monkey patch elevator as in my example above to revert to the non-block switch method.

Will try and investigate it on the Capybara side of things and whatever the results will be, I'll fire off PR here with some notes in the readme about possible workarounds.

Thanks for the help @mikecmpbll !

@mikecmpbll

This comment has been minimized.

Collaborator

mikecmpbll commented Jun 28, 2017

if you're forcing the same connection, any changes to the schema_search_path from your capybara requests will affect the tests, so obviously when your request finishes, the middleware switches back to the default tenant ("public"), which is why that's the schema after you issue visit in your tests.

i don't understand what you're getting at with the middleware stuff & capybara. what do you mean "hook elevator 'before capybara does its thing'"?

edit—woops, just read 5.1. yeah, rails 5.1 ships with the connection sharing for acceptance tests baked in: http://edgeguides.rubyonrails.org/5_1_release_notes.html#transactional-tests-with-multiple-connections

the only way around it is to disable transactional tests.

@tomdracz

This comment has been minimized.

tomdracz commented Jun 28, 2017

I understand that point, but if I force tenant switch before the visit to say "test_site", surely after the request it should switch to the previous tenant which will be "test_site" I've set.
Or am I missing the point here? This is how it works with non-js Capybara driver. Consider very simple login helper I've got and see what happens with schema_search_path at each point of the way when it's called:

def login(email, password)
  puts ActiveRecord::Base.connection.schema_search_path # "test_site" as it was set before the test
  visit new_user_session_path
  puts ActiveRecord::Base.connection.schema_search_path  # resets to "public" here
  Apartment::Tenant.switch! "test_site"
  puts ActiveRecord::Base.connection.schema_search_path # "test_site" as it was forced 
  fill_in "Email", with: email
  fill_in "Password", with: password
  click_button "Log in"
  puts ActiveRecord::Base.connection.schema_search_path # back to "public"
end

With non-js driver, schema_search_path is "test_site". Makes sense now?

As for the middleware, I'm wondering if it's a similar thing to the one that we should insert elevator before Warden::Manager to make them play nice together. Capybara seems to be having some custom stuff inserted in the stack in the tests so I'm thinking it causes the requests to be tenant-agnostic to start off with, that's why it always resets back to public.

@mikecmpbll

This comment has been minimized.

Collaborator

mikecmpbll commented Jun 28, 2017

i don't think the middleware stuff is a relevant angle, though I could be wrong. this longform discussion isn't really conducive to debugging, if you want to join me in #apartment on freenode i'll be happy to try and help get to the bottom of this.

@mikecmpbll

This comment has been minimized.

Collaborator

mikecmpbll commented Jun 28, 2017

synopsis:

capybara does it's stuff in a separate thread to the main test thread. due to connection sharing in rails 5.1, both threads use the same database connection. apartment instantiates a different adapter object per thread, and the capybara adapter doesn't receive the switch! issued during test bootstrapping, so as far as it's concerned the "current" tenant is still the default_tenant 'public' (despite the main test thread, and main test apartment adapter, changing the "schema_search_path" for the shared connection).

  1. capybara issues a request to the app
  2. middleware switches tenant to "test_site" as per elevator
  3. then switches back to "previous_tenant" after request, which as far as the capybara thread thinks, is 'public'
  4. test uses same connection, so capybara thread has set the global database connect to the 'public' tenant, hence it's switched on test thread too.

possible solutions:

  • set config.default_tenant = "test_site" during Apartment initialization
  • somehow issue a switch! on the capybara thread outside of a request during test bootstrapping
  • set Thread.local[:apartment_adapter] for all threads to the same as the main test thread (to force the same apartment adpater, like rails forces the same database connection)

@mikecmpbll mikecmpbll closed this Jun 28, 2017

@kpheasey

This comment has been minimized.

kpheasey commented Nov 29, 2017

Just spent a bunch of time trying to get my existing capybara/selenium feature specs working after implementing apartment. I found a gist that will make capybara use a single connection and allow DatabaseCleaner.strategy = :transaction.

https://gist.github.com/josevalim/470808

# frozen_string_literal: true

require 'database_cleaner'

# Forces all threads to share the same connection. This works on
# Capybara because it starts the web server in a thread.
class ActiveRecord::Base
  mattr_accessor :shared_connection
  @@shared_connection = nil

  def self.connection
    @@shared_connection || retrieve_connection
  end
end

RSpec.configure do |config|
  config.use_transactional_fixtures = false

  config.before(:suite) do
    # Clean all tables to start
    DatabaseCleaner.clean_with :truncation
    # Use transactions for tests
    DatabaseCleaner.strategy = :transaction
    # Truncating doesn't drop schemas, ensure we're clean here, app *may not* exist
    Apartment::Tenant.drop('app') rescue nil
    # Create the default tenant for our tests
    Company.create!(name: 'App', subdomain: 'app')
  end

  config.before(:each) do
    # Start transaction for this test
    DatabaseCleaner.start
    # Switch into the default tenant
    Apartment::Tenant.switch! 'app'
  end

  config.before(:each, type: :feature) do
    ActiveRecord::Base.shared_connection = ActiveRecord::Base.connection
  end

  config.after(:each) do
    # Reset tenant back to `public`
    Apartment::Tenant.reset rescue nil
    # Rollback transaction
    DatabaseCleaner.clean
  end

  config.after(:each, type: :feature) do
    ActiveRecord::Base.shared_connection = nil
  end
end
@rfdavid

This comment has been minimized.

rfdavid commented Feb 24, 2018

I know it's been a time but I'm also facing difficulties with apartment and Capybara, the same issue you guys were discussing a year ago. The hacking for the elevator middleware works but I'm wondering if there is something better. @tomdracz have you found a better solution?

MinasMazar added a commit to MinasMazar/nebulab_shop that referenced this issue Sep 26, 2018

RSpec & multitenancy. Preferences test should fail
Due to the `Capybara-shared-connection` issue, we cannot provide
multitenant feature specs.
See influitive/apartment#444

.......

Include rspec-rails gem and install with:
$ rails g rspec:install

*also add standard Rails test for preferences (skipped)

MinasMazar added a commit to MinasMazar/nebulab_shop that referenced this issue Sep 27, 2018

RSpec & multitenancy. Preferences test should fail
Due to the `Capybara-shared-connection` issue, we cannot provide
multitenant feature specs.
See influitive/apartment#444

.......

Include rspec-rails gem and install with:
$ rails g rspec:install

*also add standard Rails test for preferences (skipped)

MinasMazar added a commit to MinasMazar/nebulab_shop that referenced this issue Sep 27, 2018

Test on 'test_shop' tenant.
From Apartment wiki
https://github.com/influitive/apartment/wiki/Testing-Your-Application

> Create a single tenant before the whole test suite runs
> and switch to that tenant for the run of your tests.

Cannot swtich tenant with Capybara
see influitive/apartment#444

> Transactional fixtures do not work with Selenium tests,
> because Capybara uses a separate server thread,
> which the transactions would be hidden from.
> We hence use DatabaseCleaner to truncate our test database.

MinasMazar added a commit to MinasMazar/nebulab_shop that referenced this issue Sep 28, 2018

RSpec & multitenancy. Preferences test should fail
Due to the `Capybara-shared-connection` issue, we cannot provide
multitenant feature specs.
See influitive/apartment#444
Tried this, but seems not working... :(

.......

Include rspec-rails gem and install with:
$ rails g rspec:install

*also add standard Rails test for preferences (skipped)

MinasMazar added a commit to MinasMazar/nebulab_shop that referenced this issue Sep 28, 2018

Test on 'test_shop' tenant.
From Apartment wiki
https://github.com/influitive/apartment/wiki/Testing-Your-Application

> Create a single tenant before the whole test suite runs
> and switch to that tenant for the run of your tests.

Cannot swtich tenant with Capybara
see influitive/apartment#444

> Transactional fixtures do not work with Selenium tests,
> because Capybara uses a separate server thread,
> which the transactions would be hidden from.
> We hence use DatabaseCleaner to truncate our test database.

MinasMazar added a commit to MinasMazar/nebulab_shop that referenced this issue Oct 2, 2018

RSpec & multitenancy. Preferences test should fail
Due to the `Capybara-shared-connection` issue, we cannot test
tenants with feature specs.
See influitive/apartment#444

> Transactional fixtures do not work with Selenium tests,
> because Capybara uses a separate server thread,
> which the transactions would be hidden from.
> We hence use DatabaseCleaner to truncate our test database.

+ also add standard Rails test for preferences (skipped)

Local testing in development env should display wrong
email in "Contact us at" section.

.......

$ rails g rspec:install

MinasMazar added a commit to MinasMazar/nebulab_shop that referenced this issue Oct 5, 2018

RSpec & multitenancy. Preferences test should fail
Due to the `Capybara-shared-connection` issue, we cannot test
tenants with feature specs.
See influitive/apartment#444

> Transactional fixtures do not work with Selenium tests,
> because Capybara uses a separate server thread,
> which the transactions would be hidden from.
> We hence use DatabaseCleaner to truncate our test database.

+ also add standard Rails test for preferences (skipped)

Local testing in development env should display wrong
email in "Contact us at" section.

.......

$ rails g rspec:install

MinasMazar added a commit to MinasMazar/nebulab_shop that referenced this issue Oct 5, 2018

RSpec & multitenancy. Preferences test should fail
Due to the `Capybara-shared-connection` issue, we cannot test
tenants with feature specs.
See influitive/apartment#444

> Transactional fixtures do not work with Selenium tests,
> because Capybara uses a separate server thread,
> which the transactions would be hidden from.
> We hence use DatabaseCleaner to truncate our test database.

+ also add standard Rails test for preferences (skipped)

Local testing in development env should display wrong
email in "Contact us at" section.

.......

$ rails g rspec:install

MinasMazar added a commit to MinasMazar/nebulab_shop that referenced this issue Oct 5, 2018

RSpec & multitenancy. Preferences test should fail
Due to the `Capybara-shared-connection` issue, we cannot test
tenants with feature specs.
See influitive/apartment#444

> Transactional fixtures do not work with Selenium tests,
> because Capybara uses a separate server thread,
> which the transactions would be hidden from.
> We hence use DatabaseCleaner to truncate our test database.

+ also add standard Rails test for preferences (skipped)

Local testing in development env should display wrong
email in "Contact us at" section.

.......

$ rails g rspec:install

MinasMazar added a commit to MinasMazar/nebulab_shop that referenced this issue Oct 5, 2018

RSpec & multitenancy. Preferences test should fail
$ rails g rspec:install

Due to the `Capybara-shared-connection` issue, we cannot test
tenants with feature specs.
See influitive/apartment#444

> Transactional fixtures do not work with Selenium tests,
> because Capybara uses a separate server thread,
> which the transactions would be hidden from.
> We hence use DatabaseCleaner to truncate our test database.

+ also add standard Rails test for preferences (skipped)

Local testing in development env should display wrong
email in "Contact us at" section.
@iSarCasm

This comment has been minimized.

iSarCasm commented Oct 12, 2018

Why the issues is closed if this is not resolved?

MinasMazar added a commit to MinasMazar/nebulab_shop that referenced this issue Oct 12, 2018

RSpec & multitenancy. Preferences test should fail
$ rails g rspec:install

Due to the `Capybara-shared-connection` issue, we cannot test
tenants with feature specs.
See influitive/apartment#444

> Transactional fixtures do not work with Selenium tests,
> because Capybara uses a separate server thread,
> which the transactions would be hidden from.
> We hence use DatabaseCleaner to truncate our test database.

+ also add standard Rails test for preferences (skipped)

Local testing in development env should display wrong
email in "Contact us at" section.

MinasMazar added a commit to MinasMazar/nebulab_shop that referenced this issue Oct 19, 2018

RSpec & multitenancy. Preferences test should fail
$ rails g rspec:install

Due to the `Capybara-shared-connection` issue, we cannot test
tenants with feature specs.
See influitive/apartment#444

> Transactional fixtures do not work with Selenium tests,
> because Capybara uses a separate server thread,
> which the transactions would be hidden from.
> We hence use DatabaseCleaner to truncate our test database.

+ also add standard Rails test for preferences (skipped)

Local testing in development env should display wrong
email in "Contact us at" section.

MinasMazar added a commit to nebulab/multitenant_shop that referenced this issue Oct 26, 2018

Install and setup RSpec
        $ rails g rspec:install

+ Test on single tenant

Due to the `Capybara-shared-connection` issue, we cannot test
tenants with feature specs. See influitive/apartment#444

> Transactional fixtures do not work with Selenium tests,
> because Capybara uses a separate server thread,
> which the transactions would be hidden from.
> We hence use DatabaseCleaner to truncate our test database.

From Apartment wiki
https://github.com/influitive/apartment/wiki/Testing-Your-Application

> Create a single tenant before the whole test suite runs
> and switch to that tenant for the run of your tests.

+ Install and setup database-cleaner gem

see apartment's wiki on how to configure it
https://github.com/influitive/apartment/wiki/Testing-Your-Application

MinasMazar added a commit to nebulab/multitenant_shop that referenced this issue Oct 26, 2018

Install and setup RSpec
        $ rails g rspec:install

+ Test on single tenant

Due to the `Capybara-shared-connection` issue, we cannot test
tenants with feature specs. See influitive/apartment#444

> Transactional fixtures do not work with Selenium tests,
> because Capybara uses a separate server thread,
> which the transactions would be hidden from.
> We hence use DatabaseCleaner to truncate our test database.

From Apartment wiki
https://github.com/influitive/apartment/wiki/Testing-Your-Application

> Create a single tenant before the whole test suite runs
> and switch to that tenant for the run of your tests.

+ Install and setup database-cleaner gem

see apartment's wiki on how to configure it
https://github.com/influitive/apartment/wiki/Testing-Your-Application

MinasMazar added a commit to nebulab/multitenant_shop that referenced this issue Oct 26, 2018

Install and setup RSpec
        $ rails g rspec:install

+ Test on single tenant

Due to the `Capybara-shared-connection` issue, we cannot test
tenants with feature specs. See influitive/apartment#444

> Transactional fixtures do not work with Selenium tests,
> because Capybara uses a separate server thread,
> which the transactions would be hidden from.
> We hence use DatabaseCleaner to truncate our test database.

From Apartment wiki
https://github.com/influitive/apartment/wiki/Testing-Your-Application

> Create a single tenant before the whole test suite runs
> and switch to that tenant for the run of your tests.

+ Install and setup database-cleaner gem

see apartment's wiki on how to configure it
https://github.com/influitive/apartment/wiki/Testing-Your-Application
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment