[01:43:57] how am I meant to refactor prepareConnectionForTesting() when it was introduced for future use and still has no callers in core or extensions after a month? [01:44:16] DanielK_WMDE: ^ [02:09:25] TimStarling: https://gerrit.wikimedia.org/r/c/mediawiki/core/+/455191 I guess? [02:10:50] right [02:13:47] so this is related to your question on my interwiki table change [02:14:55] I'm somewhat interested in fixing that test, but I'm more interested in fixing the failure mode, by changing the way we do DB management in tests [02:16:39] how do you want to change it? [02:20:01] I was talking to you before about deriving tablesUsed automatically by using a proxy database object which tracks the tables [02:20:34] ideally there would only be one such object [02:20:44] I'm looking at other ways to answer Daniel's use case in BackupDumper [02:21:28] right [02:22:59] the other thing I want to do is split out database management to a separate class, since MediaWikiTestCase is an awkward place to do it [02:23:14] but that requires quite a lot of analysis [02:23:42] would https://gerrit.wikimedia.org/r/c/mediawiki/extensions/ActiveAbstract/+/456687/6/tests/phpunit/BackupDumperAbstractsTest.php be related to the BackupDumper stuff? [02:25:34] yes [02:26:10] if possible we should get rid of the unbuffered query from BackupDumper and use an index instead [02:27:49] you know BackupDumper tries to dump the whole database using a single query, which is quite a complex technique, and becoming more complex as time goes by [02:29:32] the last time I looked into the coverage of @group Database, it was sorely lacking in extension tests: https://gerrit.wikimedia.org/r/c/mediawiki/core/+/200284 [02:31:48] that's an interesting change [02:34:05] on another subject, did you know that all TestCase objects are kept alive, in TestSuite arrays, until phpunit.php terminates? I think this is the main reason why running tests requires hundreds of megabytes of memory [02:36:34] uhh, wow [02:37:11] the PHPUnit manual does say this: "However, if you create many objects in your setUp(), you might want to unset() the variables pointing to those objects in your tearDown() so they can be garbage collected. The garbage collection of test case objects is not predictable." [02:37:26] in fact it happens predictably at termination but you get the idea [02:38:39] I installed a little extension called meminfo https://github.com/BitOne/php-meminfo [02:40:38] https://github.com/sebastianbergmann/phpunit/issues/3213 looks interesting [02:40:50] specifically "After a test has been executed the respective test case object is destructed" [02:42:19] yes, and recent [02:47:32] ok WikiExporter is a huge can of worms [02:48:07] I should definitely back away and pretend I never saw this [02:48:16] (or at least clear my email inbox first) [02:53:53] I should file a bug [03:00:11] might be one to delegate to bpirkle [03:09:28] umm, bbl, going to pick up subbu... [03:26:45] there is the dumps-rewrite project in phabricator which is mostly just a bunch of unanswered questions of varying quality [03:27:05] with 128xxx and 129xxx bug numbers [03:29:07] wow, even with subsubtasks [04:22:42] o/ [04:32:07] TimStarling: do you know/remember what the purpose of --with-phpunitclass was? [04:32:27] nvm, found it [04:32:30] 110dc4d9a20b001e [04:32:31] yes, I did a git blame [04:32:58] that's why I mentioned --order-by=random, the original rationale was to use https://github.com/fiunchinho/phpunit-randomizer [04:35:08] seems reasonable, though we won't be able to use PHPUnit 7 until we drop PHP 7.0 support (it requires 7.1+) [04:48:43] we could have our own randomizer if we really wanted that, it's not rocket science [04:50:25] it's just a bit hackish, the fiunchinho randomizer uses setAccessible() to modify a protected member of TestSuite [10:18:13] 420 core tests use the database when needsDB() is false [10:18:50] tgr, I found your 2017 change about this when gerrit flagged it as conflicting with my recent work [10:19:13] I'm wondering now if we even need to keep @group Database [10:23:13] Currently, it's only useful to make tests that don't use the DB initialize more quickly when running them in isolation. [10:23:34] That could better be done by having a LBFactory that sets up the fake tables on demand [10:24:08] ...$this->db would have to go away and be replaced by $this->getDbConnection(). we could use __get to provide B/C. [10:24:20] I'm thinking of having only teardown using a detected version of $this->tablesUsed [10:24:30] Another potential use case would be to declare that a test *doesn't* use the database, and if it does, fail [10:24:41] setup will be per once per script invocation [10:25:19] that wasts a few seconds when running a non-db test in isolation, but yea. that could also be fixed by using a different base class. [10:25:27] MediaWikiDBTestCase [10:25:40] we have @group Database already [10:26:01] yea, but you were suggesting to get rid of that [10:28:52] for developer convenience, not just to replace it with a different syntax for the same thing [10:30:06] huh, forgot about that patch completely [10:30:38] one interesting comment in https://gerrit.wikimedia.org/r/c/mediawiki/core/+/377041 was that some (non-core) tests apparently intentionally use the live database [10:30:56] DanielK_WMDE: cloneTableStructure() takes 0.09 seconds [10:31:05] this is premature optimisation [10:32:27] we can just get rid of it and focus on optimising things that are actually slow, like that article insertion that happens any time anything touches the page table [10:33:18] right, i was just going to say that: the setup that is currently done for tests that use the DB is slot. Probably mostly due to UTPage. [10:33:23] *slow [10:33:46] tgr: that's... horrible? [10:34:10] it is [10:44:48] tgr: re calling ArticleRevisionViewCustom in DifferenceEngine when isContentOverwritten is true... it seems to me that renderNewRevision() doesn't work at all with forced content. [10:44:59] line 862 has $parserOutput = $this->getParserOutput( $wikiPage, $this->mNewRev ); [10:45:27] ...which in turn uses $parserOutput = $page->getParserOutput( $parserOptions, $rev->getId() ); [10:47:21] I suppose the solution would be to construct a fake revision, and allow WikiPage::getParserOutput to take a RevisionRecord as well as an int id. [11:13:02] TimStarling: how do we move forward with WikiExporter? https://gerrit.wikimedia.org/r/c/mediawiki/core/+/443608#message-390c259e0c4c2b9c3a2d8c6b326efca2d2f40ecc [11:19:11] if you leave it as is then we'll have to revert it, maybe within a month, the task I filed was not large [11:19:24] maybe revert it within a week [11:20:28] I guess the problem is that BackupDumper::dump() uses WikiExporter::STREAM unconditionally [11:20:47] you could change that to be configurable and set it to WikiExporter::BUFFER in the test [11:21:08] I conisdered that, but didn't want to leave streaming mode untested. [11:21:34] But it doesn't seem so horrible, thinking about it now. [11:21:40] so you're saying that it's not tested now, and it will be removed in a week or a month, but you wouldn't want to leave it untested for that period [11:22:04] it is tested now [11:22:24] just in a broken way? [11:22:47] it doesn't fail, because with the old db shema, no other queries happen while it's streaming [11:23:04] well, queries happen to externalstore, but that'S a separate connection, so no problem [11:25:24] the newMainLB() call has been there since 2009 [11:25:40] but MCR is the first thing to break when that is bypassed? [11:25:49] apparently [11:26:19] what is the status of MCR deployment [11:26:29] because it runs additional queries to get the slot/content meta-data, which is no longer in the revision table, and thus not being streamed [11:26:47] sounds slow [11:26:57] DB migration is done, write-both/read-new is enabled on beta [11:27:03] it should be done in batched queries [11:27:25] can't the slot table be joined to the main query? [11:27:26] next steps are: make write-both/read-new the default in CI and for new installes [11:27:40] then enabled it on test, then on mw.o, then commons, then everywhere [11:28:09] you could join, but you may have more than one slot per revision [11:28:32] because of this, RevisionStore::newRevisionFromRow dows not expect slot or content fields, since they don't really make sense per revision [11:29:01] it would be possibel to work around all that, and have RevisionStore::newRevisionFromSlotRows or something, but that would be more work [11:29:32] if we commit to fixing the streaming in the way you suggested, then it would be fine to have streaming mode untested for now [11:29:39] you should plan to benchmark it after deployment [11:29:43] i'm just worried that streaming will stay, and thus stay untested [11:30:06] benchmark the dump process? probably... [11:30:35] benchmark a stub dump on a large wiki, that will test the relevant code path won't it? [11:30:57] ...against a live DB, with read-new enabled, yes. [11:31:05] once the relevant code path is merged [11:31:21] which brings us back to my patch :) [11:31:24] if it's 2x slower then that will be a reason to do it another way [11:31:57] the reason I asked about deployment schedule was to figure out how soon you actually need dumps [11:32:00] true. But I'm pretty sure the slow bit is reading from ES [11:32:19] when will the first secondary slot be inserted [11:32:20] for now, slot rows are not read during stub generation [11:32:37] that will only become necessary when we have a multi-slot dump format, as per ariel's rfc [11:32:42] I straced dumpTextPass.php today [11:32:51] it was only doing a DB query once every 5 seconds or so [11:33:21] mostly it was just waiting for compression and decompression [11:33:31] that would not be a good MCR benchmark [11:33:36] stub dumps should not hit ES [11:33:43] and they should do more DB queries [11:34:01] yes, but for now, stub dumps don't hit the slots and content tables either [11:34:18] as per the current patch, they continue to use rev_text_id [11:34:34] that will be changed in a later patch. and *that* will have to be benchmarked [11:35:20] oh, I just realized something. [11:35:40] if what i said is true, the tests will not fail in for streaming mode for stub dumps. [11:35:45] only for full dumps in streaming more [11:35:58] so we can test streaming for stubs, and buffered for full [11:36:06] that sounds workable [11:36:14] will look into that after lunch [11:36:38] so the current concern is to make dumps work with the new schema, it is not actual MCR [11:36:49] yes [11:37:01] without the new XML schmea, they cannot be actual MCR [11:37:38] "Make BackupDumper MCR compatible (main slot only)" [11:38:52] afk for lunch [11:39:04] ok, you can merge it as it is I guess [11:39:16] I'll give it a +1 [12:33:33] TimStarling: I'm simplifiying it by using buffering mode for full dumps, and testing streaming only in stub mode [12:33:42] I hope i can make that work [13:13:53] DanielK_WMDE: or just skip that hook for content overrides, we can improve the code later [13:14:19] (re: ArticleRevisionViewCustom in DifferenceEngine) [13:20:51] tgr: i did https://gerrit.wikimedia.org/r/c/mediawiki/core/+/452708/21/includes/diff/DifferenceEngine.php#824 [13:21:16] the entire method doesn't work with content overrides [13:21:20] never did [13:22:09] tgr: do you think the view change can go in today? [13:23:34] I'll look at it in a sec [13:29:04] DanielK_WMDE: forgot to publish some draft comments, did that now [13:52:19] tgr: re. WMDE office is having connectivity issues [13:52:25] i followed up on your comments [14:04:34] DanielK_WMDE: there are a couple "done" comments but no new revision [14:06:14] oops [14:06:19] fixed [14:15:47] tgr: mCacheTime in the ParserOutput is "", so the current tiem is used. [14:15:59] it may be total coincidence that we are hitting this now. [14:16:17] afterall, this will onyly fail if the second flips just between these lines of the test case [14:17:27] oh, right, ParserOutput only gets a timestamp on save, not generation [14:18:04] in that case, yeah, my best guess is that this was just bad luck [14:18:17] tgr: yea. should be done in DPDU::prepareContent, or DPDU::getCanonicalParserOutput, or perhaps best in RR::getRevisionParserOutput [14:18:39] it was GOOD luck :) [14:19:06] do you think we should set the cahce time upon creation, as early as possible? [14:19:23] or when/where? [14:19:29] and should the fix be in your patch? [14:19:42] that seems more intuitive but I'm not really familiar with the parser cache [14:20:38] it will be set again when writing to the parsercache [14:20:42] or do you mean, store it outside the ParserOutput until it gets saved? [14:21:05] no, store it inside the ParserOutput, and set it again when it gets saved in the cache [14:22:14] seems more natural to me, but the curent assumption is that ParserOutput only has a timestamp set when it's saved; could changing that break something? [14:22:19] actually ParserOutput (or rather, CacheTime) could just store the first time it returns, for consistency [14:22:27] anyway it should be a separate patch [14:22:38] I'll make the test more robust in this one [14:22:50] i wonder how to make it more robust [14:23:00] sleep(2) ? [14:23:32] this is why it would be realyl nice to have a time service :) [14:23:37] perhaps i should make one [14:23:41] would be trivial enough [14:47:56] tgr: not sure if i love it or hate it: https://github.com/php-mock/php-mock-phpunit [14:53:32] tgr: ok, i'll give myself a couple of hours to work on that, sounds like a fun project :) [16:12:41] tgr: https://gerrit.wikimedia.org/r/c/mediawiki/core/+/457935 [16:13:26] uh, wait. oh damn, ConvertibleTimestamp is in a library! [16:18:01] I have never dealt with releases of libaries for mw core yet. i just recall it being a hassle with wikibase. [16:23:38] push a tag, update vendor, update composer.json in core [16:23:47] it's extra work but not particularly hard [16:24:39] tgr: I see no version tag in the composer.json of the library. [16:24:49] is is best practice not to have that, and rely on the tag name? [16:24:54] yes [16:25:01] hm, feels brittle, but ok [16:25:11] it should be a plain git tag like 'v1.2.3' [16:25:28] yea [16:25:36] composer.org has an explanation page somewhere about to problems with hardcoded versions [16:25:37] 2.0.0 -> 2.1.0 in this case. [16:26:42] i'm unclear when exactly i should update vendor... make the patch now, and make it Depend-On the core patch? [16:27:46] no, you said it the other way around. [16:28:11] but then the core patch cannot be tested before the new version is published [16:31:55] tgr: can i make the tag before the patch is merged? probably not, right? [16:32:17] the library patch, you mean? [16:32:37] yea [16:32:40] technically possible, don't know if anything blows up [16:32:58] well, it would point to a commit that may never exist on master [16:33:47] I'm not sure our test infrastructure even uses composer [16:34:03] it uses vendor, i'm pretty usre [16:34:06] if it uses the vendor repo, you can just use Depends-On [16:34:17] hmpf, this is turning into quite a project [16:35:29] but i have to exercise this anyway, i guess [16:55:31] composer says: The requested package wikimedia/timestamp could not be found in any version, there may be a typo in the package name. [16:55:32] what? [16:55:57] hm well, i guess it's not available in the specified version [16:56:02] ...and the error is just misleading [17:06:01] tgr: you said you'll only be working part time this week. what's your availability? I'd love to get the view patch and getSecondaryDataUpdates in [17:06:47] also, are you going to work on ContentHandler? This week? I'm tempted to pick it up afterall, since it'S more urgent than SlotRoleHandler [17:10:16] I'm assuming there's no chance at all of WMF prod moving from HHVM to PHP7 before 1.32 ships? So much for deadlines. ;-) [17:10:40] DanielK_WMDE: in theory my off days were yesterday, today and Friday [17:10:55] in practice, probably tomorrow and Friday? [17:12:12] I think getSecondaryDataUpdates is ready, the view patch looks good except it needs a bugfix for the new page issue [17:13:07] James_F: probably not, but I think we have been under the assumption that HHVM's subset of PHP7 is a good enough target [17:13:17] I suppose it does not make sense for me to start working on ConentHandler when I have no chance of finishing it and will be off for the next week [17:13:56] bd808: Right. Just going through my backlog. Though we've got a few things where HHVM fails and PHP5 and PHP7 pass, which are irritating. [17:19:50] tgr: oh, i'll look at the page creating thing! interaction with editpage is...fun! [17:20:36] James_F: want to look at a fun thing? https://gerrit.wikimedia.org/r/c/mediawiki/core/+/457935 https://gerrit.wikimedia.org/r/c/mediawiki/libs/Timestamp/+/457948 [17:22:00] DanielK_WMDE: Neat! [17:24:02] (Do you want me to review and merge?) [17:25:07] James_F: always! [17:25:54] once the change goes into the library, i'll have to do the vendor dance... [17:26:06] I can help with that. [17:26:51] DanielK_WMDE: fyi there's a timestamp mock thing in WANObjectCache already [17:26:54] the patch has only been up a few minutes. so it may be a bit rash to merge. but a review and perhaps a +1 would be appreciated [17:28:02] legoktm: ah, right - i have done similar things in some places in wikibase. [17:28:21] but I want to apply this to *all* tests, especially one who *don't* explicitly think about it [17:28:56] James_F: ah, right -- in it goes :D [17:28:58] There's definitely a test somewhere about ~~~~ that breaks when you test it too close to midnight UTC, unless that's now been fixed? [17:28:58] thanks! [17:29:03] Thank you. ;-) [17:29:33] heh, nice one! my patch should indeed fix that [17:31:39] ok, i pused v2.1.0 [17:31:54] packagist should pick this up in a few minutes, i guess [17:32:03] or does it need manual poking? [17:33:14] it takes a few minutes [17:33:15] It's automatic. [17:33:39] DanielK_WMDE: also, can you GPG sign your tags in the future? [17:34:46] legoktm: i can try, i have never done that... i'm not even sure how to push a tag from the command line to gerrit. what exactly to push to, i mean [17:35:04] there's no way to sign in the web ui, assume [17:35:08] no [17:35:13] $ git tag -s v2.1.0; git push --tags [17:35:15] that's it [17:35:27] DanielK_WMDE: `git tag -s v1.2.3 -m 'Signed v1.2.3 release`. [17:35:28] oh right, straight to master. ok [17:35:56] I can make a signed v2.1.1 if you like [17:35:56] Needs GPG set up with git locally. [17:36:12] You could also pull and re-publish the tag, but that might break things. [17:36:18] i guess i'll have to do that at some point anyway [17:36:31] uh yea, not sure packagist likes that [17:43:53] DanielK_WMDE: Did you want me to make the new pull-through for vendor and core? [17:48:31] trying to run composer update on vendor locally now [17:49:04] "the requested PHP extension gd is missing from your system." [17:49:06] *sigh* [17:49:56] that library is waiting on the Zero stuff to be killed :/ [17:51:52] hm, composer update results in changes to composer/LICENSE, composer/ClassLoader.php, and the timestamp format in composer/installed.json [17:51:55] what gives? [17:52:12] means you're using an older version of composer [17:52:37] I would recommend downloading the latest phar from https://getcomposer.org/download/ and using that to generate vendor [18:01:50] DanielK_WMDE: check the readme, it has notes about all those problems [18:03:19] bleh ;) [18:05:19] tgr: hm... i'm not finding anything about the timestamp issue [18:05:42] the new composer version helps (thanks legoktm), but the timestamps are strange [18:06:02] not the timestamp specifically but the installed.json format changed in 1.6.4 [18:06:24] if you use that or newer, it shouldn't change much [18:06:37] using 1.7.2 [18:07:07] also, did you use composer update ? [18:07:17] that should not change the license [18:07:54] no. but actually, the timestamp format thing is gone. i just looked again. [18:08:08] maybe i used the wrong composer again by accident (i now have two on my system) [18:08:16] so all looks good, except one thing: [18:08:31] "mediawiki/mediawiki-codesniffer": "22.0.0" [18:08:35] used to be 21.0.0 [18:08:40] why and how did that get updated? [18:09:13] if you use unconstrained composer update, it will jut upgrade whatever it can [18:10:05] yea, but i thought we had everything pinend to a specific version in composer.json [18:10:57] dev dependencies, I'm not sure [18:11:00] ok, now there's a small change in the CoC file, which i supposed got merged since the previous release [18:11:10] ...and a whitespace change to composer/ClassLoader.php [18:11:15] which seems odd, but whatever [18:11:38] ClassLoader gets regenerated when the list of classes changes [18:12:05] I guess the exact format depends on composer version? [18:12:15] I wouldn't worry about it [18:15:37] ok, vendor patch is up, and core patch now depends on it. [18:15:48] let's see what jenkins thinks of that [18:49:53] tgr: Argh. [18:50:32] tgr: You just merged to /vendor without a /core patch. Won't that break everyone's code until the corresponding core patch is merged? [18:51:11] (Yay for /vendor's circular dependencies.) [18:51:50] does it? [18:52:04] It can, yeah [18:52:07] Unless something's special about that library, yes. [18:52:12] OMG CONSTRAINTS DON'T MATCH [18:52:13] I thought vendor and core's library handling were independent [18:52:25] DanielK_WMDE's patch to add to composer.json should be a distinct patch (immediately merged). [18:52:32] They're… complicated. [18:52:51] One sec, I'll do it. [18:56:22] probably should be documented somewhere if that's the case [18:56:33] It is. [18:56:55] does something run composer update with the vendor directory already present? [18:57:02] Yes. CI. [18:58:15] https://www.mediawiki.org/wiki/Manual:External_libraries and the readme in the vendor repo are the places where I would look [19:00:56] tgr: https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/457971 [19:01:39] James_F, Reedy: sorry, went down a rabbit hole. only to find that I had to climb out again [19:01:47] :) [19:03:00] Essentially, libraries that core uses are special and trigger the constraints-don't-match error. If you merge a /vendor patch changing them, you must also merge a corresponding change for /core at the same time. This is because we actively disabled the constraints-don't-match error for /vendor CI to allow us to ever cut the circular Gordian knot (otherwise we could never merge those patches in either repo). Is it a hack? You betcha. [19:03:35] Maybe /vendor should have a IF YOU HAVE TO ASK YOU SHOULDN'T TOUCH THIS REPO note at the top of the README. [19:04:50] adding this to the README would already help :) [19:05:12] and I used to know this, once upon a time... [19:09:57] Fundamentally we can only fix this by (a) using no libraries via composer, just manually copied into MW core git, or (b) running real composer in prod. Neither are appealing options. [19:11:09] I wonder if we can make CI more smart about merging vendor patches [19:14:01] RoanKattouw: This is for you :D https://gerrit.wikimedia.org/r/c/mediawiki/core/+/457970 [19:14:20] Tested locally and works, some patches regarding API is coming too [19:39:46] DanielK_WMDE, Reedy, tgr: https://gerrit.wikimedia.org/r/#/c/mediawiki/vendor/+/457976 note. [19:41:08] thanks [19:43:13] thanks james! [19:48:18] Amir1: When you have time, I finished DI-ifying https://gerrit.wikimedia.org/r/c/mediawiki/extensions/ORES/+/433956/ and getting CI to pass [19:49:08] RoanKattouw: Sure thing, I'm about to finish my last coding patch on change_tags for now (adding some tests here and there) [19:49:15] Yeah no rush [19:49:16] after that, will get to it [20:01:29] I've been having fun going through https://gerrit.wikimedia.org/r/q/file:RELEASE-NOTES-1.32+is:open+-is:mergeable+-hashtag:merge-in-mw-1.33 [20:01:52] "Fun" is in the eye of the beholder, of course. [22:54:06] Krinkle: so is https://github.com/wikimedia/translatewiki/blob/e975a1cc41413ea6df036ca11f65b53fbc9c5cd9/TranslatewikiSettings.php#L410-L418 all that's needed to have the cool URL for the main page? no varnish/apache changes? [23:03:39] my second question is whether there's a standardized way to do sampling for client-side EventLogging [23:05:01] legoktm: Yes, those lines of config + MediaWiki:mainpage being set [23:05:04] that should do it afaik [23:05:48] legoktm: Yes, as well. Depend on eventlogging light (ext.el.subscriber) and use inSample() [23:06:11] ... to conditionally call mw.track('event...', ...); [23:06:34] ty