archtechx / tenancy

Automatic multi-tenancy for Laravel. No code changes needed.
https://tenancyforlaravel.com
MIT License
3.66k stars 431 forks source link

[4.x] Testing tenants is slow #250

Closed antonkomarev closed 1 year ago

antonkomarev commented 4 years ago

Accordingly to the documentation testing of tenants could be done this way:

<?php

namespace Tests;

use Illuminate\Foundation\Testing\TestCase as BaseTestCase;

abstract class TestCase extends BaseTestCase
{
    use CreatesApplication;

    protected $tenancy = false;

    public function setUp(): void
    {
        if ($this->tenancy) {
            $this->initializeTenancy();
        }
    }

    public function initializeTenancy($domain = 'test.localhost')
    {
        tenancy()->create($domain);
        tenancy()->init($domain);
    }

    public function tearDown(): void
    {
        config([
            'tenancy.queue_database_deletion' => false,
            'tenancy.delete_database_after_tenant_deletion' => true,
        ]);
        tenancy()->all()->each->delete();

        parent::tearDown();
    }
}

But if there is a lot of tests - it will be very slow. In my case ~3 seconds for each test with more than 1k tests (~50 minutes).

stancl commented 4 years ago

Yeah, it's on my roadmap to make a few traits like those provided by Laravel. (e.g. to only run migrations on phpunit setup rather than every single test setup).

There could be a trait for tests that don't need to know about tenant creation (because they don't test the central app) and only test parts of the tenant app. That trait could create a tenant & DB on phpunit setup and then cleanup after all tests are run.

drfraker commented 4 years ago

@stancl I wrote a package the hijacks the Refresh Database trait. I'm not sure, but you may be able to find some useful code in there to make this happen. If I get some free time in the upcoming weeks I might just add it myself. This is a major pain point for us right now too.

https://laravel-news.com/snipe-migrations-laravel-package

stancl commented 4 years ago

Hi @drfraker. Good idea, I'll take a look at your package alongside Laravel traits for some inspiration.

drfraker commented 4 years ago

@stancl I also like your idea of a trait that can be added onto test classes to bypass the tenant creation. Like a NoTenants trait. I've also seen where if you add certain keywords to a test name it will do the same thing for more granular control within a test class.

eg. test_it_can_do_something_no_tenants Within the test case this would be picked up and not add tenants to that particular test. It's helpful when some tests in a class use tenants and others do not. A single trait would not be usable in that case.

stancl commented 4 years ago

to bypass the tenant creation

Just don't do it. Don't create any tenants and just run the tests. The use for that would be limited, though.

I think the opposite would be useful. A trait that creates & deletes the tenant before/after phpunit runs the tests. Any calls related to the current tenant (e.g. tenant-specific calls and inline tenant() calls) require a tenant to be created & identified.

And to test tenant creation using your signup form, you'd use no traits.

So you really only need two things:

drfraker commented 4 years ago

Actually I was thinking about it a bit differently. My thought was to have 1 or more databases for testing tenants. Maybe like tenant_1_db and tenant_2_db. These would be set up beforehand by the developer. Like SnipeMigrations, it will run the migrations once then take a snapshot of the databases in the "clean" state. After initial db intialization by importing the snapshot, the databases would use database transactions for each test so that no actual data gets saved to the test tenant databases. If a tenant migration file changes the app would know about it, re-run the tenant migrations and take a new mysql snapshot for future imports. The "central" database would be pre-seeded with the tenants belonging to the 2 pre-created databases, using custom db names. Snipe already allows a seeder to run before the snapshot is taken.

This way we can avoid the slow part which is database creation and running migrations. Of course if a developer needed more than the pre-provisioned number of tenants for testing they could create a new one in the test. Since this isn't a requirement most of the time the tests should run really fast.

Is there anything that would prevent us from going in this direction?

stancl commented 4 years ago

Hmm, I see. I'll take a deeper look at SnipeMigrations. Skipping the central & tenant DB migration/seeding process even on phpunit setup could speed things up considerably.

