[WIP] Propel2 testsuite #530

Closed
wants to merge 27 commits into
from

Projects

None yet

4 participants

@marcj
Member
marcj commented Jan 29, 2014

Updated the test suite to show code coverage at coveralls.io.
Added test for some stuff.

work in progress ...

@willdurand
Member

👍 great work, as always!

@staabm
Member
staabm commented Jan 29, 2014

@marcj just give me a ping in case you are ready with it.

@marcj
Member
marcj commented Jan 29, 2014

Aijaijaij, the test suite takes forever with code coverage activated. We should move tests that are not related to a database into one extract job and extract those then from the database related jobs.
Maybe we can save then some time.

@marcj
Member
marcj commented Jan 30, 2014

Ok, I separated it. We will see how it behaves now. ✌️

//edit, hate it when it runs on my dev-machine but not on travis 👎

@willdurand willdurand commented on the diff Jan 30, 2014
src/Propel/Generator/Command/DatabaseReverseCommand.php
@@ -83,6 +82,8 @@ protected function execute(InputInterface $input, OutputInterface $output)
$more = $input->getOption('verbose') ? '' : ' You can use the --verbose option to get more information.';
$output->writeln(sprintf('<error>Schema reverse engineering failed.%s</error>', $more));
+
+ return 1;
@willdurand
willdurand Jan 30, 2014 Propel member

return 1? why not throwing an exception? the console component will catch it and it will be displayed as an error.

@marcj
marcj Jan 30, 2014 Propel member

1 is the classical unix exit code also used in Console componenent. I haven't thrown any exception because it wasn't there before. I needed it in the tests to check if the command was successful. Guess I can do it also with exception, but don't know whether it returns then a exit code of !0 as well.

@willdurand
willdurand Jan 30, 2014 Propel member

Ok. I thought it was the proper way to render error messages in the Console component.

Exceptions are catched and converted into error messages, with a status code that is not 0.

@marcj
marcj Jan 30, 2014 Propel member

Perfect, then I'll change that to a exception 👍

@staabm
staabm Jan 30, 2014 Propel member

yes, exception should do the job here 👍

@marcj
Member
marcj commented Feb 4, 2014

Ok, we got now a code coverage of 92%+, probably a few (4-5) more as coveralls can not summarize all coverages from all jobs very well. It appears that Scrutinizer says good bye completely when it comes to a build matrix (and its pretty shocking what scrutinizer complains about). Pretty sad.

