[00:34:12] TimStarling: I was actually shocked the first time I heard a French person say "tabluh" (and other similar -le words). [00:35:02] I did not realise it was actually pronounced that way. I knew it came from the same origin, but always thought it was an ancient way to spell what we pronounce today, as opposed to an ancient pronunciation spelled in a sane way. [00:36:53] wiktionary says "an early Germanic borrowing of Latin tabula" [00:37:16] so I guess the germanic convention was to drop the final vowel in latin borrowings [00:38:26] why do we do service resets on setup, rather than teardown? [00:38:51] seems like if you make a mess, it should be your own job to clean it up rather than leaving it for the next guy [00:54:16] never mind [02:02:34] Krinkle: it expects that CWD is the git repo root [02:02:54] So Jenkins will cd to the right place for extension jobs [02:04:46] cool [02:04:55] I didn't spot the 'cd'. Nice [02:07:01] I designed it to work for the 90% case, so schema changes and stuff like new composer dependencies are pretty rare that it wasn't worth designing for [03:37:57] TimStarling: I think Aryeh had a patch or was working on resetting all services before each test [04:02:10] the MCR tests are broken for various reasons, so I'm neck deep now in fixing them [04:02:35] but services being reset on setup was mostly misdiagnosed, services are injected on setup [10:29:10] DanielK_WMDE_: the way addCoreDBData() is overridden with an empty function is certainly wrong and broken, I tried fixing it but it took me a while to really understand why it is like that [10:29:58] TimStarling: I'd really love to get rid of that entirely, the vast majority of tests doesn't need that stuff... [10:30:02] but how is it broken? [10:30:16] addCoreDBData() is called *after* test execution from resetDB(), it is setting up the environment for the subsequent test [10:30:33] if oncePerClass() is true it's called both before and after [10:30:52] so when it's overridden to be empty, it leaves the database in the wrong state for the subsequent test [10:30:54] (btw, i'm working with adam on finding out what pollutes the NameTableStore cache and causes backuptestPassTest to fail. seems like some Flow tests don't declare that they use the database) [10:31:59] if you do [10:32:00] mwscript tests/phpunit/phpunit.php tests/phpunit/includes/Storage --filter 'MediaWiki\\Tests\\Storage\\(PageUpdaterTest|McrReadNewRevisionStoreDbTest).*' [10:32:11] then you can see errors caused by this DB pollution [10:32:59] i'll try that, thanks. i'm a bit confused, since none of these tests should use any of the UTpage stuff [10:33:35] but you are right - overriding addCoreDBdata to be empty, it won't run before the following test, which is bad if that test expects it to be run. [10:33:35] it may be a separate issue, not related to the backupDumper thing, if that's what you mean [10:33:48] i'm pretty usre it's separate [10:34:02] the complication is the way schema overrides are done [10:34:51] yea, that'S a bit convoluted. didn't find a better way to fit them in [10:34:58] normally you have setup and teardown functions, resetDB() is trying to tear down the database, restoring its state to the default [10:35:36] but schema overrides are left in place until the next database setup, in fact they stay in place until the next test with needsDB() is run [10:35:48] only then are they reverted [10:36:10] so you have teardown attempting to happen with the modified schema [10:36:22] the idea is that each test run defines what fixtures it needs, imposes the guarantees it needs on its environment. [10:36:56] in my mind, ideally, teardown doesn't do anything except perhaps reclaim disk space or somethign like that [10:37:23] the next test shouldn't have to rely on anything the teardown of the rpevio0us test did. i know this isn't the case, but in my mind, that's how it should be [10:37:47] not even close [10:37:50] teardown happening with the modified schema shouldn't actually be a problem [10:38:11] it tries to insert rows into a dropped table, that causes an error [10:38:52] hm, i wrote code to prevent that kind of thing [10:38:54] i'll investigate [10:39:25] I mean if I revert your attempt at fixing this then it inserts rows into a dropped table [10:39:51] specifically if the addCoreDBData() overrides are removed, then it inserts rows into a dropped table [10:40:31] right [10:41:59] anyway, tests do nothing comparable to setting up the whole environment and DB every time, this is apparently a deliberate choice for performance [10:42:01] i'm not sure into which direction i'd want to fix that - make addCoreDBData always work, or just remove it, and provide a trait for the handful of tests that actually need it. [10:42:32] yes, sure - tests declare what tables they want to have reset. [10:42:46] but if they declare that, why not do that reset *before* the tests run, instead of after? [10:43:20] if these are the tables relevant for the test, that should work just as well, or better, right? [10:43:53] maybe [10:44:00] obviously it doesn't though [10:44:59] one plan I have for now is to make sure tests *never* access the live DB, reading or writing. [10:45:25] it'S always a problem if they do, and hard to track down. [10:47:30] TimStarling: one big problem i see with tests declaring which tables to reset is that this is utterly unreliable. they just don't know, and can't. a test inserting a RecentChanges row may or may not insert a row into the actor table, and into a comment table. [10:48:07] The test doesn't know or care. any attempt to declare all tables affected is doomed to go out of sync [10:48:12] we hjave that issue all over the place [10:49:01] I'm thinking that resetting all tables, always, but in exchange only inserting the "core data" stuff when needed, would perform better than what we do now. [10:52:01] the problem is that addCoreDBData() is not done during test setup [10:52:18] it's only done if self::$dbSetup is false which it isn't [10:53:50] TimStarling: no, it's also triggered by reserDB, if any "page tables" were reset [10:53:55] line 1797 [10:54:39] ah - you mean that'S teardown, not setup [10:55:24] i'm tempted to try moving resetDB into setup instead of teardown, and see what happens :) [10:56:32] I'm looking at a debug log in which addCoreDBData() is plainly not called, but let me drill down a bit further [10:56:51] TimStarling: anyway, my biggest problem right now is that I can't reproduce the error I'm seeing in CI. I can reproduce a similar error locally, but only if I run the entire Database group. [11:00:13] where does tablesUsed come from? [11:00:33] from the individual test cases [11:00:49] usually from the setUp() function [11:02:15] it's supposed to be the tables that are written to? [11:04:37] TimStarling: yes. but it's very unreliable. close to useless, to be honest. [11:05:14] sorry to go on about something that is probably unrelated to your problem, but when resetDB() is called during setup, it is only resetting tables that are in $this->tablesUsed, i.e. the tables that are used by the new test [11:05:26] TimStarling: but having anything in there triggers needsDB(), just like @Database does. And that causes all "page tables" to be reset. page, revision, etc. The stuff listed in resetDB(). [11:06:03] TimStarling: line 1768: $tablesUsed = array_unique( array_merge( $tablesUsed, $userTables ) [11:06:21] and, more relevant: line 1772: $tablesUsed = array_unique( array_merge( $tablesUsed, $pageTables ) ) [11:06:33] that stuff always gets reset [11:07:00] ...if there is anything in tablesUsed or @group Database is found [11:07:37] you mean if $tablesUsed has any page table then all page tables are reset [11:08:11] if ( array_intersect( $tablesUsed, $pageTables ) ) { [11:08:11] $tablesUsed = array_unique( array_merge( $tablesUsed, $pageTables ) ); [11:08:11] } [11:08:14] yes, sorry for being imprecise [11:10:38] TimStarling: you said "but when resetDB() is called during setup, it is only resetting tables that are in $this->tablesUsed, i.e. the tables that are used by the new test". Yes, that'S my idea. Why should we reset anything not used by the new test? [11:10:50] why would the test care about anything it doesn't use? [11:11:33] it'S an idea i'd liek to explore at some point [11:11:41] this is the problem with calling it $tablesUsed instead of $tablesWrittenTo [11:11:57] but it'S for later, really. for now, i want to expose any spurious access to the "real" database [11:12:37] ok, so in this case, $tablesUsed should not be empty [11:12:46] yes [11:12:51] well, there is another issue [11:12:57] but in general the problem is that it's trying to read from site_stats, not write to it [11:13:00] test cases accessing the database in datab providers [11:13:07] and site_stats was mangled by the previous test [11:15:00] the test really wants all tables that it reads from populated on setup, not just the ones that it writes to [11:15:50] the problem with site_stats is that it's supposed to always have content. [11:16:10] it needs special treatment to re-initialize is after any reset [11:16:51] but yes, i agree that ideally, tablesUsed should be populated to contain everything that the test uses in any way, read or write [11:17:36] TimStarling: truncateTable() has a special case for the interwiki table. It probably do somethign similar for site_stats [11:17:55] (I'm not sure why the thing for interwiki is needed - it probably isn't) [11:19:33] no, I think tests should know which tables were written to and just reset those, it's too expensive to truncate a table every time it is read [11:19:39] we can do this using triggers [11:19:59] we can have a trigger on every unittest_* table [11:20:11] the question is really what tables a test case can expect to have any content per default [11:20:23] i think it's too expensive to always maintain default content [11:20:38] tests should declare which tables they want to have initialized [11:22:01] site_Stats may be a notable exception, actually [11:23:59] you want to rewrite all the tests [11:24:34] no. very few tests actually rely on there being anything in the tables per default [11:24:42] how many exactly? [11:24:47] a few do, but we could just have a trait that could be sued to restore the old behavior [11:25:09] I mean, I'm being facetious, it would take me a day to count them [11:25:19] i don't remember exactly, I seem to recall something like 20 classes or so. I'd have to check again [11:25:53] why count them - just force all tables to be empty, and see what tests fail :) [11:26:22] I'm saying if the problem with TextPassDumperDatabaseTest is something setting $tablesUsed incorrectly, we can debug that with triggers [11:27:10] either $tablesUsed or @group Database can be debugged that way [11:27:38] ah right [11:27:51] I was going for a hack in the Database class. [11:28:03] I'm actually nearly done with that, give me a few minutes [11:28:17] a temporary hack? [11:28:50] yes [11:28:55] DNM [11:31:56] "You cannot associate a trigger with a TEMPORARY table or a view." [11:32:00] bugger [11:34:47] TimStarling: how about associating a trigegr with all *real* tables, causing any access to fail? [11:34:58] TimStarling: but anyway, ehre we go: https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/455633 [11:36:03] that's not so bad, something like that could be permanent [11:36:21] yea, see addshore's comment to the previous PS [11:37:05] I'm hading out for lunch while this runs [11:37:12] bbl [12:31:10] tgr: im curious, whats the status of https://phabricator.wikimedia.org/T91649 ? [12:33:08] eek, MutableRevisionRecordTest does "bad" db access in a data provider. that'S one of mine. [12:34:17] interwiki table [12:34:19] *sigh* [12:35:28] ApiQuerySearchTest seems to load full revisions in a data provider. that would cause the NameTabelStore to be initialized with "live" data, and later use a completely different table, because the table prefix gets changed. [12:35:32] That's probably it. [13:53:10] orrr. more broken flow tests [14:15:07] the more i fix, the more i find... [14:15:13] amazing! [15:09:11] addshore: no one's working on it, unlikely to happen as a volunteer project. Reading Web has a pilot project for low-effort error collection (plain EventLogging, like it's done in UploadWizard) and might ask for Sentry to be resourced if that turns out to be not so useful. [16:28:37] thanks! [19:50:05] tgr, MatmaRex: https://gerrit.wikimedia.org/r/c/mediawiki/core/+/455910 [19:51:05] thanks [22:33:48] anomie: did you have a chance to look at the RevisionRenderer patch? [22:34:09] I think the latest version accounts for all the concerns you had