Page MenuHomePhabricator
Create Task
Maniphest T381981

Provide a way to test abstract schema (changes) in PHPUnit
Closed, ResolvedPublic
Actions

Assigned To
Daimona
Authored By
Daimona
Wed, Dec 11, 2:27 PM
Tags
Referenced Files
None
Subscribers
Aklapper
Daimona
Jdforrester-WMF
Umherirrender
Tokens
"Orange Medal" token, awarded by Krinkle.

Description

Abstract schema changes are currently not testable in PHPUnit. If a schema change file is invalid, or if a JSON file isn't used to (re-)generate .sql files, no test will fail. The only ways to tell that something is amiss are other tests that happen to use the schema (which isn't always the case, and for things like changing the default or type of a column, general-purpose tests may not catch it), and human reviewers.

Ideally, we would have a structure test that validates all the schema files specifically, and instructs the developer on how to fix the problem.

This was first being discussed in T237839 (wiring abstract schema to extension.json), but there hasn't been any progress on that task for two years, and there generally seems to be disagreements on the format of the proposed attributes. So, I am proposing that we start off by providing a structure test that doesn't rely on extension.json. The test could be made opt-in, like ExtensionJsonTestBase. It could later be deprecated in favour of (probably) a test method in ExtensionJsonTestBase itself, when T237839 is done.

Details

SubjectRepoBranchLines +/-
db: Drop AbstractSchemaValidator::$missingDepCallbackmediawiki/coremaster+6 -32
test: Introduce AbstractSchemaTestBasemediawiki/coremaster+184 -137
tests: Add structure test for content of abstract schema changesmediawiki/coremaster+177 -93
Drop orphaned JSON schema change filesmediawiki/coremaster+0 -205
Re-generate all schema change filesmediawiki/coremaster+2 K -785
maintenance: Factor out a few methods from SchemaMaintenancemediawiki/coremaster+237 -158
Customize query in gerrit

Related Objects

Event Timeline

Daimona created this task.Wed, Dec 11, 2:27 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptWed, Dec 11, 2:27 PM
Daimona mentioned this in T237839: Wire abstract schema and schema changes for extension.Wed, Dec 11, 2:28 PM
Daimona mentioned this in T317180: Auto-verify in CI the SQL files generated by Rdbms abstract schema.
Jdforrester-WMF subscribed.Wed, Dec 11, 2:30 PM
Daimona added a comment.Wed, Dec 11, 3:04 PM

I have a WIP for this that I'll push soon. One pretty obvious thing is that the schema generation needs to be decoupled from Maintenance.

gerritbot added a comment.Wed, Dec 11, 3:17 PM

Change #1102327 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/core@master] [WIP] Structure test for abstract schema

https://gerrit.wikimedia.org/r/1102327

gerritbot added a project: Patch-For-Review.Wed, Dec 11, 3:17 PM
gerritbot added a comment.Wed, Dec 11, 5:18 PM

Change #1102355 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/core@master] maintenance: Factor out a few methods from SchemaMaintenance

https://gerrit.wikimedia.org/r/1102355

gerritbot added a comment.Wed, Dec 11, 5:37 PM

Change #1102362 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/core@master] db: Drop AbstractSchemaValidator::$missingDepCallback

https://gerrit.wikimedia.org/r/1102362

Daimona claimed this task.Wed, Dec 11, 5:58 PM
Daimona added a comment.Wed, Dec 11, 6:49 PM

To no-one's surprise, basically every schema change in core no longer matches the current autogenerated output. Most of the times it's just whitespace changes, but sometimes there seem to be actual differences in the SQL. I'll look into this later.

I also don't get how people have been generating schema changes for core, given that the format expected by generateSchemaChangeSql ($common/$platform) is literally the opposite of what's in core (maintenance/archives for MySQL, but maintenance/sqlite/archives and maintenance/postgres/archives). Even the command in the documentation, which seems to use the MW core dirnames and not the more sensible extension conventions, doesn't work.

Umherirrender subscribed.Wed, Dec 11, 8:11 PM
php maintenance\generateSchemaSql.php --json maintenance --type all --sql maintenance

should work for the whole schema

For the schema changes it's needs separated commands to add the /archives/ part

php maintenance\generateSchemaChangeSql.php --json maintenance\abstractSchemaChanges\patch-user-user_is_temp.json --type mysql --sql maintenance\archives
php maintenance\generateSchemaChangeSql.php --json maintenance\abstractSchemaChanges\patch-user-user_is_temp.json --type postgres --sql maintenance\postgres\archives\patch-user-user_is_temp.sql
php maintenance\generateSchemaChangeSql.php --json maintenance\abstractSchemaChanges\patch-user-user_is_temp.json --type sqlite --sql maintenance\sqlite\archives\patch-user-user_is_temp.sql

But not all database types must have the output files, that could be the reason why schema changes are not part of tests (maybe needs check against the list of the updater). Often the files are one time patches. I am not sure if it is helpful to test them on every CI run. But the reviewer maybe does not rerun the commands to verify the output.

The abstract schema files should be part of structure tests, but that was part of other tasks and not further reviewed.

Whitespace changes are from ef338f3d56a33c75a4a2b064130f48fb2044861f

