Skip to content
This repository has been archived by the owner on Aug 20, 2018. It is now read-only.

Using with prepared testing database #38

Closed
wants to merge 7 commits into from

Conversation

clovnrian
Copy link

  • not everyone want setup database automatically (create db, drop db, apply migrations etc.)
  • better handling default configuration parameters
  • add php.ini for testing with required extensions
  • add checkAjaxSignal method for testing signal called from AJAX

mbabjak added 7 commits September 29, 2016 17:10
User and password may not be set (SQLite or all config is in DSN)
Setup database only if parameter 'setupDatabase' is set to true in config.
Add new method for check signal from AJAX
@mrtnzlml
Copy link
Member

mrtnzlml commented Oct 8, 2016

My intention is take a look at this PR as soon as possible. Just letting you know I am aware of this request. Be patient please... :)

@mrtnzlml mrtnzlml self-assigned this Oct 8, 2016
mrtnzlml added a commit that referenced this pull request Oct 9, 2016
mrtnzlml added a commit that referenced this pull request Oct 9, 2016
mrtnzlml added a commit that referenced this pull request Oct 9, 2016
Now you can setup new option to 'shareDatabase:yes' and in this case new database will be created but will NOT be deleted after test case. This means that database is shared with one thread. There will be up to 8 databases in case you are using 8 threads. Default behavior is "always create and delete database" therefore it's without BC break:

testbench:
    shareDatabase: no

Related PR: #38
Close issue #37
Copy link
Member

@mrtnzlml mrtnzlml left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for PR. First of all (for the future reference) please send multiple PR instead of big one. For example in this case it's not acceptable as a whole and therefore I cannot accept it at all.

I implemented on my own these changes:

  • TPresenter::checkAjaxSignal
  • better DI extension parameters handling (with simple validation)
  • fixed wrong parameter setting in TestbenchExtension
  • added support for shared databases via shareDatabase: yes option (it behaves quite differently now, but it should be good)

Please look at it if it's good for your use case and if you have time for that please rebase this branch or send another PR and close this one.

Thanks for your help. I really appreciate it.

@@ -37,6 +37,11 @@ public function __construct(
}
try {
$this->__testbench_database_setup($connection, $container);
} catch(\Doctrine\DBAL\Migrations\MigrationException $e) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It throws Could not find any migrations to execute. and I think this is way better than silently move on. If you don't have migrations, you should use this setting:

testbench:
    migrations: no

extension=pdo.so
extension=pdo_mysql.so


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file should not be needed. Just use -C switch or if you are using vendor/bin/run-tests it should be enough (no INI file needed).

$sectionConfig['user'],
$sectionConfig['password'],
isset($sectionConfig['user']) ? $sectionConfig['user'] : null,
isset($sectionConfig['password']) ? $sectionConfig['password'] : null,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this moment Testbench only supports MySQL and PostgreSQL. I cannot blindly fix this because I don't fully understand what is the problem here. You should prepare failing test.

@clovnrian
Copy link
Author

Thanks for applying some of my changes. I tried your changes in our project and the share database is not what we need. We create SQLite database for tests, so we just need to use the database connection specified in configuration file without any database setup. I will decline my pull request and when I have time I will try add SQLite support to the library.

@clovnrian clovnrian closed this Oct 10, 2016
@mrtnzlml
Copy link
Member

Yeah, the thing is that at this moment SQLite is not supported and if you are using for example MySQL in your production - then you should use MySQL in tests. No matter what.

At this moment I have to figure out better database handling in general and after that the database drivers refactoring in Testbench is crucial. Current design is bad and I don't like it. But overall it works quite well (but only with supported databases obviously).

Thanks for understanding, patience and help... :)

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

Successfully merging this pull request may close these issues.

2 participants