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

Triplex.create is too slow for tests #47

Open
dustinfarris opened this issue Nov 14, 2017 · 16 comments
Open

Triplex.create is too slow for tests #47

dustinfarris opened this issue Nov 14, 2017 · 16 comments

Comments

@dustinfarris
Copy link
Contributor

I'm trying to figure out how to incorporate Triplex into my unit tests.

Right now, I have Triplex.create :test in my setup function, but this has exponentially increased the length of time it takes to run the tests. Is there a better way?

@dustinfarris
Copy link
Contributor Author

I figured this one out. Had to create the tenant in test/test_helper.exs (note this has to be done before setting the sandbox to manual). e.g.

# in test/test_helper.exs
Triplex.create "test_tenant"
Ecto.Adapters.SQL.Sandbox.mode(IM.Repo, :manual)

@kelvinst
Copy link
Contributor

This is something we really need to improve, or at least give a tool to solve the long tests problem.

I've also got to the point of creating a tenant before executing the tests and using it for all tests, but I really don't think it's the best solution.

We could do something like a mix task to generate a dump file which will be used to load the database structure whenever a new tenant is created, so instead of running all migrations (which takes time) we would go straight to the final desired state. Maybe this solves the performance problem on tenant creation.

I've not studied this solution enough, but anyways, I will reopen your issue and rename it! Thanks for the report.

@kelvinst kelvinst reopened this Nov 14, 2017
@kelvinst kelvinst changed the title Recommended testing best practice? Triplex.create is too slow for tests Nov 14, 2017
@vinceurag
Copy link

vinceurag commented Nov 17, 2017

I'm also using that method @dustinfarris. I created a TenantCase.

@churcho
Copy link

churcho commented Feb 13, 2019

Has there been any traction on this?
I would like to test a pipeline which has a tenant create step and it doesn't fire.
I get (DBConnection.ConnectionError) connection not available and request was dropped from queue after..

@humancopy
Copy link

Great solution: https://elixirforum.com/t/using-setup-all-with-database/8865/5

@kelvinst
Copy link
Contributor

Yeah, that's pretty much what we suggested to some people, but it still a solution to a problem that could not exist at all. There are some techniques we are trying out to reduce the time it takes to create tenants, mostly because the tests are not the only ones affected by the time this takes today, the slowness can happen on production too.

@danieldocki
Copy link

Have any solution for this? I got error below

(MatchError) no match of right hand side value: {:error, "ERROR 3F000 (invalid_schema_name) schema \"tenant_test\" does not exist"}

@kelvinst
Copy link
Contributor

Well, not sure if I can help by just looking at the error you linked. What function were you calling? Do you have the callstack? What's the versions of elixir and triplex on your machine?

@danieldocki
Copy link

I changed my TenantCase and worked.

defmodule TenantCase do
  use ExUnit.CaseTemplate

  setup_all do
    :ok = Ecto.Adapters.SQL.Sandbox.checkout(Repo)
    Ecto.Adapters.SQL.Sandbox.mode(Repo, :auto)
+   if Triplex.exists?("test"), do: Triplex.drop("test")
    {:ok, tenant} = Triplex.create("test")
    
-   on_exit(fn ->
-     :ok = Ecto.Adapters.SQL.Sandbox.checkout(Repo)
-     Ecto.Adapters.SQL.Sandbox.mode(Repo, :auto)
-
-     Triplex.drop("tenant_test")
-     :ok
-   end)


    [tenant: tenant]
  end
end

@sveredyuk
Copy link

It works fine for schema tests (like"test"), but tests using public schema are still slow. Why?

my tenant_case.ex:

defmodule App.TenantCase do
  use ExUnit.CaseTemplate

  alias App.Repo

  setup_all do
    :ok = Ecto.Adapters.SQL.Sandbox.checkout(Repo)
    Ecto.Adapters.SQL.Sandbox.mode(Repo, :auto)
    if Triplex.exists?("test"), do: Triplex.drop("test")
    {:ok, tenant} = Triplex.create("test")

    [tenant: tenant]
  end
end

@thiagomajesk
Copy link

thiagomajesk commented Sep 29, 2021

Hi @kelvinst! This has become very problematic in an application I'm working on for the past two years...

We now have 431 tests that are taking almost 2 minutes to run. We can explicitly see Triplex hanging before each test case, which slows the whole process down from development to our delivery.

With the TenantCase approach, besides the obvious drawbacks in performance, we also have to drop any records in the public schema because of how Ecto.Adapters.SQL.Sandbox.mode(Repo, :auto) works; which makes our test cases more complex than they should be. I Also tried @dustinfarris's solution to create the tenant before, but we still have some major problems with the test sandbox which makes me think this alternative is not scalable at all.

