userfrosting / UserFrosting

Modern PHP user login and management framework
https://www.userfrosting.com
Other
1.64k stars 366 forks source link

First migration should also be done from "php bakery test" #1174

Closed ktecho closed 3 years ago

ktecho commented 3 years ago

I was going to try and fix the docker strategy for doing tests (docker-compose-testdb.yml is not working right now), but there is something that I cannot understand.

I think we should have 2 database containers, one for the mysql used for the app and another one with a mysql for tests. You can do that with docker-compose, no problem. And using the TEST_DB environment variable, you can point to the database connection which should run the tests.

The problem seems to be that you need to make a bakery migration first on the test database before starting, but you can only do bakery migration on the default instance. I've been looking at how the tests are launched in Github, and the trick used there is using TEST_DB=default as environment variable. So when you run php bakery migrate, you migrate the default database, and then the tests are executed in that same default database, so it works.

As between tests, a reset (undo migrations and redo them again) is made, wouldn't it be correct to make a migration at first so it's done in the database for tests and we don't need to use this trick?

Thanks!

ktecho commented 3 years ago

Forgot to say it, but the strategy I plan to test and commit would have 4 containers (app, nginx, mysql, mysql-tests), so you could run the app in the browser and run the test with the same docker-compose set of containers.

lcharette commented 3 years ago

You don’t need to run the migration on the test database as long as your tests are using the refreshdatabase trait.

If you have a more specific scenario, migrate have a Database option : https://learn.userfrosting.com/cli/commands#migrate

lcharette commented 3 years ago

I've been looking at how the tests are launched in Github, and the trick used there is using TEST_DB=default as environment variable. So when you run php bakery migrate, you migrate the default database, and then the tests are executed in that same default database, so it works.

To further develop on that part... The migration are run that way in GitHub Actions as a way to "cheat" testing the migrations themselves. It's only done to make sure migration can be run fine and don't return any errors. But it's a bad design, as they should be tested using PHPUnit (and assertion performed on the table after migrations, as well as testing the "down" migration).

Using the env var is another hacky way to make sure PHPUnit test are run against the right database. The "real" solution would be to add a new db connection, or edit the test_integration connection. CI makes uses of the default connection to make things easier. I guess we could introduce a second "test" connection based on each DB provider...

lcharette commented 3 years ago

I'll close, as this is not really an issue. We can continue this in chat, or reopen if somethign is trully broken.