Page MenuHomePhabricator

Remove MediaWikiIntegrationTestCase::$tablesUsed in favour of automatic query tracking
Closed, ResolvedPublic

Description

MediaWikiIntegrationTestCase::$tablesUsed is used to specify what tables are used by a test class, and therefore need to be cleaned up when each test in that class ends. This has a number of issue. The most prominent one is that developers need to make sure that the property is always up-to-date with what the test does. Particularly problematic is the scenario where a table used by the test is not in $tablesUsed, because that table will never be cleared, and will remain available to subsequent tests (which could fail if they assume the table to be empty).

Instead, we could keep track of every query executed, make a list of tables that were changed, and reset those table. If needed, we could provide an overridable method for tests to specify that certain tables, even if used, should not be reset. I'm not sure if this would really be any useful, so I'd rather not do this unless there's a valid use case.

Related Objects

Event Timeline

Also, I'm super confused by the documentation in https://www.mediawiki.org/wiki/Manual:PHP_unit_testing/Writing_unit_tests#Databases:

If you want to use another DB table (say, for your extension), it is not available by default for testing. To add the table to the temporary database constructed during testing, you need to add it to MediaWikiTestCase::$tablesUsed
[...]
However, please note that only the table schema is copied over from your actual database, not the existing data in that table.

I don't know if this is how it worked in the past, but it's not how it works nowadays. All tables in the real database are available for testing; tablesUsed only controls what tables are reset after each test.

All tables in the real database are available for testing; tablesUsed only controls what tables are reset after each test.

There’s also some code that tries to use tablesUsed to control which tables are reset before a test, though it’s not quite working at the moment (T265033). But if automatic query tracking can be made to work, I guess that wouldn’t be needed anymore – if all tables are cleared after every test, they should be empty at the beginning of every test already.

If needed, we could provide an overridable method for tests to specify that certain tables, even if used, should not be reset. I'm not sure if this would really be any useful, so I'd rather not do this unless there's a valid use case.

Agreed.

All tables in the real database are available for testing; tablesUsed only controls what tables are reset after each test.

There’s also some code that tries to use tablesUsed to control which tables are reset before a test, though it’s not quite working at the moment (T265033). But if automatic query tracking can be made to work, I guess that wouldn’t be needed anymore – if all tables are cleared after every test, they should be empty at the beginning of every test already.

Indeed, I think the implementation should make sure that all tables are reset, and each test starts with a clean database.

One question I had is whether we already have something that keeps track of all queries, or whether it'd have to be implemented specifically for this purpose. I suspect we may have something similar, but I'm not that familiar with the RDBMS library. Our requirements here are also pretty simple, we just need a list of tables that were part of at least one write query.

One thing to keep in mind here is that whatever we use to track queries, we must guarantee that it cannot be replaced/disabled (unintentionally) in any way by tests. Otherwise we may fail to properly clean up the database, if the relevant service is overridden. Instead, I'm thinking of a simple class with just a bunch of static methods (add, get, reset). Database::attemptQuery would add queries to it (only if MW_PHPUNIT_TEST is defined), and MediaWikiIntegrationTestCase would read those queries and reset the list after each test. I will experiment with this once T155147 is resolved.

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

[mediawiki/core@master] [WIP][POC] ChangedTablesTracker

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

Another two things in favour of this:

  1. Data added in addDBDataOnce is currently difficult to delete: if you add the tables to tablesUsed, it will be deleted after the first test and not added again. And if you don't add those tables to tablesUsed, the data just remains there forever.
  2. Some writes are out of the caller's control, and so it's hard to impossible to anticipate them. For instance, a simple page creation could create CheckUser entries, or PageTriage records, if those extensions are installed. These won't be cleaned up unless tablesUsed includes those tables explicitly. This can lead to the weirdest failures due to tests leaving crap behind. Something like r946549 for T342428 keeps failing CI for mysterious reasons, probably related to garbage test data not being cleaned up.

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

[mediawiki/extensions/Wikibase@master] tests: Use addDBDataOnce instead of DIY implementation

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

Change 949602 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] tests: Use addDBDataOnce instead of DIY implementation

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

Change 946538 merged by jenkins-bot:

[mediawiki/core@master] Replace MediaWikiIntegrationTestCase::$tablesUsed with automatic query tracking

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

The initial work was done. Tim is working on further improving the tracker, but tablesUsed has already been deprecated.

This is a nice improvement, thank you all!

Change 976228 had a related patch set uploaded (by Jforrester; author: Jforrester):

[mediawiki/core@master] Follow-up 71ff05267: Stop writing to tablesUsed in tests, now unnecessary

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

Change 976228 merged by jenkins-bot:

[mediawiki/core@master] Follow-up 71ff05267: Stop writing to tablesUsed in tests, now unnecessary

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

Ladsgroup subscribed.

Wow I didn't know this happened, awesome work!