[09:17:30] hi [10:09:29] morningz [17:30:17] sorry, spaced on the meeting. hangout is full. [18:24:46] Dr0ptp4kt TSG meeting on? [19:02:51] dbrant: bgerstle QA chat w/TSG [19:16:20] dr0ptp4kt, JonKatz or TWIMC: in https://www.mediawiki.org/wiki/Wikimedia_Product#Reading , the phabricator link ("#reading") is still broken... [20:17:55] nzr: yt? [20:18:21] nzr: what do you say about this comment from adam? "Also, is it desired to have the prefix of "zh-" when in typeahead mode but not have it shown by default when not in typeahead mode? Understood, the section label indicates "N variants for ", was wondering if there's any harm in just having the prefixes for those variants for their rectangular labels anyway"? [20:29:28] bmansurov: you there still? [20:29:35] wanted to see if you need help with staging [20:29:35] yes [20:29:41] did we get languages working there? [20:29:44] no, I think sam got that covered [20:29:48] yeah [20:29:51] SWEEET [20:29:55] http://reading-web-staging.wmflabs.org/wiki/Selenium_language_test_page?useformat=mobile#/languages [20:30:01] so what can i do to help you get languages wrapped up? [20:30:41] review [20:30:46] i'm pushing a patch in a sec [20:34:40] jdlrobson: https://gerrit.wikimedia.org/r/270047 [20:51:48] nzr: a thought... currently any language that has been clicked once is a preferred language [20:51:57] i now have 6 preferred languages from testing :) [20:52:47] jdlrobson: this feature is not for testers :) [20:53:17] nzr: i guess i'm just saying should we invalidate these at any point or raise the treshold [20:53:38] if i click one language once (e.g. I meet an Israeli in hackathon and switch to hebrew wikipedia) is it really a preferred language? [20:53:49] just something to think about [20:54:04] bmansurov: my local wiki is not setup with interwiki links [20:54:10] anything i can do to fix that quickly? [20:54:32] jdlrobson: yeah, let me give you some sql [20:59:40] jdlrobson: i've emailed you [21:04:35] bmansurov: i've got a few rows in there already i don't want to mess with but thanks. Will adjust the sql [21:04:55] cool [21:09:01] mmm not showing up in api call [21:09:17] ah reboot fixed it [22:23:58] jdlrobson: how do I stop stubbing a function in a qunit test? [22:24:12] I can't seem to find the documentation on that method [22:24:16] jhobs: can you show me the test? [22:24:23] stubs are cleared at end of the test in teardown [22:24:37] so don't have QUnit.test( and then stub a bunch of things [22:24:41] you only stub once [22:24:52] yeah I know, i'm stubbing in the setup [22:25:00] but I need to write tests for the function i'm stubbing [22:25:24] point me to code lemme see what you are up to [22:26:19] jdlrobson: https://gerrit.wikimedia.org/r/#/c/267058/8/tests/qunit/suites/resources/mediawiki/mediawiki.viewport.test.js line 26 [22:26:36] That's where I stub `makeViewportFromWindow` [22:26:55] but now I need to write a test for that function, so I want to clear the stub for just that instance [22:27:04] stub it inside the test that uses it rather than inside the module [22:27:11] several tests use it [22:27:20] so just repeat it and screw DRY? [22:27:36] you have only 3 QUnit.tests .. [22:28:08] yeah I wasn't being quippy, I was genuinely asking [22:28:15] no problem repeating it since it's only a few? [22:28:30] i mean if you had 15 tests using it [22:28:41] you could do 2 modules ( QUnit.module) inside the test script [22:28:49] ah [22:28:52] and group them that way - but here not sure if it's worth it [22:29:07] I figured I shouldn't have multiple test modules for one module, but good to know for the future [22:29:14] yeah for now I'll just stub it in the tests [22:29:21] So it seems you are testing default behaviour of isElementInViewport [22:29:30] yes [22:29:37] that's what the stub is for, anyways [22:29:41] but that is taken care of by isElementOnScreen [22:29:58] yes, I specifically repeated myself there [22:30:07] because the better thing would be to test it in isElementInViewport [22:30:08] is that not the only test that needs it? [22:30:16] but then it would seem that there are no tests for isElementOnScreen [22:30:42] so taking a leaf out of dduvall's book, I felt repeating here for clarity was worth it [22:30:47] i guess the real question is why do you need to stub more than once [22:31:08] just those two (I forgot I wasn't testing the default case in the third one) [22:31:11] well isElementInViewport test should pass an explicit viewport in [22:31:19] the isElementOnScreen should use the default value [22:31:46] hmm ok. I still feel like then it feels like the isElementOnScreen test isn't testing all parts of the function [22:31:52] but i'll defer to your judgment here [22:32:03] why not? isElementOnScreen calls isElementInViewport [22:32:08] and invokes that path [22:32:27] right, I'm thinking encapsulation-wise [22:32:35] rather than as an entire suite [22:32:46] just different mindsets is all really [22:32:53] Glancing at your qunit tests though I'm not seeing clearly why you'd need to ever stub it with a different value [22:33:55] Again, more or less for clarity. Otherwise I'll have to look at the window properties which may change between runs of the tests [22:34:05] For tests I try to keep as much the same as possible usually [22:34:49] i'm still confused. You're stubbing already which is great but why would makeViewportFromWindow need to be stubbed more than once with different values? [22:34:54] why not just use two elements? [22:35:31] It doesn't? I think now I'm confused [22:35:42] It's stubbed with the same value [22:36:23] ^ jhobs [22:36:33] I think maybe just uploading my next PS will help clear confusion [22:36:49] you said "how do I stop stubbing a function in a qunit test?" [22:36:55] i'm asking why do you need to stop stubbing [22:37:37] you never want to test the default behaviour of $( window ) as you don't have any control over what it is - it varies where-ever run [22:37:39] jdlrobson: in order to write a test for the function I was stubbing [22:38:02] What value would that give? [22:38:25] I left it out initially but bmansurov -1'd in CR asking for one :/ [22:38:48] well you'd essentially be testing a jQuery function no? [22:38:53] *functions [22:39:00] yeah, that was my thinking and why I left it out initially [22:39:04] not to mention it's a private function [22:39:06] As you'd have to stub $( window ) [22:39:42] Some tests are not worth it. I would suggest justifying your stance there and I agree with you on this. [22:39:54] alright [22:39:56] jQuery has tests, we don't need to test jQuery [22:40:10] but bmansurov_break is right about the repeated line :) [22:41:08] jdlrobson: see that's where our views differ I think. If you were to look at each of those tests individually, if either one didn't have that line then I would consider the test of that function incomplete [22:41:16] but as a complete suite, the line is repeated, yes [22:41:24] but shouldn't our method tests test every part of that method? [22:41:40] if anything, the better option might be to just remove the test for isElementOnScreen altogether since it's just an alias [22:44:23] why? It has the same test coverage no? [22:44:39] You have the line assert.ok( mw.viewport.isElementOnScreen( el ), twice [22:44:47] they test exactly the same thing [22:45:06] Yeah, I understand. Want to hop in tracy island for a minute? I don't seem to be getting my point across very well over IRC [22:45:44] You have a line assert.ok( mw.viewport.isElementOnScreen( el ) inside a test called QUnit.test( 'isElementInViewport [22:45:56] the test is not testing that function [22:46:20] Oh maybe I didn't understand lol [22:46:20] if it was assert.ok( mw.viewport.iisElementInViewport( el ) that makes more sense [22:46:33] Yeah I didn't notice that I was calling the wrong method [22:46:41] whoops, I'll fix that [22:46:51] I also think killing isElementOnScreen wouldn't be a bad idea [22:46:59] either/or is good to me [22:46:59] I'm not too keen on aliases in JavaScript [22:47:09] oh you mean the function, not the test? [22:47:27] yeah I mean that's fine. I threw it in mainly for extension later [22:47:46] to test things like an element's visibility and what not as well if we find a need for it [22:47:54] but I suppose we can just write all that when it comes up [22:48:34] jhobs: am sending you some review [22:50:09] jdlrobson, jhobs: i'll briefly butt in, you don't have to stub window or anything. You'll have to calculate window dimensions and the check agains what the function returns [22:50:59] bmansurov_break: why it does no computations [22:51:20] bmansurov_break jdlrobson it'd just be testing jquery functions and the "+" operator [22:51:23] not worth imo [22:51:33] jdlrobson: don't we have to check what it's returning is correct? [22:51:47] alright, it was just a suggestion [22:51:49] :) [22:52:09] bmansurov_break: the way i'd look at it is - am i going to change this function? [22:52:33] the way I look at it is: is the first implementation correct? [22:53:17] bmansurov_break: if it weren't, it wouldn't work properly at all, meaning it wouldn't get merged [22:53:19] Tests are only good if they catch regressions imo [22:53:47] agree, they're also good to catch edge cases that we humans don't catch well [22:53:59] while writing tests jeff found some edge cases yesterday [22:59:57] niedzielski: hey, weren't you having an issue where EventLogging events were showing up a WHOLE BUNCH of times? [23:00:12] niedzielski: i'm having that. (was it 6P-specific? trying a different device.) [23:00:58] mdholloway: i was. i think it's a beta event logging service issue only (T125423) [23:01:25] mdholloway: i don't think it's on our side but if you find otherwise, please leave a comment on the ticket :) [23:01:39] niedzielski: will do, thx for info! [23:01:49] np, thx :) [23:15:25] hey do you guys have now on tap working? [23:15:38] mdholloway: or niedzielski ^ [23:16:15] kaity_: to be honest it's been some time since i've even thought about now on tap [23:16:21] kaity_: i don't really know a lot about it. i use it all the time by accident (just long press home on any screen) [23:16:51] google has about a billion snapshots of my launcher screen [23:20:24] hm I think its not enabled for business accounts [23:22:48] kaity_: i just tried on both devices at arm's length and the activation dialog fired right up. i'm guessing you're right that it's not active on yours. [23:23:08] yes trying to enable but it wont let me [23:24:51] kaity_: why not just add a personal account? [23:25:13] niedzielski: yep trying that! [23:28:07] niedzielski: got it working. thanks! [23:28:25] 👍