Page MenuHomePhabricator

SQlite has wrong DB structure after upgrading to 1.35
Closed, ResolvedPublic

Description

Motivation

At the moment after incremental upgrade DB to 1.35, there's a difference between MySQL and SQLite DB structure:

fields related to 'actor' have default 0:

  • ar_actor
  • img_actor
  • fa_actor
  • etc...

Expected Result:

  1. DB structure should be equal. Defaults should be dropped by the SQLite's incremental upgrade scripts.
  2. Tests should pass.

Additional Info:

1) DatabaseSqliteTest::testUpgrades
Default values does not match for column filearchive.fa_actor upgrading from 1.15 to 1.35.0-alpha
Failed asserting that '0' matches expected null.

/Users/peter/work/Wiki/gerrit/mediawiki/tests/phpunit/integration/includes/db/DatabaseSqliteTest.php:399
/Users/peter/work/Wiki/gerrit/mediawiki/tests/phpunit/MediaWikiIntegrationTestCase.php:417

FAILURES!
Tests: 42, Assertions: 407, Failures: 1, Risky: 1.
Script phpunit --colors=always --testsuite=core:integration,extensions:integration,skins:integration handling the phpunit:integration event returned with error code 1

Event Timeline

daniel triaged this task as High priority.Feb 26 2020, 10:28 AM
daniel added subscribers: Ladsgroup, Anomie.
daniel added a subscriber: daniel.

Bumping to high, since this seems to break sqlite installs

Hmm. It looks like rMW214750d8d224: Define unit and integration test suites may have broken the running of this test in CI, when it moved the test from tests/phpunit/includes to tests/phpunit/integration but never added the latter path to tests/phpunit/suite.xml.

When I try to run it locally, it doesn't even get to the failure quoted, instead it dies on "Undefined index: topologyRole" (added in fb621c26a37, and the necessary change to this test wasn't done because the test wasn't being run). Fixing that turns up several more failures.

I guess you must be using composer phpunit:integration to try to run tests? When I try to run that locally, it dumps a bunch of warnings then blows up with a ContainerDisabledException. When I try it with only core:integration (to skip it trying to run tests for extensions I have in my extensions directory but don't have set up and enabled), it dumps even more warnings and fails with this error plus 5 others that seem to be from 673d496f. Given all that I have to wonder if this testing entry point is actually used anywhere.


As for this task itself, it looks like rMWc29909e59fd8: Mostly drop old pre-actor user schemas missed including the change of default for the xx_actor fields in the SQLite versions of the patches.

AFAIK composer phpunit:integration is not used in CI, because we wanted to avoid duplicate test runs. Part of T87781 implies eventually migrating to the plain PHPUnit entrypoint and removing tests/phpunit/phpunit.php so we only have one way to run these tests instead of two.

when it moved the test from tests/phpunit/includes to tests/phpunit/integration but never added the latter path to tests/phpunit/suite.xml.

Yes, sorry. Do you want to submit a patch or should I?

AFAIK composer phpunit:integration is not used in CI, because we wanted to avoid duplicate test runs. Part of T87781 implies eventually migrating to the plain PHPUnit entrypoint and removing tests/phpunit/phpunit.php so we only have one way to run these tests instead of two.

We should probably take this discussion to a different task. Do you think T227900 would be appropriate?

when it moved the test from tests/phpunit/includes to tests/phpunit/integration but never added the latter path to tests/phpunit/suite.xml.

Yes, sorry. Do you want to submit a patch or should I?

At the moment there seems to be three parts: the failure described in this task (currently claimed by @Peter.ovchyn), the failures due to changes in 673d496f, and then the actual re-enabling. I'll be happy to review if the two of you would like to divide up the work (or one of you just wants to do all of it).

We might want to retitle and expand this task to cover all three parts, or else make a parent task to this one covering all the parts.

It looks like T245995 was already filed for the second of the three parts, and has a patch.

We should probably take this discussion to a different task. Do you think T227900 would be appropriate?

Yeah let's discuss there

This issue is 100% exactly what described.

There're SQL scripts with that drop defaults for fields like filearchive.fa_actor, etc for Postgres and MySQL, but no the same for SQLite.

See patch-drop-user-fields.sql. No equivalent for SQLite. When I have time, I'll fix this.

Change 581884 had a related patch set uploaded (by Aaron Schulz; owner: Aaron Schulz):
[mediawiki/core@master] Drop DEFAULT from *_actor columns for SQLite tables that still have it

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

Change 581884 merged by jenkins-bot:
[mediawiki/core@master] Drop DEFAULT from *_actor columns for SQLite tables that still have it

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