[05:55:02] morning! [05:59:23] joakino: did you see that Parashuram asked for more info about the browser-perf issue today? [06:55:21] Morning! [06:55:46] phedenskog: just saw it, I'll reply after breakfast [06:56:14] I want to try to adb pull something other manually and see if it works [07:00:58] cool [08:10:12] yo [08:19:10] moarning [09:07:40] how're you joakino? [09:10:41] phuedx: 👍 [09:21:11] phuedx: fyi, i'm planning to signoff on the ab test stuff soon [09:21:21] not sure if there's anything else to change in prod regarding that [09:21:28] s/prod/staging [09:29:31] joakino: join tracy and i'll show you what i'll change [09:34:05] phuedx: joining [13:13:26] joakino, phuedx hey guys, can you review https://gerrit.wikimedia.org/r/#/c/290444/ ? thanks [13:37:06] bmansurov: which browser(s) is the test failing in? [13:37:19] phuedx: i tested on firefox [14:58:46] dbrant bearND /cc mdholloway: o/ any objections to publishing the beta? [14:59:01] niedzielski: o/ nope! [14:59:03] none [14:59:25] niedzielski: could i have a few more minutes to look over the notes [14:59:43] dbrant: sure! [15:19:18] bearND dbrant mdholloway: hm, i think that network state permission must be implied. it's still showing up as a required permission [15:21:51] phuedx: i'm sorry, i tested on chrome [15:22:08] phuedx: don't know why i said firefox, probably because that's my fav browser [15:22:59] bearND dbrant mdholloway: actually i think it's mapbox. it looks like it added an extra permission too, ACCESS_WIFI_STATE [15:33:21] niedzielski: mdholloway: bearND: so, the Mapbox SDK instructs us to require the ACCESS_NETWORK_STATE permission. https://www.mapbox.com/android-sdk/#app_permissions [15:33:44] bearND dbrant mdholloway: i put in a patch here to make Wi-Fi optional https://github.com/mapbox/mapbox-gl-native/pull/5119. i don't think this should hold us up [15:34:16] dbrant: those docs appear to be outdated but good find [15:37:06] niedzielski: makes sense [15:37:16] bearND dbrant mdholloway: do you folks mind if i enable this new "prelaunch report" feature? https://support.google.com/googleplay/android-developer/answer/7002270 [15:37:47] niedzielski: i don't, looks useful [15:40:06] niedzielski: mdholloway: bearND: it's fine if Mapbox *actually* doesn't need that permission; but, again, if we're going to need to add it back in the next release... :( [15:41:49] bearND dbrant mdholloway: i think i might have miscommunicated. the permission is required. i can see it in the console. it was just surprising because we had removed it and you confirmed it came from mapbox [15:43:59] bearND dbrant mdholloway: does that make sense? i can hop in the batcave if i'm not coming across clearly [15:44:25] oh! since when can a library inject permissions into the app? [15:45:12] dbrant: it's that manifest merger thingamajig. there's something similar for proguard rules too [15:46:56] niedzielski: yes, I've seen it for ProGuard rules. With Marshmallow it makes sense that a lib can request permissions, too [15:48:22] ha! well then, i suppose it's all good [15:49:24] niedzielski: bearND: dbrant: no change needed on our part, then? [15:50:44] bearND dbrant mdholloway: right. the permission is inherited from the lib [15:51:12] bearND dbrant mdholloway: any objections to 1) publishing, 2) enabling prelaunch report? [15:51:22] niedzielski: 1) no and 2) no [15:51:43] engage! [15:51:59] Make it so [15:56:07] bearND dbrant mdholloway: 1) published (yay!). due to the 64b libs, we now have four apks. 2) i guess this was already enabled in prod. there's even screenshot tests! check out the pre-launch report tab in the console [15:56:46] niedzielski: oh, neat, i'll check it out! [16:03:41] joakino: spretro [16:26:36] bearND dbrant mdholloway: nice, new test report is ready and looks pretty cool. i guess i just have to upload to alpha first. i'll update the release process notes for next time. https://play.google.com/apps/publish/?dev_acc=02812522755211381933#CloudTestLabResultsPlace:p=org.wikipedia&v=146 [16:27:41] bearND dbrant mdholloway: i recommend signing up for the pre-launch emails btw [16:28:52] niedzielski: there are a lot of options. Which ones do you recommend? [16:29:55] niedzielski: I guess all pre-launch reports for now [16:31:00] bearND: at least the pre-launch report. i also turned on performance, reviews, and news, tips, and opportunities althoug hi have received any emails for these yet [17:02:24] reets nzr /cc dbrant|brb: o/ hello! i know your time on android is limited right now so no expectations from me but where can find the most recent feed mocks and wireframes? [17:03:31] niedzielski: this is the one I know of: https://app.zeplin.io/project.html#pid=56b3ef430f64a4eb0f1b9d27&sid=571529f2c7312f087aab1b4d [17:06:05] dbrant: hm, can you add my account to that? i'm "stevn" on there [17:07:35] dbrant: thanks! :) [17:08:02] niedzielski: ok cool, wasn't sure you got it [17:08:56] i'm in :) /cc mdholloway bearND in case they don't have zeplin accounts yet [17:24:00] hi niedzielski - there's also images in the dropbox (which I've sent you a link to) though they'll essentially be the same as Zeplin files. [17:24:25] reets: thanks! [17:29:54] niedzielski: no worries, but just bear in mind they are WIP items before Kaity left, not finals :) [17:31:25] reets: yep yep :) [18:05:03] bmansurov: hey [18:05:07] yo [18:05:14] so RL protects you against not loading code unnecessarily [18:05:28] ? [18:06:04] SO if a Resource module is defined it doesn't mean it is loaded in the page [18:06:30] If there is value in testing ext.popups.schemaPopups then arguably there is value in making it a reusable unit [18:06:57] jdlrobson: sure, i'm aware of that [18:07:25] jdlrobson: say event logging is not installed, should we still load the schema code on the front end? [18:07:37] So the thing that is confusing is that the RL module ext.popups.schemaPopups has different dependencies depending on the context it is run in [18:07:51] jdlrobson: why is it confusing? [18:07:57] well how is it a true unit? [18:08:08] If I run tests with EventLogging installed they will work differently then without it. [18:08:16] we unconditionally register ext.popups.schemaPopups [18:08:34] but it's dependencies are different depending on what you are testing [18:08:35] jdlrobson: yes, let's forget tests for now [18:08:45] jdlrobson: let's talk about the actual behavior [18:08:49] the tests highlight this problem [18:09:04] jdlrobson: hmm, i disagree [18:09:11] jdlrobson: how would you load the file then? [18:09:46] i haven't got to that bit yet. I'm arguing that a unit is not truly being tested if it's behaviour can change with different setups [18:10:24] jdlrobson: that's why tests are failing. I need to load EL unconditinally in the testing envirionment [18:10:30] that way the behavior is always the same [18:10:34] no that's bad [18:10:38] why? [18:10:49] the module is being registered unconditionally so should be tested unconditionally [18:10:58] jdlrobson: that's a mock module [18:11:04] The module shouldn't require EventLogging to work [18:11:12] otherwise you shouldn't unconditionally add it [18:11:14] it should, it's all about event logging [18:11:21] that's the only thing it does [18:11:31] let me collect my thoughts.. i don't seem to be getting my point across very well. [18:12:07] i kind of understand where you're coming from, but your solution creates a real problem for the user [18:12:18] which is it loads the module unconditionally for everyone [18:12:25] even for those who don't have EL installed [18:13:46] Popups does not /require/ EventLogging. Agreed? [18:14:43] yes [18:14:52] Thus if I install Popups but not EventLogging then my unit tests should work. Agreed? [18:14:54] but event logging does [18:15:07] jdlrobson: yes [18:17:16] If a module has dependencies that means the dependencies are required to work? [18:17:45] sure [18:17:51] but given ext.popups.schemaPopups works without the EventLogging dependencies, then those dependencies are not required to work [18:18:12] I agree, but in that case there is nothing to test [18:18:12] so i think what this comes down to is that there should be an initialise event logging script module [18:18:21] why is there nothing to test? [18:18:33] because that module will load more files depending on whether EL is installed [18:18:44] if EL is not installed, then the module is just an empty module [18:20:29] bmansurov: it's still registered though but sure. [18:20:50] i guess my uncomfortableness is that tests should not behave differently depending on what you have installed [18:20:56] yes, we need to register it because the code wants to log some events if EL is available [18:21:07] If I tell you test X is failing and you cant see that because you dont have EL installed that's confusing [18:21:34] jdlrobson: ok, but we need to install EL as we need to install other dependencies if we want to run all tests [18:21:44] the methods you are testing don't seem EventLogging specific. Although we are using them purely for EventLogging they are not tied to EventLogign [18:22:03] jdlrobson: some of them yes, but I'm also testing the log function [18:22:20] resources/ext.popups.schemaPopups.js seems to mix initialisation and reusable library code [18:22:45] Why would you test the log function? Don't you trust the tests for the log methods in EventLogging? [18:23:03] jdlrobson: i want to make sure the correct data is being sent to EL [18:23:18] which method do you test? [18:23:19] jdlrobson: OK, if you're really unconfortable with it, I can find another way [18:23:43] mainly three methods that are exposed for testing [18:23:46] i'm just seeing tests for getSamplingRate and getEditCountBucket [18:24:05] (in ext.popups.schemaPopups.test.js) [18:24:50] ext.popups.renderer.article.test.js has some more tests [18:24:51] It feels to me like the crux of the problem is mixing initialisation code with reusable code [18:25:10] jdlrobson: reusable code is only used in that file [18:25:10] ext.popups.renderer.article.test.js should not be testing stuff that's in ext.popups.schemaPopups.js [18:25:21] no need to expose every utility function for every module [18:25:31] and it should be able to test those things without EventLoggign installed [18:25:53] jdlrobson: i feel like we are going back to the beginning of the argument [18:26:38] jdlrobson: nothing to worry, i'll restructure the code for your liking [18:26:48] lets regroup after lunch, but essentially my message is that well organised code shouldnt need to depend on another extension for its unit tests to work unless it's specifically using some code from the EventLogging extension [18:26:49] jdlrobson: can you review and test other parts of the patch? [18:27:06] jdlrobson: it IS using some code from EL. [18:27:07] I'd prefer you understood where I was coming from and agree with me other than do things just to please me. [18:27:17] i do understand you [18:28:18] bmansurov: yes it calls schemaPopups = new mw.eventLog.Schema(= which is a side effect of loading ext.popups.schemaPopups.js that i may not want [18:28:45] ive manually tested the code bmansurov and it works perfectly [18:28:49] it's not the side effect [18:28:54] my only issue is around this [18:28:54] it's the intended effect [18:29:04] but let's talk after lunch [18:29:06] maybe tracy [18:31:38] dbrant: would you be able to rep us at scrum of scrums tomorrow? [18:31:59] dr0ptp4kt: yep, sure [18:32:14] dbrant: thx [22:46:10] only 1 crash reported in today's beta so far [22:52:13] niedzielski: nice