Skip to content
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

test: ensure cleanup of sqlite3 db files after test #8393

Merged
merged 1 commit into from
Mar 18, 2024

Conversation

paulbalandan
Copy link
Member

@paulbalandan paulbalandan commented Dec 30, 2023

Description
Currently, writable/ has two leftover DB files from sqlite3: database.db and runner.sqlite. This PR cleans them.

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@paulbalandan paulbalandan added the testing Pull requests that changes tests only label Dec 30, 2023
@michalsn
Copy link
Member

Oh... seems like SQLite3 tests are failing now.

Copy link
Member

@kenjis kenjis left a comment

Choose a reason for hiding this comment

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

PHPUnit 9.6.15 by Sebastian Bergmann and contributors.

Runtime:       PHP 7.4.33
Configuration: /home/runner/work/CodeIgniter4/CodeIgniter4/phpunit.xml.dist

..S............................................................  63 / 667 (  9%)
...............S.S...S...........S............................. 126 / 667 ( 18%)
............................................................... 189 / 667 ( 28%)
......SSSSSSSSSSSSSSSSS.........................EEEEEEEEEEEEEEE 252 / 667 ( 37%)
EEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEE 315 / 667 ( 47%)
EEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEE 378 / 667 ( 56%)
EEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEE 441 / 667 ( 66%)
EEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEE... 504 / 667 ( 75%)
.....EEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEE 567 / 667 ( 85%)
EEEEEEEEEEEEEEEEEEEEEEEEEEE.................................... 630 / 667 ( 94%)
.............EEEEEEEEEEEEEEEEEEEEEEEE                           667 / 667 (100%)

Time: 00:10.854, Memory: 94.00 MB

There were 373 errors:

1) CodeIgniter\Database\Live\SelectTest::testSelectAllByDefault
CodeIgniter\Database\Exceptions\DatabaseException: SQLite3::exec(): attempt to write a readonly database

/home/runner/work/CodeIgniter4/CodeIgniter4/system/Database/BaseConnection.php:647
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Database/Forge.php:637
/home/runner/work/CodeIgniter4/CodeIgniter4/tests/_support/Database/Migrations/20160428212500_Create_test_tables.php:165
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Database/MigrationRunner.php:865
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Database/MigrationRunner.php:288
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Test/DatabaseTestTrait.php:134
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Test/DatabaseTestTrait.php:103
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Test/DatabaseTestTrait.php:55
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Test/CIUnitTestCase.php:249
phpvfscomposer:///home/runner/work/CodeIgniter4/CodeIgniter4/vendor/phpunit/phpunit/phpunit:106

Caused by
CodeIgniter\Database\Exceptions\DatabaseException: SQLite3::exec(): attempt to write a readonly database

/home/runner/work/CodeIgniter4/CodeIgniter4/system/Database/SQLite3/Connection.php:151
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Database/BaseConnection.php:693
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Database/BaseConnection.php:607
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Database/Forge.php:637
/home/runner/work/CodeIgniter4/CodeIgniter4/tests/_support/Database/Migrations/20160428212500_Create_test_tables.php:165
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Database/MigrationRunner.php:865
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Database/MigrationRunner.php:288
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Test/DatabaseTestTrait.php:134
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Test/DatabaseTestTrait.php:103
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Test/DatabaseTestTrait.php:55
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Test/CIUnitTestCase.php:249
phpvfscomposer:///home/runner/work/CodeIgniter4/CodeIgniter4/vendor/phpunit/phpunit/phpunit:106

Caused by
ErrorException: SQLite3::exec(): attempt to write a readonly database

/home/runner/work/CodeIgniter4/CodeIgniter4/system/Database/SQLite3/Connection.php:145
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Database/BaseConnection.php:693
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Database/BaseConnection.php:607
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Database/Forge.php:637
/home/runner/work/CodeIgniter4/CodeIgniter4/tests/_support/Database/Migrations/20160428212500_Create_test_tables.php:165
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Database/MigrationRunner.php:865
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Database/MigrationRunner.php:288
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Test/DatabaseTestTrait.php:134
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Test/DatabaseTestTrait.php:103
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Test/DatabaseTestTrait.php:55
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Test/CIUnitTestCase.php:249
phpvfscomposer:///home/runner/work/CodeIgniter4/CodeIgniter4/vendor/phpunit/phpunit/phpunit:106

https://github.com/codeigniter4/CodeIgniter4/actions/runs/7365649229/job/20047227573?pr=8393

@kenjis
Copy link
Member

kenjis commented Dec 30, 2023

To begin with, DatabaseLive tests fail in develop. Why?

$ vendor/bin/phpunit --group DatabaseLive
PHPUnit 9.6.15 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.2.13
Configuration: /Users/kenji/work/codeigniter/official/CodeIgniter4/phpunit.xml

