Execute the schema, rather than only reading it - #1225
Merged
Conversation
tests/schema-portable-collation.test.php checks the two schema files textually and rejects a collation outside a portable allowlist. That catches the shape of GH-1147, but only the shape: it cannot catch a statement that is well-formed, names nothing unusual and still will not run on MariaDB 10.5, and it cannot see what the schema RESOLVES to once a server has applied its own defaults, which is the whole of GH-1152. It is textual by design, and its docblock says why -- commons/schema.php "calls self::$DB->query() at file scope and cannot be included without a database". So this test supplies the database. TWO SCRATCH DATABASES, BECAUSE THERE ARE TWO ROUTES The steps in commons/schema.php reach a server one way and the manifest's `create` strings reach it another, so they get one database each. The first replays the indexed steps in order, which is the fresh-install path: SchemaUpdaterPage::update() slices the step array from self::$mySchema, and that is 0 on a new database, so a fresh install really does execute every step from the beginning. The second runs commons/schema-expected.php's `create` strings into an EMPTY database -- the reconciler path. An empty one specifically: run against the first, every table already exists and CREATE TABLE IF NOT EXISTS may short-circuit without the server ever resolving the collation names, which would leave the GH-1147 guard looking like it passed when nothing was checked. Each is then asserted twice: every statement executed (GH-1147), and every resulting table shares one collation (GH-1152). HOW schema.php IS LOADED WITHOUT THE APPLICATION It is include'd from inside SchemaUpdaterPage::update() and expects that method's context -- $this->schema[], self::$DB, self::getClass(). It carries no `namespace`, since the real classes reach it through class_alias into the global namespace, so global stubs are what it resolves against. Two of its dependencies are discovered rather than listed, so that adding either to schema.php cannot quietly break this test. Constants are found by tokenising the file and given placeholders; they only ever reach INSERT statements seeding default settings, and only DDL is executed, so the values do not matter -- DATABASE_NAME and FOG_SCHEMA are set for real because they do. Unknown classes are manufactured by an autoloader whose every call answers null, which is what an empty database looks like to the introspection schema.php performs while building its array. Empty is the correct answer there: a fresh install is exactly the state being modelled, and DatabaseManager::getColumns() returning nothing is why the conditional ALTERs are included, same as on a real one. Measured on this branch, the collection yields 340 indexed steps, 658 string statements, 278 of them DDL, 74 CREATE TABLE, and 9 closures. WHAT IT DOES NOT COVER The closure steps are not executed. They are data migrations that need the full application and emit no DDL. Stated in the docblock rather than left to be discovered: this covers the schema's structure, not its data. Without FOG_TEST_DSN it prints SKIP and exits 0, so `sh tests/run-all.sh` on a machine with no database still reports a green suite -- the convention secureboot-authvars.test.sh already uses for absent efitools. With a DSN set but pdo_mysql missing it FAILS instead, deliberately: a DSN was provided, so skipping would report a pass for a check that never ran. The runner picks it up through its existing *.test.php glob; nothing to register. ALSO, THE JOB NAME .github/workflows/tests.yml declared a bare `suite:`, and the job key is what GitHub puts in the middle of a called workflow's check name, so every check read "Tests / suite / tests (PHP 7.4)". Named `fogproject`, because fog-workflows hosts fog-plugins-tests.yml alongside fogproject-tests.yml and which project's suite is running is the useful thing for that segment to say. This does change the reported check name. Safe because working-1.6 has neither branch protection nor a ruleset requiring status checks -- verified through the API rather than assumed. The workflow itself keeps the name `Tests`. Part of #1221 Refs #1147, #1152 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PqufBbuckux8kitJeW3uAK
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.
Closes #1221
Refs #1147, #1152
Why
tests/schema-portable-collation.test.phpchecks the two schema files textually and rejects acollation outside a portable allowlist. That catches the shape of #1147, but only the shape. It
cannot catch a statement that is well-formed, names nothing unusual, and still will not run on
MariaDB 10.5 — and it cannot see what the schema resolves to once a server has applied its own
defaults, which is the whole of #1152.
It is textual by design, and its docblock says why:
commons/schema.php"callsself::$DB->query()at file scope and cannot be included without a database". So this testsupplies the database.
Two scratch databases, because there are two routes
schema.php's indexed steps in orderSchemaUpdaterPage::update()slices fromself::$mySchema, which is 0 on a new database, so a fresh install really does run every stepschema-expected.php'screatestrings into an empty databaseEmpty specifically: run against database 1, every table already exists and
CREATE TABLE IF NOT EXISTSmay short-circuit without the server ever resolving the collationnames — leaving the #1147 guard looking like it passed when nothing was checked.
Each is asserted twice: every statement executed (#1147), and every resulting table shares
one collation (#1152).
Loading schema.php without the application
It is
included from insideSchemaUpdaterPage::update()and expects that method's context —$this->schema[],self::$DB,self::getClass(). It carries nonamespace, since the realclasses reach it via
class_aliasinto the global namespace, so global stubs are what it resolvesagainst.
Two dependencies are discovered rather than listed, so adding either to
schema.phpcannotquietly break this test:
INSERTstatements seeding default settings and only DDL is executed, so the values do not matter —
DATABASE_NAMEandFOG_SCHEMAare set for real because they do.what an empty database looks like to the introspection
schema.phpperforms while building itsarray. Empty is the correct answer: a fresh install is the state being modelled, and
DatabaseManager::getColumns()returning nothing is exactly why the conditionalALTERs areincluded, same as on a real one.
Measured on this branch:
What it does not cover
The closure steps are not executed — data migrations needing the full application, emitting no
DDL. Stated in the docblock rather than left to be discovered: this covers the schema's structure,
not its data.
Skipping
Without
FOG_TEST_DSNit printsSKIPand exits 0, sosh tests/run-all.shwith no databasestays green — the convention
secureboot-authvars.test.shalready uses. With a DSN set butpdo_mysqlmissing it fails instead: a DSN was provided, so skipping would report a pass for acheck that never ran.
Picked up by the runner's existing
*.test.phpglob; nothing to register.Also: the job name
.github/workflows/tests.ymldeclared a baresuite:, and the job key is what GitHub puts in themiddle of a called workflow's check name — so every check read
Tests / suite / tests (PHP 7.4).Named
fogproject, because fog-workflows hostsfog-plugins-tests.ymlalongsidefogproject-tests.yml, and which project's suite is running is the useful thing for that segmentto say.
This does change the reported check name. Safe because
working-1.6has neither branchprotection nor a ruleset requiring status checks — checked through the API, not assumed. The
workflow itself keeps the name
Tests.Verification
run-all.shpicks it up and stays green.client and no WSL distro. The three CI legs are the first real execution, which is precisely the
gap No CI executes the schema against a real database server #1221 describes.
🤖 Generated with Claude Code
https://claude.ai/code/session_01PqufBbuckux8kitJeW3uAK