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

Create a dedicated testing database #388

Merged
merged 1 commit into from
Apr 22, 2022
Merged

Create a dedicated testing database #388

merged 1 commit into from
Apr 22, 2022

Conversation

jessarcher
Copy link
Member

@jessarcher jessarcher commented Apr 22, 2022

The problem

In a default sail installation, running tests with the RefreshDatabase trait will empty the user's local database and log them out. This is because the trait calls migrate:fresh.

To reproduce:

  1. Set up a project with Sail
    laravel new --git sail-test
    cd sail-test
    php artisan sail:install
    ./vendor/bin/sail up -d
    ./vendor/bin/sail artisan migrate
  2. Allow user registration (e.g. with Breeze)
    ./vendor/bin/sail composer require --dev laravel/breeze
    ./vendor/bin/sail artisan breeze:install
    ./vendor/bin/sail npm install
    ./vendor/bin/sail npm run dev
  3. Browse to http://localhost/register and register an account
  4. Run the tests (Breeze includes tests with the RefreshDatabase trait)
    ./vendor/bin/sail test
  5. Refresh the page at http://localhost and see that you're logged out and your account no longer exists.

Using SQLite for tests can be a good solution, but the lack of feature parity with MySQL et al. can be a confusing source of bugs.

The solution

This PR configures the database containers to create an additional testing database. The PHPUnit configuration is updated accordingly.

I have tested this with MySQL, MariaDB, and PostgreSQL.

Note that MySQL and Maria DB need an init script to set the appropriate permissions for the user. This is not required for PostgreSQL because the POSTGRES_USER has superuser privileges.

Notes

  • The PHPUnit configuration may also be too brittle as it's expecting a default Laravel phpunit.xml. I can make this a lot smarter at the cost of code (and regex) complexity.
  • The database name is currently hardcoded as testing. This will conflict if the users DB_USERNAME is already set to that, which would occur when running laravel new testing. It doesn't cause any failures, however, they will only get a single database which won't solve the above issue. We could use a more obscure name (e.g. sail_testing) or attempt to make it dynamic (e.g. ${DB_DATABASE}_testing)
  • It's probably worth mentioning the additional database in the docs - I will create a PR if this goes ahead.

@PHPGuus
Copy link

PHPGuus commented Apr 22, 2022

Brilliant! Just what I needed! Thanks @jessarcher
I'd prefer ${DB_DATABASE}_testing, for the name, to be honest.

@jessarcher
Copy link
Member Author

jessarcher commented Apr 22, 2022

Brilliant! Just what I needed! Thanks @jessarcher I'd prefer ${DB_DATABASE}_testing, for the name, to be honest.

I think it would be clearer. Postgres and MySQL appear to support up to 63 and 64 character names, respectively, so there is a chance (albeit slim) for an overflow. It also complicates the PHPUnit configuration, but it should be easy enough.

@driesvints
Copy link
Member

Would there be a need for people to customize the setup scripts?

@jessarcher
Copy link
Member Author

Would there be a need for people to customize the setup scripts?

@driesvints Potentially. It's a little tricky to publish because we'd need to update the path in the users docker-compose.yml file.

However, because they do own the docker-compose.yml file, it's easy enough for them to point to a different script or remove it entirely.

@taylorotwell taylorotwell merged commit 69013b1 into 1.x Apr 22, 2022
@taylorotwell
Copy link
Member

Looks good to me! We could explore that DB_DATABASE_testing approach if we get some more community feedback.

@taylorotwell taylorotwell deleted the testing-db branch April 22, 2022 14:51
@gafitescu
Copy link

If you have a separate database for testing make sense to have a mysql in-memory option too
See https://blog.deleu.dev/tip-speed-up-your-tests-with-mysql-in-memory/

@kichetof
Copy link

Just tried it and phpunit.xml could have <server or <env attributes. In my case <!-- <server name="DB_DATABASE" value=":memory:"/> -->

This line should be updated to replace <env AND <server to be fully functional.

What do you think @taylorotwell ?

@driesvints
Copy link
Member

@kichetof Laravel uses env which is the recommended default: https://github.com/laravel/laravel/blob/9.x/phpunit.xml

See the related PR here: laravel/laravel#5765

@kichetof
Copy link

@driesvints Thanks for your quick feedback! My project has been created on Laravel 8 and was updated to Laravel 9, maybe that's the issue with my phpunit server attributes?

@driesvints
Copy link
Member

I'd just change it to env.

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.

None yet

6 participants