stancl commented 4 years ago

This might be related: https://github.com/stancl/tenancy/issues/190#issuecomment-559031731

drfraker commented 4 years ago

Hey @stancl I've been looking into how to make testing faster. I think a good start would be to have some flag that when set will tell the database manager that the database already exists and not to try to create one. One of the problems is that the database has to be created/migrated/deleted for every test case, which is slow. If the database does not have to be created we could presumably have a few databases set up for testing like tenant_1_test, tenant_2_test, ... and when creating test tenants simply set the _tenancy_database_name to tenant_1_test etc. If that existed, we could mimic the functionality of the RefreshDatabaseTrait on all tenant databases. Basically, that trait runs migrations and seeds once. Then sets the migrated state in RefreshDatabaseState::$migrated to true and starts a database transaction that rolls back once the test has run. On subsequent tests (when $migrated is true) it skips the migrations and just uses the transactions. If you could get a branch to the point where there was a setting to skip database creation I could probably fill in the rest of the pieces to make this work.

Does this make sense?

stancl commented 4 years ago

There are 3 types of tests:

The third category is what I expect to be the biggest group of tests for all apps that use this package, so let's focus on that.

Assuming the DB storage driver is used, we can:

Later we can do something like Snipe migrations for even faster tests, but this is a good start.

I will have to look into how the Laravel migration traits work, because Application state is not persisted between tests (for obvious reasons), but the transaction persists. My concern is that Application being destroyed between tests could also destroy the tenant connection, which would probably break the transaction. But that shouldn't be a big issue, if RefreshDatabase works, we should be able to make a similar trait.

stancl commented 4 years ago

To add Laravel 7 support faster, I'm putting this off for 2.4.0.

colinmackinlay commented 4 years ago

A technique that I'm going to try to adapt is this one that I used to really speed up tests with hyn/multitenant: https://medium.com/@sadnub/faster-multitenant-testing-in-laravel-4769eae4b603. It made a huge difference.

Before that though I need to figure out how to make my existing test work now that I've converted to stancl/tenancy :)

At the moment my problem is that when using a route from within a test e.g.

        $this->post(route('tenant.site.area.store'),
            ['name' => 'New Location'])
            ->assertRedirect(route('tenant.site.area.index'));

I get a 404 error because the url being called is: https://host.test/site/area where it should be: https://tenant.host.test/site/area

The route works fine in the app but the test doesn't know it's in a tenant even though setup has:

        tenancy()->init('testing.host.test');

Any thoughts anyone?

stancl commented 4 years ago

You can specify the domain that the tenant should be redirected to if you enable the TenantRedirect feature.

return redirect()->route(...)->tenant($domain);

Otherwise the app has no way of knowing what domain to use, since tenants can have multiple domains.

colinmackinlay commented 4 years ago

Thanks for the response. Don't quite follow how that helps in the test. Certainly with that enabled I can get a redirect to a tenant page:

redirect()->route('tenant.site.area.store')->tenant('testing.host.test')

That works. But I can't post to a route modified by tenant which is what the test needs:

post(route('tenant.site.area.store')->tenant('testing.host.test'),['name' => 'New Location']);

Returns an error: Call to a member function tenant() on string

I would have thought the app/test could know which domain I had initialised from tenancy()->init('testing.host.test');

The app itself is fine because when it is live code the request is coming from the tenant domain. It's only the tests that fail, not the code.

stancl commented 4 years ago

redirect()->route() returns something different than route(). route() returns a string. You need tenant_route() for route strings w/ tenant domains swapped.

I would have thought the app/test could know which domain I had initialised from tenancy()->init('testing.host.test');

No, because testing.host.test is resolved to a tenant id, and each tenant can have an infinite amount of domains.

colinmackinlay commented 4 years ago

Thanks - tenant_route() sorted it, I can store the domain in the TenantTestCase and just modify all my tests.

