[18:26:51] Sigh. Why does WatchedItemQueryServiceUnitTest have to mock Database::makeList() with an inadequate replacement? [18:41:56] anomie: because it's a unit test :) How else would you write a proper unit test for something that access the database?... [18:42:57] DanielK_WMDE: That method doesn't access the database. It turns an array such as [ 'foo = bar', 'baz' => [ 1, 2, 3 ] ] into a string such as '(foo = bar) AND (baz IN (1, 2, 3))'. Except the crappy replacement doesn't do that right. [18:44:13] anomie: going by the book, that doesn't matter. a proper unit test will *completely* isolate the class under test, providing mocks for all other classes that may be called. [18:44:34] but tbh, I'm myself not convinced of the value of this type of unit test. It's something that addshore has been experimenting with. I'm sure constructive criticism is appreciated. [18:45:19] My thinking about unit tests for DAOs is: to test the contract, we'd have to emulate actual database behavior. [18:45:49] the contract doesn't say what query is used. it's about the behavior of the methods, and their relationship to each other. there is no way to properly test this without sql behind it. [18:46:13] but, well - the point of experiments is colelcting experience, right?... [18:46:29] The crappy replacement raises "Array to string conversion" warnings... [18:47:11] and throws away the 'baz' key. [18:47:22] well, mocks are supposed to be crappy replacements. they are supposed to be minimal, and not expected to conform to the contract of the interface they implement. they'll fail for anything unexpected [18:48:12] But that means when someone else has to change the implementation to make use of one of the features you left out, that person has to fix your crappy too-minimal implementation. [18:48:59] Oh, this wasn't even written by addshore. This one is by Leszek. He's not in the office today. will be back tomorrow. [18:50:00] anomie: ideally, this turns out to be a feature: changing the implementation forces you to change the test. that *can* be the right thing. Though I maintain that tests are about contracts, and should be oblivious to implementation. [18:50:12] that never works 100% with mocks, though [18:51:18] Apropos contract. What'S the contract of Revision::getParentId() if there is no parent? Return null, or 0? Is there a guarantee? Does it behave consistently? [18:51:45] DanielK_WMDE: My complaint is that *I* have to stop working on updating the test for my changes in order to fix the crappy replacement so it functions closer to the actual contract. [18:53:28] DanielK_WMDE: It looks like Revision::getParentId() is supposed to return null in that case, since the DB field is nullable. [18:53:57] checking usage of getParentId() is inconclusive [18:54:13] yea, saw that it'S nullable... but do we arite null, or 0? let me check the live db... [18:55:17] we write 0, not null [18:55:23] i guess it's nullable for old rows [19:07:43] anomie: I understand the complaint. When I first saw this kind of test, I had the feeling that mocking the DB will do more harm than good. But it seemed like a worth-while experiment. Perhaps tell Leszek and maybe addshore about the problems it cause for you. [19:10:19] Is gerrit being slow and not-really-working for everyone or just for me? [19:11:02] anomie: seems to work ok here [19:11:55] anomie: btw, i plan to work on improving unit tests over the next couple of days. and probably won't do much on MCR tomorrow at all. So if you want to look at the code, this would be a good time :) [19:12:29] I plan to push another small update soonish today, though. [19:14:43] DanielK_WMDE: >> "Daniel Kinzler addshore: do you think you'll have tiome next week for helping me write unit tests for REvisionRecord and RevisionStore?" << Yes, today has been crazy though! [19:15:45] Also re WatchedItemQueryServiceUnitTest, I will also take a look at that tomorrow and see why it is so evil. [19:15:46] but right now, bed [19:15:55] oh hey addshore! no worries. i pinged you because anomie was complaining about unit tests with mocked DB objects. we are starting to see fallout from that experiment :) [19:16:07] that one isn't yours, though [19:16:19] so don't invest time if you have other commitments [19:16:25] easy solution, delete them all ;P as far as I remember they all have integration tests too [19:16:38] hehe :D [19:16:50] anomie: did you hear that?... [19:16:58] addshore: when will you be in berlin? [19:17:01] (I wont have to double check) [19:17:06] Meh, I figured out how to hack up the makeList to work for the things I need it to work for. [19:17:42] 4:15pm (berlin time) friday until late monday evening DanielK_WMDE [19:17:45] But if someone is going to delete them all, I'll leave updating that file for last. ;) [19:18:02] anomie: I'll take a look tomorrow! [19:18:11] I don't remember exactly what they all do [19:19:26] addshore: ah, ok! wikidatacon will be a crazy ride. let's make sure we reserve an hour for planning work on mcr. i'll definitly need help, or find a cloning vat. [19:19:49] DanielK_WMDE: yup! will you be in the office on the Monday? [19:19:55] addshore: besides writing unit tests, another task will be to actually phase out Revision in core code, and replace it with REvisionStore and RevisionRecord. [19:20:53] addshore: i wasn't planning on being at the office on monday... hardly onyone will - tuesday is a bank holiday, so people either have a long weekend, or they are recovering from the conference... [19:21:22] aaaaaaah! In which case, we must find the hour either late on friday or at some point during the conf! [19:21:23] but then, i don't have any real plans for monday. [19:21:58] friday is probably goign to be completely crazy for me, preparing for the conference hand talking to wmf folks about sdc [19:22:22] well, we'll see. if we don't manage while you are here, we'll just have to do a hangout [19:22:36] oh, i just realized. [19:22:52] we have the DST switch RIGHT IN THE MIDDLE OF THE CONFERENCE with people from other continents [19:22:59] that will be a load of fun! [19:26:13] DanielK_WMDE: Timing. :-) [19:28:50] well, at least we are turning the clocks back, so everyone will be an hour early, not later. [20:35:55] hmm... so I am looking at how to display wikidata search results properly... And I see that ShowSearchHitTitle does not allow to modify link attributes (no i18n rendering possible) and ShowSearchHit does not pass position (so no way to make proper link myself there). That's annoying. [20:37:13] anybody here who knows how fulltext SERP works can advise? I don't want to start messing whole search page when I just need to display a couple of strings... [20:43:09] ah well looks like if we could make ShowSearchHitTitle also have $extraAttributes param that is passed to link renderer, that will solve the problem. [20:50:25] SMalyshev: Those hooks are a mess, as you're aware :( [20:50:38] Search is a hodgepodge of hooks and subclassing. [20:50:38] :( [20:55:10] yup... and despite having like 15 or soo hooks, all slightly different, still no way to do what I want as far as I can see, without changing them [20:57:25] anomie: Would like feedback from you - https://phabricator.wikimedia.org/T24509 [23:52:00] MaxSem: wanna re-review https://gerrit.wikimedia.org/r/#/c/385322/ ?