ENHANCEMENT Ensure test DB is flushed on either DDL or transaction-disabled tests #8193

tractorcow · 2018-06-18T04:35:32Z

tractorcow · 2018-06-18T04:38:47Z

Note: We'll need to PR to other modules protected $usesTransactions = false; for tests that were failing.

tractorcow · 2018-06-18T05:27:36Z

ok, there's still a bug.

SapphireTest::setUpBeforeClass() has this code.

// Build DB if we have objects
if (static::getExtraDataObjects()) {
    DataObject::reset();
    static::resetDBSchema(true, true);
}

We need to occasionally do this on setUp() (not always setupBeforeClass) so it needs refactoring.

tractorcow · 2018-06-18T05:34:11Z

Yay fixed it.

dhensby · 2018-06-18T13:41:58Z

Postgres is failing for some reason :/

tractorcow · 2018-06-18T23:15:00Z

Yeah that's ok. I'll test postgres locally.

tractorcow · 2018-06-19T02:25:50Z

Error is in security somewhere.

tractorcow · 2018-06-19T02:48:29Z

src/ORM/Connect/TempDatabase.php

@@ -232,7 +245,12 @@ public function resetDBSchema(array $extraDataObjects = [])
        // pgsql doesn't allow schema updates inside transactions
        // so we need to rollback any transactions before commencing a schema reset
        if ($this->hasStarted()) {
-            $this->rollbackTransaction();


Ok it turns out this code doesn't work.

Trying

public function resetDBSchema(array $extraDataObjects = []) { if (!$this->isUsed()) { return; } try { $this->rebuildTables($extraDataObjects); } catch (DatabaseException $ex) { // In case of error during build force a hard reset // e.g. pgsql doesn't allow schema updates inside transactions $this->kill(); $this->build(); $this->rebuildTables($extraDataObjects); } }

@dhensby seem ok?

PHP Fatal error: Uncaught SilverStripe\ORM\Connect\DatabaseException: Couldn't run query: SELECT tablename FROM pg_catalog.pg_tables WHERE schemaname = $1 AND tablename NOT ILIKE 'pg\\_%' AND tablename NOT ILIKE 'sql\\_%' ERROR: current transaction is aborted, commands ignored until end of transaction block in /Users/dmooyman/Sites/ss42test/vendor/silverstripe/framework/src/ORM/Connect/DBConnector.php:64 Stack trace: #0 /Users/dmooyman/Sites/ss42test/vendor/silverstripe/postgresql/code/PostgreSQLConnector.php(234): SilverStripe\ORM\Connect\DBConnector->databaseError('Couldn't run qu...', 256, 'SELECT tablenam...', Array) #1 /Users/dmooyman/Sites/ss42test/vendor/silverstripe/framework/src/ORM/Connect/Database.php(168): SilverStripe\PostgreSQL\PostgreSQLConnector->preparedQuery('SELECT tablenam...', Array, 256) #2 /Users/dmooyman/Sites/ss42test/vendor/silverstripe/framework/src/ORM/Connect/Database.php(222): SilverStripe\ORM\Connect\Database->SilverStripe\ORM\Connect\{closure}('SELECT tablenam...') #3 /Users/dmooyman/Sites/s in /Users/dmooyman/Sites/ss42test/vendor/silverstripe/framework/src/ORM/Connect/DBConnector.php on line 64

WHY

I think we need rollbackRecursive() on database.

It's doing the rollback, but the rollback doesn't seem to actually rollback on postgres.

ok, there is like 6 levels of nesting at this error. A single rollback isn't enough.

ok the problem is hasStarted is true sometimes when there is no transaction. After a rollback hasStarted needs to stay true, even though the transaction is rolled back. It's a badly documented var.

I'm dropping hasStarted() and using isUsed() instead. Between runs, there will be two states:

The DB Is populated, assume that it's ready and safe to use.

The DB is killed, assume that it requires a rebuild.

There is no "there but needs a reset state".

tractorcow · 2018-06-19T04:22:45Z

@dhensby the last commit + silverstripe/silverstripe-postgresql#86 will fix postgres, at least for the tests that WERE failing.

I've dropped the started flag on test DB since it's not really a state of the test db, but the test fixtures. I've shifted this into a separate state in FixtureTestState.

I've also added a new API to database; databases must now report transactional nesting level, in order to enable tests to recover from incorrectly nested transactions.

This is the last hour of my last day, so I'm afraid I have to hand over to @robbieaverill + @dhensby .

tractorcow · 2018-06-19T04:42:51Z

Local testing is VERY promising.

Only this test broke in postgres:

Time: 21.31 minutes, Memory: 124.00MB

There was 1 failure:

1) SilverStripe\ORM\Tests\DataListTest::testSortMixedCase
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
     0 => 'Bob'
-    1 => 'bonny'
-    2 => 'jane'
-    3 => 'John'
-    4 => 'sam'
-    5 => 'Steve'
+    1 => 'John'
+    2 => 'Steve'
+    3 => 'bonny'
+    4 => 'jane'
+    5 => 'sam'
     6 => 'steven'
 )