Couldn't find anything in the documentation about the tenant_route() helper but got it in the helpers file.

All the work converting from hyn definitely seems to be worth it though. Great package :)

devoncmather commented 4 years ago

Looks like this has been added to V3 roadmap 👍🏻

In the meantime has anyone had relative success in setting this up on their own to speed up tests? My current test suite takes about 15 mins to run when using sqlite, and on GitHub actions on a mysql db it takes anywhere between 1-5 hours if it doesn't time out first. It's gotten to the point where I would like to try to solve it using @stancl comment from 15 jan as an initial approach. Just wanted to reach out and see if anyone has already had any luck with a solution so far.

Cheers 😊

stancl commented 4 years ago

With v3 coming soon, I'd like to make this part of 3.0 or 3.1.

okihus commented 4 years ago

Any updates on this one?

devonmather commented 4 years ago

I have had luck using mysql databases. This is a stripped-down version of my test case, the brand model is a wrapper around the tenant this is for V2. The second brand created is used for a couple of edge cases (like making sure commands run for multiple tenants). There are caveats with this approach but if there is interest in this solution I can go into a bit more detail. Running 2000+ tests takes about 5 minutes, was close to an 1+ hours before this change.

abstract class TestCase extends BaseTestCase
{
    use CreatesApplication, RefreshDatabase;

    protected $brand;

    protected bool $tenancy = false;

    protected function setUp(): void
    {
        parent::setUp();

        config(['tenancy.database.prefix' => 'tenant_test_']);
        config(['tenancy.filesystem.suffix_base' => 'tenant_test']);

        if ($this->tenancy) {
            $this->initBrand();

            DB::beginTransaction();
        }
    }

    protected function tearDown(): void
    {
        if ($this->tenancy) {
            DB::rollback();
        }

        parent::tearDown();
    }

    public function initBrand()
    {
        $this->brand = Brand::first();

        Brand::init($this->brand->domain);
    }

    /**
     * Override the refresh trait for the conventional test database.
     *
     * @return void
     */
    protected function refreshTestDatabase()
    {
        config(['tenancy.storage_drivers.db.cache_ttl' => 60]);
        config(['tenancy.database.prefix' => 'tenant_test_']);
        config(['tenancy.filesystem.suffix_base' => 'tenant_test']);

        if (!RefreshDatabaseState::$migrated) {
            $this->artisan('migrate');
            $this->artisan('tenants:migrate');

            if (!Brand::first()) {
                factory(Brand::class)->create([
                    'subdomain' => 'tenant-test',
                ]);
            }
            if (!Brand::skip(1)->first()) {
                factory(Brand::class)->create([
                    'subdomain' => 'tenant-test-2',
                ]);
            }

            $this->app[Kernel::class]->setArtisan(null);

            RefreshDatabaseState::$migrated = true;
        }

        $this->beginDatabaseTransaction();
    }
}
stancl commented 4 years ago

Any updates on this one?

There's this: https://sponsors.tenancyforlaravel.com/frictionless-testing-setup

And I'll be adding database transactions to it soon.

lewiswharf commented 3 years ago

I've run into this issue and wonder what's the best approach currently?

stancl commented 3 years ago

See https://github.com/stancl/tenancy/discussions/579

lewiswharf commented 3 years ago

@stancl WOW, I only needed to add the following to my testing MySQL Docker container

    tmpfs:
      - /var/lib/mysql

Thank you!

hackerESQ commented 3 years ago

Is the best practice still reflected in the https://sponsors.tenancyforlaravel.com/frictionless-testing-setup?

It works well.... but it's super slow. By hooking into the setUp (and tearDown) function, it creates a new tenant for each test method. Since I create an S3 bucket and Stripe customer whenever a TenantCreated event is called, this obviously adds some overhead to each test method call. Assuming others have similar Jobs running on the creation of new tenants.

I can't seem to figure out how to create a test tenant once, and clean up (i.e. delete test S3 bucket and test Stripe customer) once all tests are complete. Is this possible?