Fixed also a couple of bugs and removed unused code. Guess it's time for a first review although it's not completely done. (ignore .travis.yml etc for example, I'm still trying to get a code coverage visualisation tool set up that supports a build matrix)

@mpscholten mpscholten commented on the diff Feb 4, 2014
src/Propel/Generator/Reverse/SqliteSchemaParser.php
@@ -148,7 +160,10 @@ public function parse(Database $database)
*/
protected function addColumns(Table $table)
{
- $stmt = $this->dbh->query("PRAGMA table_info('" . $table->getName() . "')");
+ $tableName = $table->getName();
+
+// var_dump("PRAGMA table_info('$tableName') //");
@mpscholten
mpscholten Feb 4, 2014 Propel member

I think this should be removed, shouldn't it?

@marcj
marcj Mar 18, 2014 Propel member

indeed

@mpscholten mpscholten commented on the diff Feb 4, 2014
tests/Propel/Tests/Generator/Builder/NamespaceTest.php
{
- protected function setUp()
- {
- parent::setUp();
- Propel::init(__DIR__ . '/../../../../Fixtures/namespaced/build/conf/bookstore_namespaced-conf.php');
- }
-
- protected function tearDown()
- {
- parent::tearDown();
- Propel::init(dirname(__FILE__) . '/../../../../Fixtures/bookstore/build/conf/bookstore-conf.php');
- }
+// protected function setUp()
@mpscholten
mpscholten Feb 4, 2014 Propel member

Could be removed too, or is there a special reason you are keeping this?

@marcj marcj commented on the diff Feb 17, 2014
src/Propel/Generator/Reverse/SqliteSchemaParser.php
@@ -121,7 +121,19 @@ public function parse(Database $database)
continue;
}
+ $schema = null;
+ if (!$database->getSchema()) {
+ if (false !== ($pos = strpos($name, '§'))) {
+ $schema = substr($name, 0, $pos); //2 because the delimiter § uses in UTF8 one byte more.
@marcj
marcj Feb 17, 2014 Propel member

copy&paste issue

@staabm staabm commented on the diff Feb 17, 2014
tests/bin/phpunit.pgsql.sh
@@ -1,2 +1,3 @@
#!/bin/sh
+#phpunit --group database --exclude-group mysql;
@staabm
staabm Feb 17, 2014 Propel member

will this run all pgsql tests + all db agnostic tests?

@marcj
marcj Mar 18, 2014 Propel member

This won't run anything since its commented out. :-P
But no, it would run all db-related non-mysql tests.

@willdurand willdurand added this to the 2.0.0 milestone Mar 16, 2014
@marcj marcj commented on the diff Mar 18, 2014
tests/Propel/Tests/Runtime/ActiveQuery/CriteriaTest.php
@@ -36,29 +39,28 @@ class CriteriaTest extends BookstoreTestBase
*/
private $c;
- /**
- * DB adapter saved for later.
- *
- * @var AbstractAdapter
- */
- private $savedAdapter;
+// /**
+// * DB adapter saved for later.
+// *
+// * @var AbstractAdapter
+// */
+// private $savedAdapter;
@marcj
marcj Mar 18, 2014 Propel member

remove

@marcj marcj commented on the diff Mar 18, 2014
tests/Propel/Tests/Runtime/ActiveQuery/CriteriaTest.php
protected function setUp()
{
parent::setUp();
$this->c = new ModelCriteria();
- $defaultDatasource = Propel::getServiceContainer()->getDefaultDatasource();
- $this->savedAdapter = Propel::getServiceContainer()->getAdapter($defaultDatasource);
- $newAdapter = Propel::getServiceContainer()->getAdapter(BookTableMap::DATABASE_NAME);
- $this->adapterClass = $newAdapter->getAdapterId();
- Propel::getServiceContainer()->setAdapter($defaultDatasource, $newAdapter);
+// $defaultDatasource = Propel::getServiceContainer()->getDefaultDatasource();
+// $this->savedAdapter = Propel::getServiceContainer()->getAdapter($defaultDatasource);
+// $newAdapter = Propel::getServiceContainer()->getAdapter(BookTableMap::DATABASE_NAME);
+// Propel::getServiceContainer()->setAdapter($defaultDatasource, $newAdapter);
@marcj
marcj Mar 18, 2014 Propel member

remove

@marcj marcj commented on the diff Mar 18, 2014
tests/Propel/Tests/Runtime/ActiveQuery/CriteriaTest.php
}
- protected function tearDown()
- {
- Propel::getServiceContainer()->setAdapter(Propel::getServiceContainer()->getDefaultDatasource(), $this->savedAdapter);
- parent::tearDown();
- }
+// protected function tearDown()
+// {
+// Propel::getServiceContainer()->setAdapter(Propel::getServiceContainer()->getDefaultDatasource(), $this->savedAdapter);
+// parent::tearDown();
+// }
@marcj
marcj Mar 18, 2014 Propel member

remove

@marcj marcj commented on the diff Mar 18, 2014
tests/Propel/Tests/Runtime/Collection/CollectionTest.php
@@ -314,6 +312,9 @@ public function testSerializable()
$this->assertEquals($col, $col2, 'Collection is serializable');
}
+ /**
+ * @database
+ */
@marcj
marcj Mar 18, 2014 Propel member

remove

@marcj marcj commented on the diff Mar 18, 2014
tests/Propel/Tests/TestCaseFixtures.php
+ */
+ protected static $withDatabaseSchema = false;
+
+ /**
+ * Depending on this type we return the correct runninOn* results,
+ * also getSql() and getDriver() is based on that.
+ *
+ * @var ConnectionInterface
+ */
+ protected $con;
+
+ /**
+ * Setup fixture. Needed here because we want to have a realistic code coverage value.
+ */
+ protected function setUp()
+ {
@marcj
marcj Mar 18, 2014 Propel member

indentation

@marcj marcj commented on the diff Mar 18, 2014
tests/Propel/Tests/TestCaseFixtures.php
+
+namespace Propel\Tests;
+
+use Propel\Generator\Command\TestPrepareCommand;
+use Propel\Runtime\Propel;
+use Symfony\Component\Console\Application;
+use Propel\Runtime\Connection\ConnectionInterface;
+use Symfony\Component\Finder\Finder;
+
+/**
+ * This test case class is used when the fixtures are needed. It takes care that
+ * those files (model classes and -conf.php files) are created.
+ *
+ * If you need additional to that also database's tables use TestCaseFixturesDatabase instead.
+ *
+ * @author William Durand <william.durand1@gmail.com>
@marcj
marcj Mar 18, 2014 Propel member

copy&paste

@marcj marcj commented on the diff Mar 18, 2014
tests/Propel/Tests/TestCaseFixtures.php
+ $app->add($r->newInstance());
+ }
+ }
+ if (0 !== strpos($dsn, 'sqlite:')) {
+ $options['--user'] = getenv('DB_USER') ?: 'root';
+ }
+
+ if (false !== getenv('DB_PW')) {
+ $options['--password'] = getenv('DB_PW');
+ }
+
+ $input = new \Symfony\Component\Console\Input\ArrayInput($options);
+
+ $output = new \Symfony\Component\Console\Output\BufferedOutput();
+ $app->setAutoExit(false);
+ if (0 !== $app->run($input, $output)) {
@marcj
marcj Mar 18, 2014 Propel member

indentation

@marcj
Member
marcj commented Apr 17, 2014

Fixed and merged. Thanks for the review!

@marcj marcj closed this Apr 17, 2014
@willdurand
Member

awesome!

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