test(Database): fix state leaks and random-order test determinism for MySQLi, PostgreSQL, and OCI8#10377
Open
gr8man wants to merge 6 commits into
Open
test(Database): fix state leaks and random-order test determinism for MySQLi, PostgreSQL, and OCI8#10377gr8man wants to merge 6 commits into
gr8man wants to merge 6 commits into
Conversation
- Connection::initialize() sets NLS session formats via ALTER SESSION, preventing ORA-01843 when env vars are not set. - _indexData() handles null CONSTRAINT_TYPE to avoid deprecated null array offset error. - MetadataTest captures prefix before createExtraneousTable() to prevent prefix leak on exception. - ExecuteLogMessageFormatTest removes stale OCI8-specific array_slice.
Contributor
Author
|
IMPORTANT Note to reviewers and developers: Please ensure that tests are run regularly with the --order-by=random flag during development. Identifying test pollution and global state leakage bugs at a later stage is extremely difficult and time-consuming, as these failures only manifest under very specific test suite execution orders. |
There was a problem hiding this comment.
Pull request overview
This PR improves determinism of the database test suite when executed in randomized order by removing shared connection state leaks (notably DB prefix/config caching), adding stronger cleanup between tests, and hardening OCI8 behavior/configuration to avoid environment-dependent failures.
Changes:
- Avoid shared DB connection/config state in several live driver tests by connecting with
getShared = falseand resetting cached instances where needed. - Add/adjust cleanup to prevent cross-test state leakage (table cleanup, cache resets, preserved prefixes).
- OCI8: set session NLS date/timestamp formats during initialization and harden index metadata handling to avoid PHP 8.1+ deprecations.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/system/Database/Migrations/MigrationRunnerTest.php | Adjusts teardown cleanup for migration runner tests |
| tests/system/Database/Live/WorkerModeTest.php | Minor teardown formatting cleanup |
| tests/system/Database/Live/Postgre/ConnectTest.php | Uses non-shared connection to avoid state leakage during charset test |
| tests/system/Database/Live/MySQLi/NumberNativeTest.php | Uses non-shared connections to avoid config/prefix bleed between tests |
| tests/system/Database/Live/MySQLi/FoundRowsTest.php | Uses non-shared connections to avoid config/prefix bleed between tests |
| tests/system/Database/Live/MetadataTest.php | Preserves original prefix reliably even on exceptions |
| tests/system/Database/Live/GetVersionTest.php | Relaxes version assertion pattern |
| tests/system/Database/Live/ForgeTest.php | Centralizes mock-table cleanup and improves DB existence handling in tests |
| tests/system/Database/Live/ExecuteLogMessageFormatTest.php | Makes backtrace line detection more robust across drivers |
| tests/system/Database/Live/ConnectTest.php | Resets cached Database instances; makes instance-count assertion resilient |
| system/Database/OCI8/Connection.php | Sets OCI8 session NLS formats and hardens index metadata mapping |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
68
to
73
| protected function tearDown(): void | ||
| { | ||
| parent::tearDown(); | ||
|
|
||
| // To delete data with `$this->regressDatabase()`, set it true. | ||
| $this->migrate = true; | ||
| $this->regressDatabase(); | ||
| $this->resetTables(); | ||
| } |
Comment on lines
+79
to
+84
| protected function tearDown(): void | ||
| { | ||
| parent::tearDown(); | ||
| $this->dropAllMockTables(); | ||
| self::$doneMigration = false; | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes database tests to be deterministic under random execution order
for MySQLi, PostgreSQL, and OCI8 drivers.
Ref: #9968
MySQLi (commits cd8f5dc, 8cc20d3)
prefix state to subsequent tests.
PostgreSQL (commit 24d5a44)
table name after creation.
OCI8 (commit fc9e325)
and NLS_TIMESTAMP_TZ_FORMAT via ALTER SESSION after connecting.
Prevents ORA-01843 "invalid month" when NLS env vars are not set.
$row->CONSTRAINT_TYPE ?? ''to avoid PHP 8.1+deprecation "Using null as an array offset" for indexes without a
matching constraint.
original prefix is preserved even if an exception is thrown.
that caused a backtrace line pattern mismatch.
All 859 OCI8 database tests pass with 0 failures in both default and
randomised execution order. MySQLi and PostgreSQL tests also pass
deterministically under random order.