sinamiandashti commented 3 years ago

@stancl did u add db transactions to it or not ?

hackerESQ commented 3 years ago

Ended up writing my own static method that creates a test tenant once when the first tenant-related test is run. Importantly, this means only one tenant is created.

Additionally, to help with cleanup, this method also keeps track of how many tests need to run (phpunit --list-tests | grep '-' | wc -l), and what the current test is. Then on the last test, it deletes the test tenant (which also triggers the jobs for deleting S3 bucket and Stripe customer).

The trade-off of speed vs. testing "best practice" (i.e. creating a fresh environment/application for each test) is very clear here. The code I use is super specific to my use case, but happy to answer questions on how it works.

If there's enough interest, I might be able to package it up or create a PR to include as a testing helper trait.

sinamiandashti commented 3 years ago

@hackerESQ ur workaround wont work properly in : php artisan test --parallel

hackerESQ commented 3 years ago

@hackerESQ ur workaround wont work properly in : php artisan test --parallel

Another tradeoff that is acceptable for my use case...

erikgaal commented 3 years ago

I was running into this issue as well. I've made a slightly adapted RefreshDatabase trait that works for me. Tests were previously running for more than a second each, with this trait it was reduced back to 300ms (as it was before using multi-database tenancy).

<?php

namespace Tests;

use App\Tenant\Models\Tenant;
use Illuminate\Support\Facades\URL;
use Illuminate\Foundation\Testing\RefreshDatabase;

trait RefreshDatabaseWithTenant
{
    use RefreshDatabase {
        beginDatabaseTransaction as parentBeginDatabaseTransaction;
    }

    /**
     * The database connections that should have transactions.
     *
     * `null` is the default landlord connection
     * `tenant` is the tenant connection
     */
    protected array $connectionsToTransact = [null, 'tenant'];

    /**
     * We need to hook initialize tenancy _before_ we start the database
     * transaction, otherwise it cannot find the tenant connection.
     */
    public function beginDatabaseTransaction()
    {
        $this->initializeTenant();

        $this->parentBeginDatabaseTransaction();
    }

    public function initializeTenant()
    {
        $tenant = Tenant::firstOr(fn () => Tenant::factory()->name('Acme')->create());

        tenancy()->initialize($tenant);

        URL::forceRootUrl('http://acme.localhost');
    }
}
stancl commented 3 years ago

Seems like a very clean implementation, thanks a lot Erik! I'll test it when I start working on v4.

Surt commented 3 years ago

I was running into this issue as well. I've made a slightly adapted RefreshDatabase trait that works for me. Tests were previously running for more than a second each, with this trait it was reduced back to 300ms (as it was before using multi-database tenancy). ....

@erikgaal solution works great but I can't make it work with --parallel (https://laravel.com/docs/8.x/testing#parallel-testing-and-databases). Doctrine\DBAL\Driver\PDO\Exception: SQLSTATE[HY000] [1049] Unknown database 'testing_db_test_2' triying with --recreate-database blindly to see if it works and it does not :p

(I did previously changed the tearDown to delete only the initialized tenant)


    protected function tearDown(): void
    {
        config([
            'tenancy.queue_database_deletion'               => false,
            'tenancy.delete_database_after_tenant_deletion' => true,
        ]);
        tenancy()->tenant->delete();

        parent::tearDown();
    }

I will try to go deep to see what is happening.

plakhin commented 3 years ago

I've been able to use Parallel Testing with php artisan test -p --recreate-databases

<?php

namespace Tests;

use App\Models\Tenant;
use Illuminate\Foundation\Testing\RefreshDatabase;
use Illuminate\Support\Facades\DB;
use Illuminate\Support\Facades\ParallelTesting;
use Illuminate\Support\Facades\URL;

trait RefreshDatabaseWithTenant
{
    use RefreshDatabase {
        beginDatabaseTransaction as parentBeginDatabaseTransaction;
    }

