silverstripe / silverstripe-framework

Silverstripe Framework, the MVC framework that powers Silverstripe CMS
https://www.silverstripe.org
BSD 3-Clause "New" or "Revised" License
720 stars 820 forks source link

ORM test failures when using SQLite #9235

Open michalkleiner opened 4 years ago

michalkleiner commented 4 years ago

Affected Version

4.4.x-dev (latest 4)

Steps to Reproduce

  1. Enable SQLite tests in .travis.yml by adding a new matrix stanza (feel free to remove the other ones to speed up the testing)

    - php: 7.3
      env:
        - DB=SQLITE
        - PDO=1
        - PHPUNIT_TEST=framework
  2. Export env variables and create public dir

    export COMPOSER_ROOT_VERSION=4.4.x-dev
    export DB=SQLITE
    export PHPUNIT_TEST=framework
    mkdir ./public
  3. Add and install Composer dependencies

    composer require silverstripe/sqlite3:2.2.x-dev --no-update
    composer require silverstripe/recipe-testing:^1 silverstripe/recipe-core:4.4.x-dev
    composer install --prefer-source --no-interaction --no-progress --no-suggest --optimize-autoloader --verbose --profile
  4. Run framework tests suit

    vendor/bin/phpunit --testsuite $PHPUNIT_TEST

Observed output

Errors in DatabaseTest::testRepeatedIteration and TransactionTest ::testReadOnlyTransaction tests.

2) SilverStripe\ORM\Tests\DatabaseTest::testRepeatedIteration
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    0 => 'one'
     1 => 'two'
     2 => 'three'
     3 => 'four'
 )

/github/silverstripe-framework/tests/php/ORM/DatabaseTest.php:294
/github/silverstripe-framework/vendor/phpunit/phpunit/phpunit:52

3) SilverStripe\ORM\Tests\TransactionTest::testReadOnlyTransaction
Failed asserting that SilverStripe\ORM\Tests\TransactionTest\TestObject Object &000000005d7b8c7a000000003a6a7f14 (
    'destroyed' => false
    'record' => Array &0 (
        'ClassName' => 'SilverStripe\ORM\Tests\TransactionTest\TestObject'
        'LastEdited' => '2019-09-12 01:23:48'
        'Created' => '2019-09-12 01:23:48'
        'Title' => 'Read only page failed'
        'ID' => 2
        'RecordClassName' => 'SilverStripe\ORM\Tests\TransactionTest\TestObject'
    )
    'joinRecord' => null
    'changed' => Array &1 ()
    'changeForced' => false
    'original' => Array &2 (
        'ClassName' => 'SilverStripe\ORM\Tests\TransactionTest\TestObject'
        'LastEdited' => '2019-09-12 01:23:48'
        'Created' => '2019-09-12 01:23:48'
        'Title' => 'Read only page failed'
        'ID' => 2
        'RecordClassName' => 'SilverStripe\ORM\Tests\TransactionTest\TestObject'
    )
    'brokenOnDelete' => false
    'brokenOnWrite' => false
    'components' => Array &3 ()
    'unsavedRelations' => null
    'sourceQueryParams' => null
    'failover' => null
    'customisedObject' => null
    'objCache' => Array &4 ()
    'extension_instances' => null
    'beforeExtendCallbacks' => Array &5 ()
    'afterExtendCallbacks' => Array &6 ()
    'extra_method_registers' => Array &7 ()
) is null.

/github/silverstripe-framework/tests/php/ORM/TransactionTest.php:194
/github/silverstripe-framework/vendor/phpunit/phpunit/phpunit:52

Expected output

Passing tests without errors.

Considerations

Should at least one set of tests also run against the SQLite database? Without that (removed in https://github.com/silverstripe/silverstripe-framework/commit/c47a1d9091c4cba52109c7fa9d9a46ac57d4ea93 back in 2016), some code changes might be breaking for SQLite support. In fact, it can be viewed as a nice surprise that only two small tests are not passing.

If not run for each PR/build, is there another way how to run tests using SQLite periodically at least once in a while?

ScopeyNZ commented 4 years ago

SilverStripe\ORM\Tests\DatabaseTest::testRepeatedIteration

This test was added recently after a bug was found in other DB drivers. I'm not too surprised to see it for SQLite, but it's probably the adapter that needs fixing here.

ScopeyNZ commented 4 years ago

See: https://github.com/silverstripe/silverstripe-postgresql/pull/99

michalkleiner commented 4 years ago

Thanks @ScopeyNZ — yeah, one is the module, one is the test itself. Created PRs for both.

sminnee commented 4 years ago

We removed SQLite testing because SQLite support wasn't a priority and it was slowing down every single test execution to add this one in there. I'm not keen to add it back to the standard build, but if we wanted a more kitchen-sinky daily or weekly build, we could potentially add it back there.

michalkleiner commented 4 years ago

New PR for the "ReadOnlyTransaction" test - https://github.com/silverstripe/silverstripe-framework/pull/9240