[21:00:02] #startmeeting RFC meeting [21:00:02] Meeting started Wed Mar 20 21:00:02 2019 UTC and is due to finish in 60 minutes. The chair is KateChapman. Information about MeetBot at http://wiki.debian.org/MeetBot. [21:00:02] Useful Commands: #action #agreed #help #info #idea #link #topic #startvote. [21:00:02] The meeting name has been set to 'rfc_meeting' [21:00:08] #topic RFC: Add a frontend build step to skins/extensions to our deploy process https://phabricator.wikimedia.org/T199004 [21:00:43] who is here to discuss this ^^^^ RFC? [21:00:52] \o [21:01:05] o/ [21:01:06] present. [21:01:40] * bawolff didn't know about this rfc but sounds interestiong, so o/ [21:02:18] o/ [21:02:29] I am, too. [21:02:51] Hello [21:03:02] i do not at present have strong opinions on the topic, but i'm here on behalf of RelEng's CI future working group since it seems like it may have bearing on our requirements: https://www.mediawiki.org/wiki/Wikimedia_Release_Engineering_Team/CI_Futures_WG [21:03:19] ResourceLoader by design supports direct resource inclusion (https://www.mediawiki.org/wiki/ResourceLoader/Requirements) but does not support inclusion of derived resources. I would like to frame this RFC discussion around the following question: "What needs to be true for frontend assets shipped to end users to not live in the repo?" [21:04:02] o/ [21:04:39] milimetric: that sounds similar to the framing you were suggesting, right? [21:04:49] is it possible to separate the question of "npm install" from "npm run build" etc.? [21:05:27] the task description seems to imply that the build step will be fetching packages from npmjs.org, but it's not really clear to me whether that's correct [21:05:37] It might also be helpful to focus on a simple example : Imagine a repo with 5 SVGs minified (for performance reasons). The developers want to keep the SVGs un-minified to allow easy modification and readability. However, they want to ship these to end users minified. They have an npm script for minifying SVGs. How do they do that? [21:06:00] I think there are a few distinctions to unpack there [21:06:08] Right now the following options are available to them: 1) Just commit just the unminified assets and give up on the readability [21:06:14] minifying is just running a local command, not fetching potentially unreviewed packages from the internet [21:06:19] 2) Commit the minified assets themselves alongside the unminified 3) Write some PHP code to do minification. [21:06:37] Minified SVGs are a derived resource, from a real resource in the repo, not from the internet as Tim said [21:07:03] Also SVG minification is a lot more doable in PHP and in real-time than transformations like Babel or Webpack [21:07:28] this was also a theme of legoktm's comments on this task, he had concerns about the remote aspect [21:08:06] ResourceLoader does do things like JS minification, CSS remapping and image embedding, but the constraints they all satisfy are 1) implemented in PHP; 2) fast enough for on-demand execution w/ caching; 3) using only files within the repo, nothing from the outside internet [21:08:28] Okay, so to talk about the specific problem we're using - Popups builds JavaScript files (substitute with minified SVG files) using local JavaScript files (substitute with SVG) using a tool webpack pulled down from npm (a minifier) [21:08:36] A lot of things that we'd like to do violate 1 and/or 2, but I think that's a very different beast than things that violate #3 [21:09:29] jdlrobson: let's maybe address the "download from internet" concern first, in isolation [21:09:29] Right, so that's a non-PHP tool doing things that are probably too slow to do on-demand in a web request, but not necessarily needing to use the internet to do it (except for the webpack tool itself coming from the internet but that kinda doesn't count IMO) [21:09:48] jdlrobson: do you have an example of that and why it might be useful and problems the lack of it cause? [21:10:30] I would also like to see more clarity around what Tim said. Fetching dependencies from random places on the internet is something I'm going to have strong opinions on. Having a deterministic minifier is something I don't really care about comparitively [21:11:19] it seems to me like "doing transformations" and "pulling in dependencies" are fundamentally different things [21:11:34] Yeah [21:11:37] sorry, clarifying question for RoanKattouw, why doesn't fetching webpack from the interwebs count? [21:11:39] I am talking about doing transformations using things installed from the internet, but defined in the repo. [21:11:41] i don't think discussionm about them should be bundled together [21:12:11] niedzielski: The software performing the transformation comes from the internet, but the input to that software doesn't [21:12:13] if all you need is a copy of webpack, that can presumably be mirrored to our own git, that doesn't need to be done every time [21:12:20] And yes this --^ [21:12:20] Presumably we could cache WebPack in CI/whatever or mirror it. [21:12:22] Yeah. [21:12:37] jdlrobson: perhaps this example from the RFC? "In MediaWiki core external libraries are copied and pasted into the resources/lib folder. This is done manually (https://github.com/wikimedia/mediawiki/commit/065b21b4bdd49ffca494ff905de29bfc500ed535). These files do not actively get updated, but if they did, a build step could help them live elsewhere." [21:12:42] ...or at least pin it and verify a signature? [21:12:42] If we're really paranoid we can also freeze Webpack at a known good version and things will keep working, at least for some time [21:13:11] milimetric: And I pointed out in the RfC that we're not doing that any more for the major libraries, and I'm converting the rest over time. [21:13:12] #info I am talking about doing transformations using things installed from the internet, but defined in the repo. [21:13:27] RoanKattouw: TimStarling if we're concerned about what version of webpack we are using of course, we could use a global webpack install that we've okayed [21:13:33] Yeah exactly [21:13:37] OK so setting aside the internet thing [21:13:40] however running webpack would generate some assets that we need to deploy [21:14:07] so for simplicity our webpack/minifier tool might output resources/dist/build.js or resources/dist/arrow-minified.svg [21:14:19] Downloading things from the internet that run in prod still seems like something that we should approach carefully [21:14:20] maybe helpful to differentiate between the level of security we aspire to and the one we currently have [21:15:07] what we aspire to is that all code gets committed in readable form to out repository, goes through review, and only then gets transformed by tooling which went through a similar review [21:15:13] RoanKattouw: I don't think we can table it quite yet, but I think maybe we're close [21:15:16] Right. The workaround we use currently is that resources/dist/* is checked into the repo, which causes problems, but it also means that deployment is trivial [21:15:20] For my particular use cases, if there are global npm installs of the tools I need to use that have been approved I don't need to pull from the Internet. I just need to run them using input defined in a repo. [21:15:45] It sounds like this proposal is for resources/dist/* to not be in the repo, but instead be built during some stage of the deployment process? [21:15:54] what we actually have is all code gets committed in some form (mostly unreviewed, but can be audited later if there is an investigation) [21:15:56] bawolff: so I believe all dependencies would be clear in the repository, but the proposal is to not necessarily download and use/build them locally, but on a build server [21:16:00] RoanKattouw: exactly. the is the crux of what i want to solve [21:16:06] as right now, every file you deploy needs to be in the repo. [21:16:07] bawolff: so at review time, we could audit dependencies [21:17:08] and require pinning dependencies [21:17:09] ***** reminder that this meeting is 45 minutes so we are 1/3 of the way through ***** [21:17:33] Definition of pinning here is the specific version number? Or is it a cryptographic hash? [21:17:42] specific version number yes. [21:17:43] we could make it whatever makes us safe [21:18:05] commit id [21:18:05] you can't effectively pin npm dependencies AUIU [21:18:08] and fail the build if some dependencies are not pinned according to our definition [21:18:42] I think another question is it even necessary to use webpack? [21:18:43] the npm owner just uploads a file and tells npmjs.org "this is the 1.2.3 version", no actual connection to version control exists [21:19:00] No, you can. [21:19:06] This is what package-lock.json is for [21:19:28] I think tgr is saying that someone could switch out what 1.2.3 is for different projects [21:19:29] OK, so it sounds like the main thing people are worried about right now is: if the output files aren't in the repo, then they need to be built for deployment, and doing that on our servers is scary without taking measures like pinning [21:19:35] in which case, yeah, let's define it as a checksum [21:19:44] could there be a separate, vetted, repo of build tools that could be used in core and extensions (via git repo or container or whatever)? [21:19:45] Obviously upstream can unpublish a version, but if you pin versions and use package-lock you don't ever get changes you don't manually approve. [21:19:55] we can install webpack on our servers [21:20:19] Might it be feasible to do builds on our local machines, and instead upload the results to our servers? [21:20:22] (Both in your direct dependencies but also for the whole tree.) [21:20:46] The simplest way to do that would be to check in resources/dist/ into the wmf* branches. Although that does make cherry-picks more annoying [21:20:48] RoanKattouw: You mean like how most of the services have a development repo and a /deploy repo? Eh. Maybe, but it's icky. [21:20:53] (which is the tangential question of, "if this necessary" rather than "is this feasible") [21:21:00] *is this [21:21:01] Oh yes services have a model for this too, true [21:21:08] A bad model. [21:21:08] Arguably the /deploy repo for MW is the wmf* branches [21:21:16] James_F: AIUI upstream can just silently replace a version with a different file, and npm install will dutifully update it, even if it is locked. And the developer who ran npm install might or might not notice the unexpected changes in vendor. [21:21:18] Which we're replacing with docker. [21:21:34] tgr: No? npm will freakout with fatals if the hashes don't match any more. [21:21:58] tgr: Obviously if you don't look you don't find issues, but we could build that in to this frankendeploymentbuild step. [21:22:11] At the very least, i'd like there to be an audit trail. So if anything bad happens, we know precisely what code was deployed, and what code the build process ran [21:23:07] ok, now it seems like we've handled the "scary internet code" problem, without being specific about which tools we're using except npm and package-lock [21:23:15] if anyone disagrees, speak now [21:23:25] otherwise, what's next jdlrobson [21:23:26] I don't know why we are still talking about npm install when apparently all we need is a potentially very old copy of webpack [21:23:31] we have build steps all over the place and each of them is bespoke and time consuming to setup. In a dream environment I would define certain scripts to run on deploy inside an `npm run deploy` command. Right now every single Jenkins run and precommit hook checks whether all SVG assets in a repo are minified. That would be something I'd love to put in an `npm run deploy` job [21:24:19] OOUI handles tons of SVG assets, and it isn't really that hard for us to ensure they are minified before being committed [21:24:34] Most repos only have a handful of SVGs that change very rarely [21:24:55] scap could run "npm run deploy" if it takes only a minute or so [21:25:03] So if I am provided with the tool available with whatever version I am allowed to use, how do I make sure it runs where it needs to [21:25:04] it's not very scalable but it does do l10n stuff already [21:25:25] i'm very far from the most knowledgeable person on this, but i have a suspicion we would really rather not do that. [21:25:26] it would possibly need to run on 1) deploy 2) browser tests/unit tests 3) docker/vagrant 4) prior to releasing code [21:25:44] brennen: what would we rather do? [21:25:56] I think the restrictions we'd have to put in place to allow scap to run "npm run deploy" directly on the deployment servers would be pretty frustrating [21:25:56] ("that" being run npm by way of scap.) [21:26:15] We're talking about Webpack, but what if someone wants to run Babel, or do SVG minification, or a number of other things [21:26:32] RoanKattouw: could webpack coordinate that? [21:26:36] (it does for us) [21:26:47] can someone spell out for me what "proper CI pipeline" means? [21:26:57] Sure, Webpack can do a bunch of things, but my point is someone's going to want a package not named webpack at some point [21:27:00] since that seems to be the non-scap solution in the task description [21:27:14] I think the more general the better. The way we setup `npm test` to allow us to run arbitrary tests in CI has been liberating and I'd expect saved RelEng a lot of bespoke requests from the teams it serves. [21:27:24] I would prefer not running "npm run deploy" on the deployment servers, but instead in a place where it's less scary to run random stuff [21:27:47] We could call them "developers' machines". [21:27:49] RoanKattouw: what critical things is it doing that we can't do in ResourceLoader? [21:28:05] James_F: hehe [21:28:11] edsanders: I really don't think this particular conversation is helpful right now but I am happy to have it offline. [21:28:20] RoanKattouw: what is scary with "npm run deploy"? [21:28:24] WebPack has a bunch of fancy features that RL doesn't, like babel. But we're not planning to add that? [21:28:53] I'm trying to bridge the gap between edsanders / James_F and jdlrobson and I'm thinking we could define base configs for webpack and build tooling in a separate repo, then include that from extensions and use them to run npm run build. Or take the project template approach like the react cli / vue cli projects do (happy to elaborate) [21:28:54] Nikerabbit: You either have a very restricted set of packages that it can use, or it's running code from all over the internet [21:28:58] I disagree, the RfC is about having a build step, I think a discussion about how to do it "best" is premature without justifying why to do it. [21:29:02] running outside a container on my laptop as tstarling is pretty much the same as root access to every server [21:29:12] doing minification on dev machines has its own scary security implications, unless it's deterministic enough that CI can warn if it's not the same output byte by byte [21:29:18] I'd rather run things on deploy1001 [21:29:41] TimStarling: That's reasonable. Krinkle has local containers for just that reason. [21:29:46] are you just going to reject patches containing minified files from all devs who are not fully trusted? [21:29:51] jdlrobson: it seems that we have started with the assumption that this is something we want to do, but I assumed that a discussion about it was going to be part of this process? [21:29:52] I agree though that if we're generating assets that are not easily verified by humans, we'd want a reproducible build step [21:30:04] I have local containers, nobody is reviewing how my setup works though [21:30:30] "very restricted set of packages" is a probably good thing for production (compare with mediawiki-vendor) [21:30:58] Nikerabbit: Sure, except I'm talking about packages that generate code that is run in production, not packages that run in production directly [21:31:06] TimStarling: that's a terrifying and great point... developer machines are no less dangerous than scap deploy servers, perhaps much more dangerous in your case [21:31:16] **** 15 minutes left ***** [21:31:24] I guess if the build process is containerized, that might make it less scary to run on deploy1001? [21:31:38] RoanKattouw: that's how the service workflow works [21:31:42] docker builds [21:32:05] And eventually the MW workflow will be that. [21:32:11] (Spoilers.) [21:33:08] the reason I am asking what "proper CI pipeline" means is because we are apparently already using CI for this, but it has a bunch of problems which led to this RFC [21:33:23] I can speak to what's going on in our repo. [21:33:30] I think the tooling choices needed will vary based on team. The tooling my team is working with is working great for us (webpack) however I could complete understand people wanting to use babel, rollup or other libraries to build their code. [21:33:54] does it mean having a /deploy repo, like services, which CI will commit to? [21:33:56] The only thing that sucks about our setup if having to commit the built assets into the repo so they get deployed (since we deploy master branch). We have Jenkins to verify the committed assets match what are expected on every single change to gerrit [21:34:02] jdlrobson: Not that great. I can no longer submit patches in your repos because your toolchain rejects the versions of local dev environments I have. :-( [21:34:11] If the code is not trusted, I find it scary no matter how much the code is sandboxed [21:34:11] The resulting artifacts are not going to be sandboxed after all [21:34:49] TimStarling: i think it would essentially mean that, for example, minified resources would be built into an image delivered to production. [21:35:14] with the minification happening as part of the pipeline. [21:35:15] delivered how? [21:35:23] TimStarling: a deploy repo would help, but the downside of this approach is that 1) someone needs to remember to do this 2) Special:JavaScript/test and browser test CI pipeline would run potentially a weeks worth of changes in on go [21:35:41] and possibly reject it (and we'd need to spend time working out how to remedy that) [21:36:01] jdlrobson: why would someone need to remember to deploy? It seems to me that at least would be obvious when deploying :) [21:36:04] Yeah, deploy repos are an anti-pattern, but making this something that can derail the train in each of 200 deployed repos also isn't great. [21:36:10] I'd be less concerned about having a deploy repo if deploys were 1) automated and if 2) CI always ran against the latest code (after being built) [21:36:41] milimetric: we tried feature branches in this fashion, making "dev" the default and doing weekly builds to master [21:36:47] but we found that confused a lot of people [21:36:54] jdlrobson: interesting, how [21:36:54] who didn't often work in our repo [21:37:12] Someone would manually merge the code in dev branch to master and then it would ride the train [21:37:17] we also found problems running the CI alongside it [21:37:22] If we do have build steps, we need the feedback to the developer to be very quick – a week late is way too late, it should block merge. [21:37:38] ^ that [21:37:49] jdlrobson: that seems like a people problem more than a tooling problem, should be pretty easy to explain in a README not to do that [21:37:57] so we just have a couple minutes left, please add anything important to be added to the minutes using #info [21:38:14] millimetric: a compilation step is a pretty standard process for many non-mediawiki softwares, even as an automated part of CI and deployment [21:38:14] jdlrobson any last minute remarks or questions? [21:38:15] milimetric: No. You can't randomly differ prod repos like that, it's really disruptive. [21:38:59] #info If we do have build steps, we need the feedback to the developer to be very quick – a week late is way too late, it should block merge. [21:40:10] it's kind of hard to approve when there's not really a concrete proposal [21:40:21] James_F: makes sense, but if a policy like this gets adopted, it should be standard IMO [21:40:46] @timstarling @KateChapman I would appreciate some direction on how to fix the problem I have with running webpack and deploying the result. I'd love to know what the constraints would be to understand what modifications my codebases would need to be compatible with that [21:41:05] I know very little about our deploy pipeline so I don't really know what's feasible [21:41:09] milimetric: I seriously doubt 99% of repos will migrate to have a build step with concrete wins, none of which we've talked about. [21:41:11] so idea.. for deploy. what if we create some sort of standard infrastructure, where a dev can basically npm submit vendor as a seperate patch.... [21:41:13] I think the simplest thing is to run it from scap [21:41:41] that way, you keep the flexibility in the repo, but make it easier to keep 'checked' stuff at the wmf level [21:42:03] In the Magical Future™ with CD and containerised MW deploys, we'll have "build" steps where the pipeline makes the docker image. [21:42:09] But that's a long way off. [21:42:10] where vendor, i mean /vendor /node_modules /dist, whatever... [21:42:26] it shouldn't be just wmf... it should be easy for anyone to reproduce a stable mw release, for example [21:42:42] yeah ok. whatever, we will be the primary consumers [21:42:45] thedj: node_modules usually has 3rd party stuff and /dist is usually a build of everything, which one [21:42:48] Nikerabbit: Totally. [21:43:32] +1 to James & Tim I haven't seen any discussion here of concrete benefits or if there are viable alternatives [21:43:49] if there's a reproducible build step we can handle it for tarballs the same way we handle composer, that seems like the least problematic aspect [21:43:53] the second simplest thing is maybe to handle it like composer, where we have a git submodule in production for the output of the build process, but devs mostly just run the build directly [21:44:14] *** just 2 minutes left please tag things to the minutes with #info *** [21:44:38] I agree we should aim for something like mediawiki/vendor, except maybe per-repo and wit ha less manual process [21:44:42] TimStarling: exactly, thats the direction i was thinking in, but that provide tooling to make management of that easier for devs. [21:44:45] TimStarling: One for each extension with a build step? [21:44:49] FWIW, i will raise this discussion to various folks discussing the Magical Future™, although honestly i think any direction we go with CI will almost by default support the desired build step features and it's more a question of concrete decisions about what we want to do with that. [21:44:50] #info jdlrobson looking for advice, not putting forward a proposal for approval at this time [21:45:07] **** okay this meeting is ending I'm going to then restart the bot for the next one **** [21:45:14] #endmeeting [21:45:14] Meeting ended Wed Mar 20 21:45:14 2019 UTC. Information about MeetBot at http://wiki.debian.org/MeetBot . (v 0.1.4) [21:45:14] Minutes: https://tools.wmflabs.org/meetbot/wikimedia-office/2019/wikimedia-office.2019-03-20-21.00.html [21:45:14] Minutes (text): https://tools.wmflabs.org/meetbot/wikimedia-office/2019/wikimedia-office.2019-03-20-21.00.txt [21:45:14] Minutes (wiki): https://tools.wmflabs.org/meetbot/wikimedia-office/2019/wikimedia-office.2019-03-20-21.00.wiki [21:45:15] Log: https://tools.wmflabs.org/meetbot/wikimedia-office/2019/wikimedia-office.2019-03-20-21.00.log.html [21:45:25] #startmeeting RFC meeting [21:45:25] Meeting started Wed Mar 20 21:45:25 2019 UTC and is due to finish in 60 minutes. The chair is KateChapman. Information about MeetBot at http://wiki.debian.org/MeetBot. [21:45:25] Useful Commands: #action #agreed #help #info #idea #link #topic #startvote. [21:45:25] The meeting name has been set to 'rfc_meeting' [21:45:38] #topic RFC: Let's stop using QUnit as a mechanism for integration tests https://phabricator.wikimedia.org/T212521 [21:45:54] who is here for this meeting? (sorry for the abrupt ending of the last) [21:46:09] O/ [21:46:26] I am. [21:46:30] o/ [21:46:35] \o [21:46:45] o/ [21:47:01] \o [21:47:22] o/ [21:47:35] \o [21:48:02] jdlrobson: do you have a specific way you want to frame this discussion? Or specific questions you'd like to discuss? [21:48:30] So I tried my best to summarise this yesterday: https://phabricator.wikimedia.org/T212521#5038625 - I think there are a lot of benefits for running true unit tests for an extension in isolation from the rest of the extension and thanks to changes recently made by RoanKattouw these are within reach. [21:48:57] Right now all tests are run via a special page: Special:JavaScript/Test. [21:49:10] *Special:JavaScriptTest/qunit [21:49:17] I have proposed we carve out a second set of QUnit tests that are run via Node.js [21:49:27] I hope to get some clarity on whether this is a sensible direction, what problems there are with this approach (if any) and how we can make this reusable to other extensions (would it make sense to have a single npm module for testing that includes dependencies for Selenium, grunt etc) [21:50:06] jdlrobson: are you talking about devs running them locally, or CI? [21:50:30] both. Running in CI is a solved problem from my perspective [21:50:51] In MobileFrontend we actually run tests in both Special:JavaScript/qunit and via a Node.js test runner [21:51:14] However to support the former, we currently have to build a JS asset for inclusion in ResourceLoader. [21:52:05] Ideally, we would not do that and define a thin separate set of tests for that runs in the browser. [21:52:19] Yeah, we've had a node-based JS test runner in VE and OOUI (and a bunch of places) for years, but it's a pain to remember to maintain it manually when new tests/etc. get added, and we have to add manual local hacks to package.json to specify them into `npm test`. [21:52:30] https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/487168/ demonstrates how a node qunit test runner would work [21:52:40] Naive question, is it not possible to generate code coverage reports on the tests that use Special:Javascript? [21:52:53] kostajh: nope it's not possible but would be with Node.js [21:53:13] kostajh: We'd have to re-engineer nyc/etc. which'd be a lot of work. [21:53:17] Would we lose any functionality by stopping qunit [21:53:34] Zppix: right now, yes [21:53:44] I think Krinkle had an idea about that but, yeah it was complicated [21:53:51] Zppix: we'd still use QUnit in this setup. [21:54:01] Zppix: the difference would be the tests would run outside the MediaWiki environment [21:54:44] jdlrobson: why separate tests? Why not just have two different runners? Special page and some standard tooling and conventions to run them all in node as well, like karma [21:54:44] at least for me, rewriting the tests to work in headless mode was illuminating - it showed how many assumptions are made in our test [21:55:09] milimetric: I think that's what I'm proposing? (two runners) [21:55:10] oh, maybe that answers my question ^ :) [21:55:12] not only in the tests, in all the code :) [21:55:31] Say we go to Node.js will it be there of tech debt? [21:55:43] jdlrobson: oh, I thought from how you phrased it that you meant some tests would only run on the special page and a subset would only run on node [21:56:01] Will there be alot of tech debt if we go to node.js* [21:56:15] Some of the tests won't be suitable on node (because they're semi-integration tests). [21:56:24] milimetric: oh i see what you are asking - you are asking whether the same tests could run in both environments? [21:56:35] James_F: seems solvable with some decorations/tagging of tests maybe? [21:56:43] yeah jdlrobson [21:57:09] My experience says no, since node.js tests would need to use jsdom [21:57:09] milimetric: If we're going to do that epic work (mocking all of MW for each extension's tests), why bother having a second runner just for coverage? [21:57:24] but I guess if we cared we could (although i'm not sure what the benefit would be) [21:57:26] It'd probably be less work to make coverage work with MW's testrunner. [21:57:39] if coverage were the only concern, probably not too hard to solve to get it from Chrome, e.g. puppeteer can do that [21:57:44] James_F: we currently use subdirectories to distinguish runners. qunit/ to mean Special:JavaScriptTest and node-qunit/ to mean headless [21:57:49] VE deliberately keeps all of it's non-MW code out of MW, but I can see the benefit to other extensions that don't have that luxury [21:57:49] Zppix: I think this exercise would reduce tech debt and modernise our code to make better use of ResourceLoader's new packageFiles feature [21:57:55] having properly isolated unit tests would be great though [21:57:58] niedzielski: Yeah. Nice model. [21:58:10] Obviously we'll miss coverage reports from the tests we don't run on node. [21:58:17] But some coverage data is better than none. [21:58:21] One pleasant unexpected outcome was making our internal libraries packagable and versionable https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/487168/9/resources/src/mediawiki.String/package.json [21:59:02] James_F: I didn't mean literally tag each test, what niedzielski fits, basically some way to differentiate [21:59:14] *what niedzielski said fits [21:59:16] Tests would also be discoverable, so no need to list them all in a php file (and expose yourself to human error ) like so: https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/487168/9/tests/qunit/QUnitTestResources.php [21:59:56] I've just started using this approach in the MediaInfo extension (node-qunit headless testing), and it's been very helpful. Isolating components one at a time and making their dependencies explicit in both unit tests and code (through using require statements) has made it much easier to think through the code [21:59:56] Would ext maintainers have to change their code for these tests to work? [22:00:14] Yes, but given we'd be supporting 2 sets of test they could migrate (or not migrate) at their leisure [22:00:30] (that's a reply to Zppix^) [22:01:16] I feel like supporting 2 tests would potientally become more of a hassle why not just do one or the other? [22:01:21] can we make it so that we can run the qunit tests inside the MW env if we want ? [22:01:29] cause then i would be all in ;) [22:01:36] **** 1/3 of the way through the meeting 30 minutes left **** [22:01:37] sorry s/qunit/nodejs [22:01:41] it would be a little harder, but would it be more useful to define the difference as "unit tests" vs. "integration tests" rather than node runner vs. specialpage runner [22:01:45] thedj: I'm sure we could - but it would require some kind of bundler to create a distributable jS file [22:01:52] jdlrobson: I'll second your note about splitting out new libraries. The tests have really shown us just how much we depend on. When developers have to consume their own libraries like `require('foo')`, it encourages more thought about assumptions and dependencies, in my opinion, which eases code reuse. [22:02:24] jdlrobson: node is cool and all, but it will also hide browserbugs.. ;)_ [22:02:28] thedj: you'd just need to add them into a resourceloader module, no? [22:02:42] like it's done with qunit tests now [22:02:44] thedj: Zppix but one benefit/con (depending on how your look at it) of Special:JavaScript/qunit/plain is it runs a set of tests across different extensions [22:02:58] so if your extension has a dependency on VisualEditor it would catch those errors [22:03:01] this is not really my area, but it seems weird to not run unit tests on browsers, only node, it'd be like running phpunit tests only on PHP 7, not HHVM [22:03:14] ^^ [22:03:45] I also am not too confident now that i know browser bugs can be overlooked by node tests [22:03:46] to that point, jdlrobson, why not karma with the chrome runner that way you can run all tests? [22:03:56] You also have the potential for non-DOM issues, such as ES capabilities [22:04:18] we don't actually run most of our qunit tests on multiple browsers, do we? [22:04:22] thedj TimStarling do you think we would find many browser bugs in a *unit* test? I think they tend to be more browser agnostic in my experience. Regardless, Special:JavaScriptTest can still still be used where it makes sense. [22:04:30] I've been studying the MDN JavaScript docs lately for my PEG.js (now WikiPEG) work, the support tables are often surprising [22:04:37] thedj: We run OOUI's tests via Karma on both Chrome and Firefox, and for OOjs on Safari, IE and others (via a third party). [22:04:49] We have some linting to prevent people using ES features that aren't available in all our supported browsers, but it isn't perfect [22:04:50] for ES6 stuff especially [22:05:29] edsanders: could we use a transpiler for unsupported features and throw an error if the transpiled code doesn't match the original? [22:05:45] niedzielski: i have found several tablesorter browserbugs with unittest.. Only triggered with nice browser scheduling race conditions etc.. [22:05:54] e.g. https://github.com/wikimedia/eslint-config-wikimedia/issues/140 [22:05:59] (we just use whatever we feel like and transpile it to a specific set of supported browsers) [22:06:09] milimetric: We don't shard RL cache based on browser features; adding that would be fun but costly. [22:06:23] also, date parsers in browsers are all over the place... [22:06:43] James_F: no, just as a linter not to actually ship the transpiled code [22:06:54] milimetric: my understanding is that babel transpiles syntactic features, but doesn't polyfill [22:07:01] milimetric: Oh, right, we've written that already. eslint-config-wikimedia. [22:07:07] This sounds like to me like a decision between easy to config v.s. reliability [22:07:26] I would say that the proper place to catch browser bugs is in integration testing. As more and more complex JS gets written, headless unit tests for pure logic things (ensuring a method returns the expected value, etc.) will be useful as well. [22:07:31] thedj: https://gerrit.wikimedia.org/r/plugins/gitiles/oojs/core/+/master/Gruntfile.js#94 FWIW is where we configure SauceLabs for OOjs. [22:07:35] edsanders: webpack or babel or some combination can do both, but I was mostly talking about linting [22:07:55] thedj: that sounds useful. I think when the browser support is suspect, we can stick with Special:JavaScriptTest, especially for big integration tests. Lots of code doesn't need this though and Special:JavaScriptTest is overkill. [22:08:14] agreed. [22:08:15] Most regressions in our Mobile site are caught in our Selenium tests despite our Qunit tests being run in both a special page and headless Node.js. [22:08:15] EricGardner_: There are browser bugs on seriously low-level stuff, like old Opera silently dropping array elements after the 255th. [22:08:19] EricGardner_: But yes. [22:08:31] I can see that linting would detect new syntactic features, but what about new methods (or new features in methods) on standard objects? detecting those would depend on type inference [22:08:33] QUnit's purpose seems to be stopping errors prior to merge [22:08:34] the demarcation between unit test vs. integration test is a bit more fuzzy for JS then other languages, I think [22:08:54] For me it's more about code functioning correctly [22:08:57] TimStarling: We wrote that into the eslint profile too (based on work from matmarex and jdlrobson). [22:09:04] since they are intended to run in the browser, which is a form of integration [22:09:17] tgr: yup, agree [22:09:20] Right now we're basically using QUnit for integration testing with Special:JavaScriptTest [22:09:27] TimStarling: See https://github.com/wikimedia/eslint-config-wikimedia/blob/master/language/not-es6.json etc. [22:09:40] only recently a failure in a test in MobileFrontend blocked Multimedia team from merging for a day : https://phabricator.wikimedia.org/T218172 [22:09:41] I am not too knowledgeable in this topic so honestly, I am just talking out my rear end here, but would node.js make it easier to support future ES* versions [22:09:52] Zppix: That's unrelated. [22:09:57] This attribute of the test environment is more of a nuisance than helpful [22:10:00] tgr: I'm suggesting the demarcation can stay fuzzy, but that we call it something more generic so we're not as tied to the tooling [22:10:02] James_F: ah okay i wasnt sure [22:10:35] that's more the bundeling/dist/deploy stuff of the previous meeting that would influence that really. [22:11:10] soo.... [22:11:25] milimetric: we are currently calling our test directories 'phpunit', 'qunit' and such typically... so the names are already tied to tooling [22:11:44] :) we're weird [22:12:03] node runners go... but with karma options, and lets not don't destroy old qunit just yet ? [22:13:09] thedj: are you seeking consensus? [22:13:15] James_F: TimStarling: Yes, we don't have type inference, hence the bug I linked to before. It's a bit of a guessing game, but for the most part you can assume that foo.padStart() is using String.prototype.padStart. [22:13:16] sort of ;) [22:13:17] let's spell that out more [22:13:19] the dev depencies chain might still be an issue that effects this however [22:13:55] any thoughts on that ? [22:14:28] thedj: sorry but can you clarify a little more? [22:14:33] aka, different moment lib version in RL as deployed on the site, vs as defined for nodejs. [22:14:37] moment.js [22:15:13] What is the issue with running the tests standalone in the browser via Karma? Is it just performance? [22:15:28] thedj: I think if you need a dependency chain then you're not writing unit tests and should use the real test runner. [22:15:28] edsanders: I haven't explored doing that and am happy to explore doing so if that's a blocker [22:15:29] It avoids all the compatibility / js-dom issues. [22:15:46] Practically: I would love to (and am highly motivated) to move https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/487168/ through to completion so understanding what is problematic about that patch would be helpful. [22:15:50] thedj: i think that's just back to the slow and unstable but real environment (aka browser) vs fast and stable simulated [22:16:19] edsanders: are there karma runners for all browsers we support? [22:16:20] i'm thinking CI, having differnt versions of libraries than what production site runs seems problematic... [22:16:24] milimetric: Yes. [22:16:29] oh, cool, [22:16:30] milimetric: See my links above. :-) [22:16:33] that's the gap i want to address in some way. [22:16:35] then... why don't we just do that [22:16:54] Stand-alone browser testing isn't as fast as doing it on node. [22:16:54] I'm sure it is a bit slower but for VE, which is an absolute monster test suite in JS terms, it takes ~10 seconds [22:16:59] **** 15 minutes left #info is your friend **** [22:17:28] edsanders: but have you tried it with the karma watcher? That tends to be really fast [22:17:44] (where it just runs tests affected by file changes) [22:17:48] even better [22:18:27] milimetric: I'm saying it's already fast enough :) [22:18:42] karma used to be problematic to install sometimes, but i believe it's much easier than it was like 3 years ago when i last used. (npm install can manage it all). [22:19:02] one thing I would add is that I'd like to be able to run the tests in the browser on occasion for debugging [22:19:27] karma has the little debug interface, no? [22:19:38] (that's how I debug anyway) [22:19:53] every time I've tried to use node-debug it's been pretty terrible compared to Chrome debugging [22:20:05] edsanders: oh, I mean karma with the chrome runner [22:20:11] it gets a little banner with "Debug" [22:20:20] and especially if the tests involves the DOM [22:20:29] edsanders: is your code using file imports? [22:20:39] i mean, this is an optional thing, right? no one is saying you absolutely have to use node for qunit. just use it where it makes sense [22:20:44] I find when every function is exported and importing it's super easy to debug a test failure in node [22:21:03] if it's not it suggests the thing being tested is not well isolated [22:21:10] we've found it super fast and stable to run code in isolation without the browser and it's been great to have coverage reorts [22:21:32] we've got about 90% coverage in popups [22:21:56] our karma headed runner generates coverage too [22:21:57] i think it's useful for some other projects and i think it would be valuable to formalize that it can be [22:22:19] that's great. keep using that if it makes sense. this is one more option that is awesome [22:22:38] niedzielski: when you say optional, you mean the tests would still be available to run in the browser? [22:22:41] maybe the standard tooling we develop here can do both? [22:22:44] headless and headed? [22:23:00] like npm run karma, npm run karma-headless? [22:23:09] edsanders: not necessarily, optional for authors to decide [22:23:39] well if that is applied to all our core tests it becomes less optional for developers [22:23:42] as long as we use standardized commands accross all repos for the same setup please ! [22:23:49] milimetric: that would be useful [22:23:51] `npm test` [22:23:51] otherwise its a hellhole. [22:24:05] thedj: I'm thinking some standard template, yea [22:24:43] I think in general both this and the previous discussion point to standard webpack/karma/package.json templates [22:24:53] #info milimetric and edsanders apparently support using karma to run tests on browsers, jdlrobson says "I haven't explored doing that and am happy to explore doing so if that's a blocker" [22:24:57] milimetric edsanders supporting multiple forms of execution is a waste of resources in my opinion. the author should pick the runner that makes sense for a given test [22:24:58] niedzielski: thats fine for tests, but i want to be able to indeed be able to know that npm run karma always uses karma, if that repo has karma etc.. [22:24:58] that we would maintain and people could optionally use [22:25:06] what npm test bundles i don't care about [22:25:13] Is this a final call? [22:25:28] https://www.irccloud.com/pastebin/dXlE6Oxq/ [22:25:45] thedj: headless tests should run fast enough that the thought of excluding them from any npm test script is off the table [22:25:47] let me paste that without the snippet: [22:25:47] Since we're running out of time.. Essentially I'm offering to build on https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/487168/ and do the following: [22:25:53] ) Provide tests which can't break other extensions [22:25:58] 2) Code coverage reports for any repo that wants them [22:26:03] 3) every ResourceLoader module built in such a way it could be published on npm [22:26:07] niedzielski: I think it depends on the context - if the tests are passing then use headless if it's faster - having the headed option allows CI to do more thorough cross browser testing, and allows devs to use a browser debugger [22:26:09] 4) modernising code to use require statements using ResourceLoader packageFiles [22:26:11] niedzielski: its not about choosing, its about knowning what does what. [22:26:14] 5) backwards compatible - if you don't like it don't worry about it. [22:26:19] 6) importantly providing a boilerplate that others can copy from [22:26:41] sorry, couldn't be here. reading backscroll, but will probably comment on phab instead. [22:26:57] if npm test is defined to do npm run karma, npm run nodejs then im fine. if it does only one of those, i'm also fine. [22:27:30] thedj: currently, in mf and popups, the distinction is test:unit for headless tests and npm test should call that [22:27:31] Is the only constraint being able to run these tests in karma/real browser or are there things that block doing this or are there other things I need to be aware about? [22:27:33] but if it's npm run karma in one repo and npm run karmav2 then i'm gonna hate on people ;) [22:27:54] thedj: to me this is why it is important to do in core - it leads by example [22:28:17] headless != node. chrome headless works from CLI, and can be invoked from npm-run (as core does already for karma). chrome headless starts in <100ms, overhead is insignificant compared to node-qunit. afaik if you find karma slow, it's because the test is slow. Do we have evidence that it would run faster when executed without a browser? We'd also have to mock all native libraries, and support "jsdom", and can't use or have to mock all other [22:28:17] browser features and APIs. [22:28:23] #info jdlrobson> thedj: to me this is why it is important to do in core - it leads by example [22:28:47] edsanders: i don't think you should write the test for the node runner if you want to catch a bug in the browser [22:29:00] Krinkle: I was wonder why it was so fast.... [22:29:23] niedzielski: that assumes you know it's going to trip a browser bug [22:30:05] I see on phab that jdlrobson is open to running this more disciplined use of qunit in a browser as well. If I understood that correctly, could we just do that instead? There's imho no added value is also running it node if it's equally fast, and has the benefit of actually testing an environment we support. [22:30:08] being able to run JS code in the browser when in production it is also going to be run in browser seems like common sense to me [22:30:16] and we've run out of time, please continue on phab [22:30:17] edsanders: that's right. however, no test covers every possible scenario [22:30:22] #endmeeting [22:30:22] Meeting ended Wed Mar 20 22:30:22 2019 UTC. Information about MeetBot at http://wiki.debian.org/MeetBot . (v 0.1.4) [22:30:22] Minutes: https://tools.wmflabs.org/meetbot/wikimedia-office/2019/wikimedia-office.2019-03-20-21.45.html [22:30:22] Minutes (text): https://tools.wmflabs.org/meetbot/wikimedia-office/2019/wikimedia-office.2019-03-20-21.45.txt [22:30:22] Minutes (wiki): https://tools.wmflabs.org/meetbot/wikimedia-office/2019/wikimedia-office.2019-03-20-21.45.wiki [22:30:23] Log: https://tools.wmflabs.org/meetbot/wikimedia-office/2019/wikimedia-office.2019-03-20-21.45.log.html [22:30:33] From what Krinkle is saying it sounds like there is no benefit to using node over chrome-headless? [22:30:54] code coverage using nyc also works fine in browsers. [22:31:00] But from my perspective the rest of what is proposed sounds good [22:31:01] I kind of agree, but we should take it to phab now, and maybe jdlrobson and niedzielski can point to examples [22:31:12] the coverage reports at https://doc.wikimedia.org/cover/ for oojs, oojs-ui and visualeditor come from karma, qunit and nyc. [22:31:30] (examples of where karma itself is too slow) [22:31:46] (and Chrome headless) [22:32:07] i think it would be beneficial to also read some of the past reports on node.js tests vs special:javascripttest here if want to understand why anyone would favor the former over the latter in some cases: https://phabricator.wikimedia.org/phame/post/view/96/fast_and_isolated_js_unit_tests/ [22:32:08] (for an example of Chrome-headless, you can run grunt karma:main from withing VisualEditor/lib/ve) [22:32:31] Krinkle: right now Special:Javascript/qunit runs in about 30s on every jenkins job - compared to 10 seconds using Node.js. Compare https://integration.wikimedia.org/ci/job/mwgate-npm-node-6-docker/89812/consoleFull (start 20:28:19 end 20:28:30) with https://integration.wikimedia.org/ci/job/mwgate-npm-node-6-docker/89812/consoleFull (start: 20:32:18, finish 20:32:55). However, the pain I experience is local - it [22:32:31] takes me 3 minutes to run Special:JavaScript/qunit on my local machine compared to 10s) [22:32:57] I don't think we're talking about using Special:JS [22:33:01] Remember not everyone has the same kind of machine. [22:33:16] yeah, agreed, special page tests are super slow [22:33:17] just using chrome-headless for your non-MW standalone unit tests [22:33:25] jdlrobson: It's doing a lot more, that's not a straight comparison. Also, did you compare to headless chrome? [22:33:31] Special:JavaScriptTest?debug=true is almost not an option on my machine [22:34:03] Krinkle: but my main problem here is there is zero value (at least to me) in running unit tests from other repos [22:34:30] for me that part sounds perfectly reasonable [22:34:35] agreed [22:34:39] If the problem statement is to get code coverage, let's make the RFC about that. If it's about speed, lets bench and focus on that. These are very different RFCs. Right now hanging them together means it's either this solution which happens to solve two unrelated problems or nothing. That's not productive. [22:35:11] I am not opposing node-qunit at this time. It may be a viable solution in both RFCs. [22:35:52] I suppose the problem is as along as *some* of your tests are still in Special:JS, you still have to run that in CI [22:36:08] Okay so if we're agreed on not running tests from other repos that's a great start. [22:36:38] ^+1! [22:37:45] tests are not auto-discoverable they need to be registered via hook. A test also can't using file imports right given the current setup? [22:38:12] So right now if I add a test, not only do I have to add it, I need to fill out some paperwork in the appropriate places (usually Hooks.php) [22:38:35] Not running tests from other repos ever? Or just not in some cases? [22:38:35] tests can be registered in extension.json now [22:38:50] Nikerabbit: I think this depends on what you are testing [22:38:51] Our jenkins jobs already run npm-test. Individual projects can use that to lint and run other arbitrary things. There is no RFC needed for adopting that in more repos for projects interested in doing tests that way. Having said that, it'd be equally complicated to use a local npm-test based approach in a browser (local HTML, no MW relation) and get the same benefits, speed and coverage. [22:39:02] that seems like a fairly small overhead to adding a file [22:39:09] uncomplicated* [22:40:19] I'd love to have less merge blockers in CI due to test in Wikibase (as an example) failing for some random reason, but I'd also be worried if those kind of failures would go undetected and cause issues in production [22:40:54] Well, we're not going to drop running all(*) tests in gate. [22:41:17] Nikerabbit: This is just "not locally for the dev". [22:41:30] sounds reasonable [22:41:33] jdlrobson: editing extension.json when adding a new feature/test suite is the issue? If https://xkcd.com/1205/ approves, then one might create a little script that globs test/* and then assigns an extension.json property. Alternatively, it's also fine to set the attribute from a PHP hook and use glob(). Last I checked, repos you maintain already did that. [22:41:47] the theory of unit testing is that if the unit works, and the wires are connected correctly, the whole works. "the wires connected correctly" bit is where integration tests come in [22:44:33] Krinkle: it's one small issue in a big set of issues. Fundamentally why do we test our code differently from what is the standard elsewhere (headless mocha/qunit/ tests)? Can anyone provide some background on why we do things the way we do them here? [22:44:45] If it just a case of we were trailblazing in this area and never re-evaluated? [22:44:55] *is [22:45:31] the standard for frontend libaries and node libraries is not the same. [22:45:57] I haven't yet found a frontend library that doesn't run their unit test in a browser during development. [22:46:03] (unless they had not tests) [22:46:11] Krinkle: can you give some examples? [22:46:37] every upstream module we use in prod? [22:46:42] which ones don't? [22:47:42] anyway, I'm going to sign off now. Let's focus the RFC on the specific problems you'd like to address, what the requirements are, and then we can weigh the costs of different solutions. [22:48:23] It being different from elsewhere is a valid cost to consider, That'd be good to include indeed. [22:48:53] https://github.com/videojs/video.js/blob/master/package.json "test:unit": "karma start test/karma.conf.js", [22:51:24] I doubt any popular frontend library would accept a PR to have 'npm test' not run their unit tests in a browser anymore. There seems little incentive to accept that, all else being equal. [22:52:48] typescript itself does: [22:52:49] gulp runtests # Run tests using the built compiler and test infrastructure. # You can override the host or specify a test for this command. # Use --host= or --tests=. [22:52:53] gulp runtests-browser # Runs the tests using the built run.js file. Syntax is gulp runtests. Optional [22:53:00] React? [22:54:36] Well I'd love some practical feedback on what's wrong with a patch like https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/487168/ [22:54:51] I'm happy and will explore what it would take to run those in karma [22:55:07] rxjs runs on pure node, but it depends on modern JS features. [22:55:37] you need the polyfills. (and it doesn't do dom stuff). [22:57:52] Krinkle: i'd love to talk more about projects that do not run unit tests in a browser, I'm pretty sure React.js is one of them (https://github.com/facebook/react/blob/master/package.json#L102) and it feels like this is the norm, but I'd need to do more research [22:58:40] i think it's a fallacy in today's world that unit test fail in different modern browsers. It might have been true in the IE6 days, but I don't believe it's the case today. [22:58:55] Selenium would be a much better solution for catching those (although we obviously have lots of problems in that arena) [22:59:37] jdlrobson: I dont think that's the norm. If facebook releases react versions without knowing their unit tests pass in a browser, that is simply foolish. [22:59:43] and a reason not to use it [22:59:46] but, alas [22:59:47] https://github.com/facebook/react/issues/11079 [23:00:00] They're looking to reduce the cost of their manual QA efforts by making them run in a browser. [23:00:18] There's various mentions of Firefox and Chrome specific issues in React source code. [23:00:52] If youre running in both, why bother running in node as well. [23:01:07] Like I said, let's continue on phab with neutral objectives and compare the approaches available to us. [23:01:21] Krinkle: but there is no need for developers to run these locally. If we want to run these on jenkins or deploy i'm all for it, but i think we should optimise for providing code coverage and encouraging the writing of tests. IF we have no tests it doesn't matter where we run them. [23:01:49] jdlrobson: how does Chrome headless not achieve those goals? [23:02:05] You can get speed and coverage without running it in node.js. Getting code coverage is not a new invention in these new node.js based toys. This is a solved problem. [23:02:14] I'm going now. [23:02:19] react is also pure node.. [23:02:40] Yeah, react tests on node is an integration test effectively. They support that run-time for SSR [23:02:48] o/ [23:02:48] but i see tickets like: "We should really have a testcase for this browser issue" and then the ticket is closed without a testcase... [23:02:58] so... that's cool. [23:03:30] bedtime for me.. [23:03:55] react devs do advise using mocha for your react based apps if you want browser tests [23:04:03] nite nite