Daimona mentioned this in T382030: Canonicalize path of core schema changes.Wed, Dec 11, 8:40 PM
Daimona added a comment.Wed, Dec 11, 8:47 PM
In T381981#10398865, @Umherirrender wrote:
php maintenance\generateSchemaChangeSql.php --json maintenance\abstractSchemaChanges\patch-user-user_is_temp.json --type mysql --sql maintenance\archives
php maintenance\generateSchemaChangeSql.php --json maintenance\abstractSchemaChanges\patch-user-user_is_temp.json --type postgres --sql maintenance\postgres\archives\patch-user-user_is_temp.sql
php maintenance\generateSchemaChangeSql.php --json maintenance\abstractSchemaChanges\patch-user-user_is_temp.json --type sqlite --sql maintenance\sqlite\archives\patch-user-user_is_temp.sql

Ah, thank you, what I missed is that you should specify the full path in --sql. I already tried separating by platform, but didn't find a way to specify --sql as a directory for SQLite/Postgres and make it work. This doesn't make it less inconvenient though, so I filed T382030.

But not all database types must have the output files, that could be the reason why schema changes are not part of tests (maybe needs check against the list of the updater).

Yup, in my WIP patch I'm checking that the autogenerated file exists for at least one dialect. I don't think we can be more accurate here without T237839, but we don't need to, for now.

Often the files are one time patches. I am not sure if it is helpful to test them on every CI run. But the reviewer maybe does not rerun the commands to verify the output.

Exactly, because CI should do that for me. As long as we're tolerant with whitespace differences (which is the case for my patch), and the test isn't too expensive (and schema changes seem relatively cheap; testing the whole schema is slower, but also more important), I don't think that should be an issue.

Whitespace changes are from ef338f3d56a33c75a4a2b064130f48fb2044861f

Thanks for the pointer. For now I'm ignoring all whitespace differences. However, there seem to be non-whitespace differences that need to be addressed, especially for SQLite. I still haven't looked at those.

Umherirrender edited projects, added MediaWiki-Installer; removed MediaWiki-libs-Rdbms.Wed, Dec 11, 9:22 PM
Daimona added a comment.Thu, Dec 12, 1:31 AM

The non-whitespace changes are DROP INDEX statements being removed since doctrine/dbal 3.4.0, for example in maintenance/sqlite/archives/patch-change_tag-rename-indexes.sql. I think we should just regenerate all the schema change files with non-trivial differences. There's ~30 of them.

gerritbot added a comment.Thu, Dec 12, 3:51 PM

Change #1102362 merged by jenkins-bot:

[mediawiki/core@master] db: Drop AbstractSchemaValidator::$missingDepCallback

https://gerrit.wikimedia.org/r/1102362

ReleaseTaggerBot added a project: MW-1.44-notes (1.44.0-wmf.8; 2024-12-17).Thu, Dec 12, 4:00 PM
gerritbot added a comment.Thu, Dec 12, 8:06 PM

Change #1102355 merged by jenkins-bot:

[mediawiki/core@master] maintenance: Factor out a few methods from SchemaMaintenance

https://gerrit.wikimedia.org/r/1102355

gerritbot added a comment.Thu, Dec 12, 10:53 PM

Change #1102991 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/core@master] Re-generate all schema change files

https://gerrit.wikimedia.org/r/1102991

gerritbot added a comment.Thu, Dec 12, 11:05 PM

Change #1102992 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/core@master] Drop orphaned JSON schema change files

https://gerrit.wikimedia.org/r/1102992

gerritbot added a comment.Fri, Dec 13, 1:55 AM

Change #1102995 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/core@master] test: Introduce AbstractSchemaTestBase

https://gerrit.wikimedia.org/r/1102995

gerritbot added a comment.Fri, Dec 13, 3:55 PM

Change #1102991 merged by jenkins-bot:

[mediawiki/core@master] Re-generate all schema change files

https://gerrit.wikimedia.org/r/1102991

gerritbot added a comment.Fri, Dec 13, 3:57 PM

Change #1102992 merged by jenkins-bot:

[mediawiki/core@master] Drop orphaned JSON schema change files

https://gerrit.wikimedia.org/r/1102992

gerritbot added a comment.Tue, Dec 17, 4:02 PM

Change #1102327 merged by jenkins-bot:

[mediawiki/core@master] tests: Add structure test for content of abstract schema changes

https://gerrit.wikimedia.org/r/1102327

Jdforrester-WMF added a comment.Tue, Dec 17, 4:11 PM

Resolved?

Daimona added a comment.Tue, Dec 17, 4:12 PM
In T381981#10409957, @Jdforrester-WMF wrote:

Resolved?

Yeah, I was just waiting for r1102995 to have a couple random selenium failures and eventually get merged.

gerritbot added a comment.Tue, Dec 17, 4:20 PM

Change #1102995 merged by jenkins-bot:

[mediawiki/core@master] test: Introduce AbstractSchemaTestBase

https://gerrit.wikimedia.org/r/1102995

Daimona closed this task as Resolved.Tue, Dec 17, 4:28 PM
In T381981#10409959, @Daimona wrote:
In T381981#10409957, @Jdforrester-WMF wrote:

Resolved?

Yeah, I was just waiting for r1102995 to have a couple random selenium failures and eventually get merged.

Or not.

Maintenance_bot removed a project: Patch-For-Review.Tue, Dec 17, 4:30 PM
ReleaseTaggerBot edited projects, added MW-1.44-notes (1.44.0-wmf.12; 2025-01-14); removed MW-1.44-notes (1.44.0-wmf.8; 2024-12-17).Tue, Dec 17, 5:00 PM
Krinkle awarded a token.Sat, Dec 21, 12:00 AM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL · Credits
  NODES
Idea 1
idea 1
Note 5
Project 5
Verify 4