    /**
     * The database connections that should have transactions.
     *
     * `null` is the default landlord connection
     * `tenant` is the tenant connection
     */
    protected array $connectionsToTransact = [null, 'tenant'];

    /**
     * We need to hook initialize tenancy _before_ we start the database
     * transaction, otherwise it cannot find the tenant connection.
     */
    public function beginDatabaseTransaction()
    {
        $this->initializeTenant();

        $this->parentBeginDatabaseTransaction();
    }

    public function initializeTenant()
    {
        $tenantId = 'acme';

        $tenant = Tenant::firstOr(function () use ($tenantId) {
            config(['tenancy.database.prefix' => config('tenancy.database.prefix') . ParallelTesting::token() . '_']);

            $dbName = config('tenancy.database.prefix') . $tenantId;

            DB::unprepared("DROP DATABASE IF EXISTS `{$dbName}`");

            $t = Tenant::create(['id' => $tenantId]);

            if ( ! $t->domains()->count()) {
                $t->domains()->create(['domain' => $tenantId . '.localhost']);
            }

            return $t;
        });

        tenancy()->initialize($tenant);

        URL::forceRootUrl('http://acme.localhost');
    }
}

However, for some tests DatabaseMigrations Trait will be needed instead of RefreshDatabase. As soon as I need it and get it ready, will also post here.

UPD: Also adding this container to docker-compose.yml speeded up tests significantly (I use Laravel Sail)

    mysqlt:
        image: 'mysql:8.0'
        ports:
            - '${FORWARD_DB_PORT:-3307}:3306'
        environment:
            MYSQL_ROOT_PASSWORD: '${DB_PASSWORD}'
            MYSQL_DATABASE: '${DB_DATABASE}'
        tmpfs:
            - /var/lib/mysql
        networks:
            - sail
        healthcheck:
          test: ["CMD", "mysqladmin", "ping", "-p${DB_PASSWORD}"]
          retries: 3
          timeout: 5s

Please note, that you'll also need this update DB_HOST=mysqlt in `.env.testing'.

plakhin commented 3 years ago

BTW, did someone find a way to use php artisan schema:dump with multi-database multi-tenancy?

mreduar commented 2 years ago

Has anyone been able to get their tests to run fast? None of the above solutions worked for me, not even the one with the sponsors documentation.

chrillep commented 2 years ago

any update ? and maybe migrate to using LazilyRefreshDatabase

lionslair commented 2 years ago

Nope still slow. Tried a range of things too but it needs to migrate each run. Using on a legacy project and has a lot of migrations.

ReinisL commented 2 years ago

In my case, I was able to run tests much faster using schema:dump command. To create a tenant schema dump you can run - artisan tenants:run schema:dump

DanjBethel commented 2 years ago
config(['tenancy.database.prefix' => 'tenant_test_']);
        config(['tenancy.filesystem.suffix_base' => 'tenant_test']);

Can you elaborate? Perhaps share some code?

francoisauclair911 commented 2 years ago

@plakhin did you ever manage to get this to work with DatabaseMigrations, your solution works but it doesn't clear the tables within a test suite. So some of my assertions fail

plakhin commented 2 years ago

@francoisauclair911, I've managed to get database refreshing before each test, will post it here later, currently AFK. However it's not as fast as using transactions.

plakhin commented 2 years ago

@francoisauclair911.