I've been meaning to ask, will this be addressed in the near future or are there plans to work on it!? I wanted to check with you first before start studying the possibility to change how we handle tenants in our application.

@kelvinst
Copy link
Contributor

kelvinst commented Oct 4, 2021

Hello @thiagomajesk. There is no plan for this in the near future. But feel free to investigate it further and suggest options for it here.

One option for TenantCase would be to seed the database with some tenants before the whole test suite and use those on your tests.

@thiagomajesk
Copy link

Hi @kelvinst! Thanks for the response. I've checked this possibility beforehand and it indeed helps speed up the test setup phase. Although not perfect as you mentioned before in this thread, it helps. However, the problems start arising for more complex scenarios.

Because there wasn't an immediate (and obvious) solution for this I ended up rewriting the whole test suite to remove special cases that needed to interact with ecto's sandbox for some reason and managed to cut the time to be acceptable for development purposes.

But feel free to investigate it further and suggest options for it here.

Besides the open issues, do you have a roadmap for future versions somewhere? I figure this could help align the common objective with people meaning to contribute to the project.

@kelvinst
Copy link
Contributor

kelvinst commented Oct 4, 2021

Besides the open issues, do you have a roadmap for future versions somewhere?

Nope, the open issues are the roadmap basically. But I'm planning to get some time this quarter to build a new major release for triplex, as there are multiple breaking changes that we want to release (not promising anything though).

So if you manage to have any idea of how to solve this problem that you want to try out, let's discuss it here, and maybe work on a PR for that.

I want to try out the dump/load strategy that I mentioned a couple years ago too. Let's see if I get any free time someday, but if you have other ideas or want to try that out as well, feel free to do so!

@Valian
Copy link

Valian commented Jan 27, 2022

Hello everyone! I'm a bit late to the party, but I think I possibly managed to find a reasonable solution. Heavily inspired by https://elixirforum.com/t/using-setup-all-with-database/8865/5. I think it's something @dustinfarris suggested initially, just slightly improved to allow asynchronous tests and also ones expecting no tenant.

The assumption is that in a multi-tenant application there will be much more tenant-specific tests than not. So I tried to optimize for that case. I'm creating a shared tenant in test_helper.ex, and then in tenant-specific tests I'm simply using it, otherwise I'm removing it in setup. Removing is much faster than creating, and is done less often. Can be done asynchronously, since transaction will be rolled back after each test leaving us in a pristine state.

More or less:

# test_helpers.exs
ExUnit.start()

# creating shared tenant for all tests
# it's disabled for tests where we don't expect tenant to be available
MyApp.Tenants.list_tenants() |> Enum.each(&MyApp.Tenants.delete_tenant/1)
# runs Triplex.create under the hood.
{:ok, _tenant} = MyApp.Tenants.create_tenant(%{name: "test_tenant"})


Ecto.Adapters.SQL.Sandbox.mode(MyApp.Repo, :manual)
# data_case.exs
defmodule MyApp.DataCase do
  use ExUnit.CaseTemplate
  # ...
  setup tags do
    pid = Ecto.Adapters.SQL.Sandbox.start_owner!(MyApp.Repo, shared: not tags[:async])
    # Tenant is created in test_helper to be shared across all TenantCase tests
    # DataCase doesn't expect Tenant to be there, so we're simply removing it in transaction.
    # It will be automatically rolled back
    MyApp.Repo.delete_all(MyApp.Tenants.Tenant)
    on_exit(fn -> Ecto.Adapters.SQL.Sandbox.stop_owner(pid) end)
    :ok
  end
end
# tenant_case.exs
defmodule MyApp.TenantCase do
  use ExUnit.CaseTemplate

  # ...
  setup_all do
    # checkin and checkout to fetch tenant once for all tests, and not in each test separately
    :ok = Ecto.Adapters.SQL.Sandbox.checkout(MyApp.Repo)
    tenant = MyApp.Repo.one(MyApp.Tenants.Tenant)
    :ok = Ecto.Adapters.SQL.Sandbox.checkin(MyApp.Repo)
    [tenant: tenant]
  end
end

Usage:

defmodule MyApp.UsersTest do
  use MyApp.TenantCase, async: true
  
  test "tenant is available", %{tenant: tenant} do
      assert tenant
  end

Hope it could be helpful for someone.

@thiagomajesk
Copy link

Hi @Valian, thanks for your contribution... This is essentially what I've been doing today - except the drop tenant part. The thing is that doing the tenant set up as a test case is indeed impractical (relative to just creating a transaction or snapshot of the database at least). Cheers!

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

No branches or pull requests

9 participants