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

Add seedings capabilities (#3552) #3625

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
3 participants
@Bouhnosaure
Contributor

Bouhnosaure commented Oct 20, 2016

Hello
I've made a simple way for seeding the database you just set run_database_seeds to true but run_database_migrations need to be true too.

for the first comment in #3552 with the ability to choose what seeds we want to run
a simple way would be to set APP_ENV to testing in the .env.testing and detect in DatabaseSeeder.php if wich environement we are and run only the seeds we want.

Show outdated Hide outdated src/Codeception/Module/Laravel5.php
@@ -152,6 +153,10 @@ public function _before(\Codeception\TestInterface $test)
if ($this->config['run_database_migrations']) {
// Must be called before database transactions are started
$this->callArtisan('migrate');
if ($this->config['run_database_seeds']){

This comment has been minimized.

@Nitpick-CI

Nitpick-CI Oct 20, 2016

Expected 1 space after closing parenthesis; found 0

@Nitpick-CI

Nitpick-CI Oct 20, 2016

Expected 1 space after closing parenthesis; found 0

@janhenkgerritsen

This comment has been minimized.

Show comment
Hide comment
@janhenkgerritsen

janhenkgerritsen Oct 21, 2016

Contributor

I have a few comments and questions.

Firstly, can you submit your PR to the 2.2 branch instead of master?

I also think this implementation is a bit too simple. I think it should at least be possible to specify a class name of the seeder you want to run. This way you can have different seeders for different environments. I think a good way to implement this is by adding another configuration option database_seeder_class that defaults to DatabaseSeeder, which is the name of the class used in a new Laravel installation.

I also think run_database_seeds should be changed into run_database_seeder.

Lastly, I think you should move the code outside the if statement that checks if migrations should run. This way you are not required to run the migrations, which provides more flexibility. You could for example use the DB module to setup your database and then run your database seeders via the Laravel5 module.

Let me know what you think.

Contributor

janhenkgerritsen commented Oct 21, 2016

I have a few comments and questions.

Firstly, can you submit your PR to the 2.2 branch instead of master?

I also think this implementation is a bit too simple. I think it should at least be possible to specify a class name of the seeder you want to run. This way you can have different seeders for different environments. I think a good way to implement this is by adding another configuration option database_seeder_class that defaults to DatabaseSeeder, which is the name of the class used in a new Laravel installation.

I also think run_database_seeds should be changed into run_database_seeder.

Lastly, I think you should move the code outside the if statement that checks if migrations should run. This way you are not required to run the migrations, which provides more flexibility. You could for example use the DB module to setup your database and then run your database seeders via the Laravel5 module.

Let me know what you think.

@Bouhnosaure

This comment has been minimized.

Show comment
Hide comment
@Bouhnosaure

Bouhnosaure Oct 21, 2016

Contributor

Yes i can change the branche of my PR to 2.2

Hum you are right for the multiple seeder option,
I will do this soon, i think i have to make a loop throught the seeder classes in the option database_seeder_class because laravel doesn't permit to specify multiples classes in the command (code)

For the options names, it's okay too, i will change it for better names.

And sorry again for the PR into the master, i was a bit confused, i didn't see the changes in this file, i had contribued to Laravel5 module but before this commit 😕

Contributor

Bouhnosaure commented Oct 21, 2016

Yes i can change the branche of my PR to 2.2

Hum you are right for the multiple seeder option,
I will do this soon, i think i have to make a loop throught the seeder classes in the option database_seeder_class because laravel doesn't permit to specify multiples classes in the command (code)

For the options names, it's okay too, i will change it for better names.

And sorry again for the PR into the master, i was a bit confused, i didn't see the changes in this file, i had contribued to Laravel5 module but before this commit 😕

@Bouhnosaure Bouhnosaure changed the base branch from master to 2.2 Oct 21, 2016

@Bouhnosaure Bouhnosaure changed the base branch from 2.2 to master Oct 21, 2016

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