trait RefreshDatabaseWithTenant
{
    use RefreshDatabase {
        beginDatabaseTransaction as parentBeginDatabaseTransaction;
    }
    ...
    protected static function addAfterClass()
    {
        app()->make('db')->connection()->disconnect();
        tenant()->delete();
    }
    ...
abstract class TestCase extends BaseTestCase
{
    ...
    public static function tearDownAfterClass(): void
    {
        if (method_exists(static::class, 'addAfterClass')) {
            method_exists(static::class, 'addAfterAll')
                ? (new static(fn () => null, '', []))->setUp()
                : (new static())->setUp();
            static::addAfterClass();
        }
        parent::tearDownAfterClass();
    }
    ...
mreduar commented 2 years ago

@francoisauclair911.

trait RefreshDatabaseWithTenant
{
    use RefreshDatabase {
        beginDatabaseTransaction as parentBeginDatabaseTransaction;
    }
    ...
    protected static function addAfterClass()
    {
        app()->make('db')->connection()->disconnect();
        tenant()->delete();
    }
    ...
abstract class TestCase extends BaseTestCase
{
    ...
    public static function tearDownAfterClass(): void
    {
        if (method_exists(static::class, 'addAfterClass')) {
            method_exists(static::class, 'addAfterAll')
                ? (new static(fn () => null, '', []))->setUp()
                : (new static())->setUp();
            static::addAfterClass();
        }
        parent::tearDownAfterClass();
    }
    ...

How long would you say it would reduce this Trait?

plakhin commented 2 years ago

It depends, just try in your project

stancl commented 2 years ago

I'll reopen this since testing is something we want to focus on in v4 👍🏻

mreduar commented 2 years ago

I'll reopen this since testing is something we want to focus on in v4 👍🏻

How are things going with V4?

viicslen commented 1 year ago

Here's what I have been using so far and it has been working perfectly

trait WithTenancy
{
    protected function setUpTraits(): array
    {
        $uses = parent::setUpTraits();

        if (isset($uses[WithTenancy::class])) {
            $this->initializeTenancy($uses);
        }

        return $uses;
    }

    protected function initializeTenancy(array $uses): void
    {
        $organization = Organization::firstOr(static fn () => Organization::factory()->create());
        tenancy()->initialize($organization);

        if (isset($uses[DatabaseTransactions::class]) || isset($uses[RefreshDatabase::class])) {
            $this->beginTenantDatabaseTransaction();
        }

        if (isset($uses[DatabaseMigrations::class]) || isset($uses[RefreshDatabase::class])) {
            $this->beforeApplicationDestroyed(function () use ($organization) {
                $organization->delete();
            });
        }
    }

    public function beginTenantDatabaseTransaction(): void
    {
        $database = $this->app->make('db');

        $connection = $database->connection('tenant');
        $dispatcher = $connection->getEventDispatcher();

        $connection->unsetEventDispatcher();
        $connection->beginTransaction();
        $connection->setEventDispatcher($dispatcher);

        $this->beforeApplicationDestroyed(function () use ($database) {
            $connection = $database->connection('tenant');
            $dispatcher = $connection->getEventDispatcher();

            $connection->unsetEventDispatcher();
            $connection->rollBack();
            $connection->setEventDispatcher($dispatcher);
            $connection->disconnect();
        });
    }
}

Then on the tests that need it I just have to reference it like so:

PestPHP

use Illuminate\Foundation\Testing\DatabaseTransactions;
use Tests\WithTenancy;

uses(WithTenancy::class);
uses(DatabaseTransactions::class);

// ...

PHPUnit

namespace Tests\Feature;

use Illuminate\Foundation\Testing\DatabaseTransactions;
use Tests\TestCase;
use Tests\WithTenancy;

class ExampleTest extends TestCase
{
    use WithTenancy;
    use DatabaseTransactions;

    // ...
}
mreduar commented 1 year ago

@viicslen It looks very nice and elegant. But how is the speed? Did you have it in a different way before?

viicslen commented 1 year ago

When used with DatabaseTransactions it's very fast (almost like without tenancy) since it only needs to create the tenant database once and then on every subsequent test, it just rolls back any database change done during the test

PS: I have also setup a custom version of the schema dump functionality for the tenant database

mreduar commented 1 year ago

When used with DatabaseTransactions it's very fast (almost like without tenancy) since it only needs to create the tenant database once and then on every subsequent test, it just rolls back any database change done during the test

PS: I have also setup a custom version of the schema dump functionality for the tenant database

For this to work, do I have to have the database already created and empty?