[00:04:28] bd808: documneted at https://phabricator.wikimedia.org/T190111 [07:58:08] <_joe_> ftr, ori's patch broke apache server-status for ipv6 [07:58:28] <_joe_> it was our patch to unbreak that that had the consequence of breaking calls to localhost for any other site [15:23:05] Hmm, the code coverage tool seems to be pretty buggy. On localhost it leaves out entire functions in some files for no reason I can discern. [15:23:20] The version is very outdated, for one thing . . . [15:50:28] God knows how well it works with PHP >= 7 [15:51:34] This is completely broken, it doesn't register any of the lines at all: https://doc.wikimedia.org/cover/mediawiki-core/includes/api/ApiQueryDeletedrevs.php.html [15:53:07] may be worth checking if it works any better with PHPUnit 6 [15:53:28] I wanted to try that out, PHPUnit 6 should be runnable with a small compatibility alias for setExpectedException [16:50:54] AryehGregor: Coverage is not measured for code that is @deprecated [16:51:02] That is intentional and a documented behaviour of PHPUnit. [16:51:28] Its marked as 100% but note that the details say 0/0 lines, not 523 [16:51:38] It does the same for interfaces and empty files. [16:53:42] _joe_: Hm.. link to the second patch? [16:53:56] _joe_: I suspect moving the Require local, from to might work. [16:54:20] <_joe_> Krinkle: I think that's what we should do too [16:54:26] Hm.. actually, I don't know [16:54:32] It's only a problem when used as loopback [16:54:40] So I don't think it will make a difference [16:54:42] <_joe_> but I need to dedicate some time to squint hard at our apache config [16:55:09] We basically want Apache to just use the Host header for deciding the VirtualHost [16:55:16] and then honor 'Require' within that [16:55:36] Instead of always mapping a loopback connection to regardless of Host header [16:55:43] <_joe_> well we want to honor 'Require' for /server-status [16:55:51] Yeaj [16:55:52] YEah [16:56:10] But it would also be fine to restrict the whole virutalhost under that, given it contains nothing else anyway [16:56:23] <_joe_> what do you mean? [16:56:33] _joe_: moving it up one level won't break the restriction [16:56:40] But also won't solve the problem [16:57:49] <_joe_> Krinkle: we could just move that virtualhost to the bottom of our stack [16:58:01] <_joe_> I have to think a bit about it and re-read some docs [16:58:39] Hm. might work yeah. The before-last probably, before nonexistent.conf [16:58:56] Although if memory serves, Apache does it the other way around (last match is used). non-existent.conf loads first [16:59:16] So as second one in that case between nonexistent and everything else [16:59:27] Tricky thoguh, because one comes from sites-enables and the other config-enabled [16:59:30] but I suppose we can fix that too [16:59:34] <_joe_> no, for virtualhosts first match is used [16:59:55] <_joe_> the first virtualhost that matches gets used [17:00:41] 00-nonexistent.conf, 02-redirects, 03-main etc. [17:00:50] first one is wildcard [17:01:02] so unless specificity is used somehow, last one matches. [17:01:10] Assuming it loads sites-enabled in a-z order [17:01:20] <_joe_> it does [17:01:28] <_joe_> and yes, specificity is indeed used [17:01:37] Ah okay :) [17:01:52] Then registering the fallback first makes sense, to provide recovery if later ones break stuff I guess. [17:01:54] Cool [17:02:15] <_joe_> Krinkle: https://httpd.apache.org/docs/2.4/vhosts/details.html [17:03:06] <_joe_> "The address can be specified as *, which will match a request if no other vhost has the explicit address on which the request was received. [17:03:09] <_joe_> " [17:04:27] "When the connection is first received on some address and port, the server looks for all the VirtualHost definitions that have the same IP address and port." [17:04:53] That suggests moving it back won't work, IP:port always wins? [17:05:04] But maybe if we rename it to localhost:80 instead of 127.0.0.1:80 [17:05:27] but I guess that might be part of the protection we wanted to add in addition to 'require local' [17:05:49] Not sure if that was it, or whether the original didn't even use 'require local' [17:06:17] I'd imagine Require local to do the correct protection by default. That's pretty widely used on the web with a strong assumption it can't be circumvented. [17:06:46] <_joe_> Krinkle: I'll have to look with some brainpower, I'm currently writing python unit tests [17:06:52] kk :) [17:07:13] <_joe_> it's a task that really makes me miserable [17:07:14] <_joe_> :P [17:17:51] I forget, what needs to be done so Jenkins will run 'test' rather than 'check' for AryehGregor's patches (e.g. https://gerrit.wikimedia.org/r/#/c/420714/)? I know I can comment 'recheck', but that's not scalable. ;) [17:18:30] Oh wow, Aryeh is back?! [17:18:32] :D [17:19:15] Contracting for MWPT for some relatively short period of time. [17:20:02] I see I see [17:20:21] But to answer your question.... [17:22:13] integration/config.git [17:22:24] zuul/layout.yaml [17:22:43] Look for &email_whitelist [17:26:34] Krinkle: Oh, that explains it. But I would think we'd want tests for deprecated functionality too, as long as it remains in the software. We presumably don't want to break tools that rely on it until we actually remove it. [17:33:27] Particularly if it's going to sit around in the codebase for years and not actually get removed for who knows how long. [17:59:45] AryehGregor! Long time no see! :D [18:08:16] AryehGregor: Yeah, that's fine, the tests will run and pass/fail the build. But they're not counted towards coverage goal. [18:09:03] no_justification: anomie: All that's needed is "recheck" it will vary is behaviour based on who says it :) [18:09:14] "Main test build" vs "Basic test build", looks like it worked [18:09:25] Yes. The point was adding Aryeh to trusted users :) [18:09:30] So he gets better tests [18:09:31] Oh sorry. Right [18:09:38] By default :D [18:09:45] :D [18:10:42] Woo, contractor! [18:10:53] Welcome indeed, AryehGregor :) [18:25:39] Krinkle: As I said, "I know I can comment 'recheck', but that's not scalable. ;)" [18:26:45] anomie: Yeah, I missed that :| [18:27:53] Krinkle: Ah, I see. Yes, I suppose that makes sense. [18:28:28] (I spent the last several years in web standards land, where there's no concept of "obsolete" because nobody will fix their pages so all old features have to be supported forever, generally speaking.) [18:45:31] AryehGregor: Well, we still have quite a few settings like that in MW ;-) [18:45:42] legacy encoding :p [18:47:05] we still have back-compat for schema changes from 10 years ago because we never ran maintenance scripts to update stuff in WMF production :) [18:48:32] (did you know that logging.log_user_text was only introduced in 2009? i learned about it recently when stuff broke because old rows have this field empty) [18:49:24] Yes I knew. I remember when we added it haha [18:51:31] Krinkle, anomie, AryehGregor: https://gerrit.wikimedia.org/r/#/c/420815/ [18:51:38] I don't have time to deploy it right now, train duties [18:51:40] But there ya go [18:52:25] no_justification: What does that actually do? [18:52:33] Whitelists you for the "better" tests [18:52:37] What "recheck" does [18:52:43] But by default [18:52:43] :) [18:53:50] no_justification: I guess you didn't see https://gerrit.wikimedia.org/r/#/c/420777/ ? [18:54:17] Nope [18:54:18] Heh [18:56:44] How do I publish a reply to a comment in Gerrit? [18:57:32] AryehGregor: click "Reply…" and "Post" on the main changeset's page [19:00:40] Keyboard shortcut: 'r' [19:00:58] Oh, reply to a /comment/ [19:01:01] MatmaRex: I see, that's very discoverable. [19:01:08] There's a little arrow on them I thought that quotes them? [19:01:21] AryehGregor: :) [19:01:42] Like this: https://usercontent.irccloud-cdn.com/file/Tw47G7Pp/gerrit_reply_arrow.png [19:02:19] oh yeah, I spent ~15m looking for the reply button today [19:02:56] AryehGregor: if you try the "New UI" (link in bottom right of the page), it's a bit better, the workflow is still a bit awkward, but there's a tooltip that explains it, heh [19:03:20] AryehGregor: actually, if you haven't used Gerrit before, you should probably enable the new UI and try to forget the old one ;) [19:04:40] Okay, will do. [19:04:44] Thanks for the tip. [19:05:01] AryehGregor: done now - https://wikitech.wikimedia.org/w/index.php?title=Release_Engineering/SAL&diff=1786063&oldid=1786048 [19:05:14] The "New UI" is a little rough in 2.14.x still, but upon upgrade to 2.15 (timeline tbd) it's much more polished [19:05:50] Can anyone confirm in Chrome canary build - https://bugs.chromium.org/p/chromium/issues/detail?id=823824 just so I know it's not my machine only? [19:06:16] Oh wow, someone literally just triaged it with git bisect and all [19:06:29] That rarely happens within a month [19:09:32] There's a new UI? [19:11:59] PolyGerrit [19:12:22] Hmm. Not horrible with a maximized browser window, although the metadata seems a bit squished on the left. Pretty bad with my usual 1024-px-wide non-maximized browser window, but the side-by-side diffs were already bad there in the old UI. [19:13:08] I wonder if they fixed the annoying bug where the diff view randomly jumps around and loses focus in the comment textarea, then starts interpreting typing as random keyboard shortcuts? [19:14:49] It actually works on mobile :) [19:14:52] anomie: Yes! [19:14:58] They replaced the highlighter/editor [19:15:14] Ok, just for that fix I'm not going to go back to the old UI. [19:16:00] anomie: There's a couple of rough edges (mostly groups & repo listing pages not implemented, forces back to old UI), but mostly it works :) [19:35:21] Only two PHP 7.1 unit test failures left (see Travis) [19:36:27] no_justification except the admin screen is not on mobile. /me keeps pushing for it to be :) [19:36:40] Krinkle: MW core? [19:36:51] Yeah [19:37:01] Not bad [19:37:07] The XMP fix worked [19:37:15] Landed yesterday [19:37:28] Postgres is failing rather weirdly though [19:37:35] Not sure what that is about yet [19:38:15] Why do the main entry points in Database (select, update, . . .) have no function-level documentation whatsoever? [19:38:24] Like, documenting the format of the parameters, say? [19:38:43] Cause they're in IDatabase? [19:38:51] That's a relatively good reason. [19:41:56] AryehGregor: aye good point. There’s (now dormant) a discussion about whether to enforce use of inheritDocs to visually hint this to non-IDE editors and viewers [19:42:20] Of course, DatabaseMssql has it's own... [19:42:47] Doxygen (doc.wikimedia.org), IDEA, PHPStorm etc inherit by default [19:43:01] Krinkle: It's useful in an IDE itself too... If you're looking at the Database.php and there's no comments [19:43:49] Reedy: hmm but I imagine in the iDE it’s show the docs of hovering the class method? [19:43:59] It’d* [19:44:24] PHPStorm complains there's no comment [19:44:24] https://imgur.com/a/EEdmK [19:44:27] But that might just be my config [19:44:48] How should I mark an expected fail in phpunit? [19:45:00] I.e., I wrote a test for a bug and am checking it in before the actual bug is fixed. [19:45:21] Ideally, it should be flagged if the test suddenly starts to pass unexpectedly, in case the bug was fixed by mistake. [19:45:31] (This is very useful behavior in my experience.) [19:45:58] Most frequently, we mark "broken tests" as skipped... But this doesn't help your use case [19:46:13] @group Broken in the PHPDoc? [19:47:54] How does said test fail? [19:48:04] Just returns an incorrect value. [19:48:44] Mozilla tests can be flagged as expected to fail, and then they get run, and if they *pass* it breaks the build (forcing you to update the annotation and bring them into service). [19:49:13] It's very useful behavior. It wasn't at all uncommon for me to commit something and send it for a test run only to find that I fixed some expected failures somewhere. [19:49:26] If we don't support it, though, I'll just add it to the broken group. [19:50:32] One easy way to hack it would be to just expect the incorrect result. [19:50:43] I guess that makes the most sense, although it's not very aesthetically pleasing. [19:51:14] * AryehGregor remembers the old days when he ranked #1 or #2 on the number of lines spoken in #mediawiki [19:51:19] AryehGregor: oh you mean assert that it does and should throw an exception? There’s expectExcepyion in PHpunit [19:51:39] Certainly, that would work.. And the you fix the test when you fix the bug... [19:51:48] I'll do that. [19:52:36] If it’s an incomplete test I test for future behavior then we just let it fail behind group Broken or leave unmerged in Gerrit [19:56:33] AryehGregor: You can also expect exceptions, of course. [19:58:07] I think markTestSkipped or markTestIncomplete in PHPUnit should cover also this [21:06:28] AryehGregor: You could do like https://stackoverflow.com/a/5181681 I suppose: expect the exception PHPUnit throws on failure. OTOH, writing a test that tests the specific wrong behavior will prevent someone from accidentally making it differently broken. [22:23:40] or [23:32:51] * AaronSchulz is suprised to see AryehGregor here :) [23:44:47] AaronSchulz: You mis-spelt "delighted". ;-) [23:54:35] James_F: the weather must be dampening my ethusiam [23:57:02] * James_F grins.