/Users/dmooyman/Sites/ss42test/vendor/silverstripe/framework/tests/php/ORM/DataListTest.php:627
/Users/dmooyman/Sites/ss42test/vendor/phpunit/phpunit/phpunit:52

FAILURES!
Tests: 2149, Assertions: 7858, Failures: 1, Skipped: 14, Incomplete: 2.
Damians-Macbook:ss42test dmooyman$

tractorcow · 2018-06-19T05:14:48Z

Wow green.

This story was SO hard to get working properly... sorry in advance @dhensby if the new logic is hard to follow. =(

robbieaverill · 2018-06-19T09:19:37Z

src/ORM/Connect/Database.php

+    public function transactionDepth()
+    {
+        // Placeholder error for transactional DBs that don't expose depth
+        if ($this->supportsTransactions()) {


Should this logic be the opposite, i.e. if the DB doesn't support transactions, generate a user error.

Assume this hasn't surfaced because this method is overloaded in MySQL and Postgres

No, its an error because even though you support transactions, the depth is hard coded to 0.it's a placeholder for making this api abstract.

robbieaverill · 2018-06-19T09:23:28Z

tests/php/Security/GroupTest.php

+    protected function setUp()
+    {
+        parent::setUp();
+    }


What's the benefit of this?

Oops. That was my var dump holder lol

robbieaverill · 2018-06-19T09:24:41Z

src/ORM/Connect/TempDatabase.php

-    public function hasStarted()
-    {
-        return $this->hasStarted;
-    }


This looks like a semver break - perhaps we should leave it there but deprecate it? I realise it won't be doing anything any more other than always returning false, which could be misleading to people using this API

I'm not too concerned about this

It wasn't in a stable tag so I'm un-api ing it.

dhensby · 2018-06-19T14:47:30Z

Do we know how or why we were getting to so many levels of transaction nesting? It seems like we really shouldn't get more than 2 or 3 levels of transactions deep during a test...

dhensby · 2018-06-19T16:55:49Z

A point worth making, but not something I think needs specifically addressing as we've likely covered 99% of usecases, but the commands that cause "implicit commits" aren't necassarily just DDL queries - https://dev.mysql.com/doc/refman/5.7/en/implicit-commit.html

…sabled tests Fixes silverstripe#8182

dhensby · 2018-06-19T17:32:36Z

I've addressed some of the feedback and made some changes I felt were appropriate

dhensby · 2018-06-19T17:33:06Z

I'm wondering if the nested transaction changes will mean we need to go and update all our DB drivers to match

robbieaverill · 2018-06-19T21:32:07Z

Ok we lost the 💚 😞

robbieaverill · 2018-06-19T22:31:37Z

src/ORM/Connect/MySQLDatabase.php

-        if ($this->transactionNesting <= 0) {
-            $this->transactionNesting = 0;
-            $this->query('COMMIT AND ' . ($chain ? '' : 'NO ') . 'CHAIN');
-        }


I feel like maybe this logic should remain, could explain the test breakages

I added this back locally and re-tested and it fixes the new failures, pushed a fix to add this back

tractorcow · 2018-06-20T21:34:02Z

@dhensby the logic on the nesting method was correct before you changed it. It's ment to error if not overridden eg sqlite. Can you revert.

dhensby · 2018-06-21T13:23:42Z

@dhensby the logic on the nesting method was correct before you changed it. It's ment to error if not overridden eg sqlite. Can you revert.

ah, will do

dhensby · 2018-06-21T13:27:02Z

I've pushed the change straight into the repo with 27b60ae

dhensby · 2018-07-23T12:57:33Z

@maxime-rainville did you use "rebase merge" on this PR? I can't find the merge commit anywhere...

…et-on-ddl This is a cherry-picked PR from 4.2 -> 4.2.0

maxime-rainville · 2018-07-24T02:37:43Z

@dhensby Probably.

tractorcow mentioned this pull request Jun 18, 2018

BUG Issue with transactions polluting test state #8182

Closed

3 tasks

tractorcow commented Jun 19, 2018

View reviewed changes

robbieaverill suggested changes Jun 19, 2018

View reviewed changes

robbieaverill added the affects/v4 label Jun 19, 2018

Damian Mooyman added 5 commits June 19, 2018 17:57

ENHANCEMENT Ensure test DB is flushed on either DDL or transaction-di…

82aaadf

…sabled tests Fixes silverstripe#8182

BUG Ensure that build includes extra classes

daed8a9

BUG FIx manual resetDBSchema() calls breaking the database

630fbe9

Maybe fix it?

8779033

Use 2.1 of postgres

76d3c67

dhensby force-pushed the pulls/4.2/reset-on-ddl branch from b19cd9d to 76d3c67 Compare June 19, 2018 16:58

Address feedback

12eb499

robbieaverill reviewed Jun 19, 2018

View reviewed changes

Restore check for zero or negative transaction nesting

0d1906c

robbieaverill approved these changes Jun 19, 2018

View reviewed changes

maxime-rainville self-assigned this Jun 20, 2018

maxime-rainville merged commit df25768 into silverstripe:4.2 Jun 20, 2018

maxime-rainville deleted the pulls/4.2/reset-on-ddl branch June 20, 2018 02:46

This was referenced Jun 20, 2018

FIX Make sure nested transactions get reset on implicit commits silverstripe/silverstripe-sqlite3#47

Merged

FIX Make sure nested transactions get reset on implicit commits silverstripe/silverstripe-mssql#54

Merged

dhensby added a commit to dhensby/silverstripe-framework that referenced this pull request Jul 23, 2018

Merge pull request silverstripe#8193 from open-sausages/pulls/4.2/res…

c74a6e7

…et-on-ddl This is a cherry-picked PR from 4.2 -> 4.2.0

dhensby added a commit to dhensby/silverstripe-framework that referenced this pull request Jul 23, 2018

Merge pull request silverstripe#8193 from open-sausages/pulls/4.2/res…

5ec0faf

…et-on-ddl This is a cherry-picked PR from 4.2 -> 4.2.0

ENHANCEMENT Ensure test DB is flushed on either DDL or transaction-disabled tests #8193

ENHANCEMENT Ensure test DB is flushed on either DDL or transaction-disabled tests #8193

Conversation

tractorcow commented Jun 18, 2018

tractorcow commented Jun 18, 2018

tractorcow commented Jun 18, 2018 • edited by dhensby Loading

tractorcow commented Jun 18, 2018

dhensby commented Jun 18, 2018

tractorcow commented Jun 18, 2018

tractorcow commented Jun 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tractorcow Jun 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tractorcow commented Jun 19, 2018

tractorcow commented Jun 19, 2018

tractorcow commented Jun 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhensby commented Jun 19, 2018

dhensby commented Jun 19, 2018

dhensby commented Jun 19, 2018

dhensby commented Jun 19, 2018

robbieaverill commented Jun 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tractorcow commented Jun 20, 2018

dhensby commented Jun 21, 2018

dhensby commented Jun 21, 2018

dhensby commented Jul 23, 2018

maxime-rainville commented Jul 24, 2018

tractorcow commented Jun 18, 2018 •

edited by dhensby

Loading

tractorcow Jun 19, 2018 •

edited

Loading