..S...........................................................................S.S...S...........S.......... 107 / 667 ( 16%)
........................................................................................SSSSSSSSSSSSSSSSS.. 214 / 667 ( 32%)
........................................EEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEE 321 / 667 ( 48%)
EEEEEEEEEEEEEEEEEEEE...............................EEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEE 428 / 667 ( 64%)
EEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEE........EEEEEEEEEEEEEEEEEEEEEEEEEE 535 / 667 ( 80%)
EEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEE.....EEEEE.E.................................... 642 / 667 ( 96%)
.EEEEEEEEEEEEEEEEEEEEEEEE                                                                                   667 / 667 (100%)

Time: 00:11.947, Memory: 102.00 MB

There were 331 errors:

1) CodeIgniter\Database\Live\TransactionDBDebugFalseTest::testTransStartTransException
CodeIgniter\Database\Exceptions\DatabaseException: SQLite3::exec(): no such table: db_user

/Users/kenji/work/codeigniter/official/CodeIgniter4/system/Database/BaseConnection.php:647
/Users/kenji/work/codeigniter/official/CodeIgniter4/system/Database/BaseBuilder.php:2712
/Users/kenji/work/codeigniter/official/CodeIgniter4/tests/_support/Database/Seeds/CITestSeeder.php:180
/Users/kenji/work/codeigniter/official/CodeIgniter4/system/Database/Seeder.php:145
/Users/kenji/work/codeigniter/official/CodeIgniter4/system/Test/DatabaseTestTrait.php:205
/Users/kenji/work/codeigniter/official/CodeIgniter4/system/Test/DatabaseTestTrait.php:193
...

@paulbalandan
Copy link
Member Author

To begin with, DatabaseLive tests fail in develop. Why?

I thought this was on my machine only.

The failing test is saying DELETE FROM db_user fails because db_user does not exist, but it seems to be existing.

Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

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

Welp, I approved before I saw test results and the discussion 🤦‍♂️ Code looks good though, haha!

Copy link

👋 Hi, @paulbalandan!

We detected conflicts in your PR against the base branch 🙊
You may want to sync 🔄 your branch with upstream!

Ref: Syncing Your Branch

@github-actions github-actions bot added the stale Pull requests with conflicts label Jan 20, 2024
@kenjis
Copy link
Member

kenjis commented Jan 24, 2024

@paulbalandan I found the cause. #8447
When we use :memory:, a new connection will create a new database.
So if we change $this->db, it will connect to a different database.

@paulbalandan paulbalandan removed the stale Pull requests with conflicts label Jan 26, 2024
@paulbalandan paulbalandan force-pushed the leftover-db-files branch 5 times, most recently from 30bfaa3 to 84134b8 Compare January 29, 2024 16:11
@paulbalandan
Copy link
Member Author

I am really confused why changes to SQLite break tests on linux but not on macos.


if (is_file(WRITEPATH . 'database.db')) {
unlink(WRITEPATH . 'database.db');
}
Copy link
Member

@kenjis kenjis Jan 30, 2024

Choose a reason for hiding this comment

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

You should not remove the database file in use.
It causes "SQLite3::exec(): attempt to write a readonly database"
because the file is gone, but the connection remains.

Try:
Set database.tests.database = database.db and

$ vendor/bin/phpunit tests/system/Database/

@kenjis
Copy link
Member

kenjis commented Jan 30, 2024

I think we don't need to remove database.db. It is the main database for SQLite3 testing.
We don't remove other driver's databases in testing.

@paulbalandan
Copy link
Member Author

Yes, or it could be that there are lingering shared DB connections used by the next test instead of creating a new one.

Based on the stack trace, the error comes from setUpDatabase.

@kenjis
Copy link
Member

kenjis commented Jan 31, 2024

We use the shared DB connection during all tests by default.
Database::connect($this->DBGroup) is a static method and returns the shared instance.

if ($this->db === null) {
$this->db = Database::connect($this->DBGroup);
$this->db->initialize();

I think reconnecting for each test class would slow down the tests.

@github-actions github-actions bot added the stale Pull requests with conflicts label Feb 1, 2024
Copy link

github-actions bot commented Feb 1, 2024

👋 Hi, @paulbalandan!

We detected conflicts in your PR against the base branch 🙊
You may want to sync 🔄 your branch with upstream!

Ref: Syncing Your Branch

@kenjis kenjis removed the stale Pull requests with conflicts label Mar 10, 2024
@kenjis
Copy link
Member

kenjis commented Mar 11, 2024

runner.sqlite was removed after running tests tests/system/Database/.

@paulbalandan paulbalandan requested a review from MGatner March 11, 2024 12:36
@kenjis kenjis merged commit 985ac9f into codeigniter4:develop Mar 18, 2024
59 checks passed
@paulbalandan paulbalandan deleted the leftover-db-files branch March 18, 2024 02:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing Pull requests that changes tests only
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants