[00:19:44] (03PS1) 10Krinkle: [WIP] Test [extensions/MultimediaViewer] - 10https://gerrit.wikimedia.org/r/121905 [00:20:18] (03CR) 10jenkins-bot: [V: 04-1] [WIP] Test [extensions/MultimediaViewer] - 10https://gerrit.wikimedia.org/r/121905 (owner: 10Krinkle) [00:23:15] (03CR) 10Gilles: Replay early thumb link clicks when bootstrap is ready to receive them (032 comments) [extensions/MultimediaViewer] - 10https://gerrit.wikimedia.org/r/121629 (owner: 10Gilles) [00:54:44] (03CR) 10Gergő Tisza: Replay early thumb link clicks when bootstrap is ready to receive them (031 comment) [extensions/MultimediaViewer] - 10https://gerrit.wikimedia.org/r/121629 (owner: 10Gilles) [01:44:01] (03PS1) 10Gergő Tisza: Make $.animate a noop in some tests to avoid conflicts [extensions/MultimediaViewer] - 10https://gerrit.wikimedia.org/r/121909 [01:44:37] (03CR) 10jenkins-bot: [V: 04-1] Make $.animate a noop in some tests to avoid conflicts [extensions/MultimediaViewer] - 10https://gerrit.wikimedia.org/r/121909 (owner: 10Gergő Tisza) [01:50:23] maybe it wasn't the only one [01:50:39] basically I started commenting out tests in order, until commenting those made the issue go away [01:50:59] but maybe there were other tests before those that also triggered an animation and ran over, but by that time they were commented out [01:52:10] do you have any tip on how to reproduce this locally? [01:52:28] i use chrome and all tests pass [01:52:39] just having core at the latest master and chrome with ?debug=1 [01:52:49] also with cache disabled in chrome [01:52:58] (in the developer tools) [01:53:13] I don't think there's anything else that's special about how I run it [01:54:38] still no failures [01:56:58] can you try adding clock.restore() to the end of the color.js test? [01:57:07] I think you can basically run the tests and hunt down any .animate calls that end up reaching the real jQuery animate, by putting a breakpoint [01:57:12] i'm not sure if that happens automatically or not [01:57:18] they should all be cleaned anyway, even if they don't trigger the issue right now [01:57:40] i was kind of hoping to avoid that :) [01:58:25] with your changeset applied I can't trigger it anymore [01:58:43] there's your invite to the breakpoint doom [01:59:30] and if you don't apply the changeset but do restore the clock after the color.js test? [02:00:11] this.clock.restore(); right after the assertion? [02:00:19] or after the tick? [02:00:28] at the end [02:00:58] or in the breakdown callback, but i don't know the exact syntax for that [02:01:17] between line 16 and line 17? or 17 and 18? [02:02:04] 16 and 17 [02:03:56] the bug still happens [02:04:07] bah [02:04:22] animation hunting weekend it is [02:04:58] it shouldn't be that bad, we only have 3 or 4 sources of them and most of the tests that deal with those areas directly disable the parent function or the animation [02:05:06] i'll finish for today though, unless this is blocking you [02:05:16] it's not, we can do that on monday [02:05:38] ok, see you later [16:08:28] (03PS1) 10Siebrand: Migrate to JSON i18n [extensions/UploadWizard] - 10https://gerrit.wikimedia.org/r/121960 [16:15:36] (03CR) 10Nikerabbit: [C: 032] Migrate to JSON i18n [extensions/UploadWizard] - 10https://gerrit.wikimedia.org/r/121960 (owner: 10Siebrand) [16:15:44] (03Merged) 10jenkins-bot: Migrate to JSON i18n [extensions/UploadWizard] - 10https://gerrit.wikimedia.org/r/121960 (owner: 10Siebrand) [19:56:33] (03PS2) 10Gilles: Make $.animate a noop in some tests to avoid conflicts [extensions/MultimediaViewer] - 10https://gerrit.wikimedia.org/r/121909 (owner: 10Gergő Tisza) [20:01:53] (03CR) 10Gilles: "That should be all of them, I used a break point in jQuery to find them. The only test that still calls $.animate indirectly (Metadata scr" [extensions/MultimediaViewer] - 10https://gerrit.wikimedia.org/r/121909 (owner: 10Gergő Tisza) [20:02:44] tgr: looks solved. it wasn't that bad, really [20:07:31] gi11es: thanks! [20:07:53] i've been thinking about how to improve our tests [20:08:11] one easy win would be to use sinon.js sandboxes [20:09:00] you wrap the whole test in an extra function and use sinon.stub(object, 'method', function) to mock globals functions like mw.message [20:09:42] when control exits the function, sinon restores it -> more readable, and the global state is not messed up if something throws an error and the test is interrupted halfway [20:10:03] that sounds interesting, but can't we do it in the setup phase like the color test? [20:10:26] wrapping each test in a function call isn't the most elegant-looking thing [20:10:46] i thought about that, but you have only one setup function per test suite, not per test [20:11:38] can't we augment QUnit.test with our own stuff? [20:12:07] we could define our own function that calls QUnit.test, with extra parameters for sinon [20:12:21] we can, when you call QUnit.module you can pass a config object which gives you a lot of options [20:12:29] i still have to read up on that [20:12:41] worth exploring, but I think we should file it for after the launch [20:12:59] but i think wrapping everything in a sinon sandbox is something that could be done on the core level [20:13:25] also possible that it already happens and i just dont know about it [20:13:32] ok, i'll file a card [20:14:03] I'll file a task about isolating all our scroll and animate calls so that we override our own functions instead of jQuery's to neuter the animations [20:14:16] i'll ask on wikitech-l how other people do UI tests [20:14:34] I'm guessing most of them don't have coverage anywhere near ours [20:14:36] but the QA team's position seems to be that you don't [20:14:52] not after every commit anyway, and certainly not on developer machines [20:14:53] selenium tests are painfully slow to write and take so much time to debug when an issue arises [20:15:21] yeah, what i want to know is whether anyone else does QUnit UI tests [20:15:46] probably not, but people don't do that for silly ideological reasons [20:16:08] the name says "Unit", it must be used for unit tests, not integration tests... [20:16:12] there are a lot of people and a lot of ideologies, i would not generalize [20:16:36] the asynchronous issues of animate also exist in selenium [20:16:43] and anyway i think using unit test frameworks for integration tests is pretty common [20:16:49] it's an issue with JS in general, it's impossible to reset the state between subtests [20:17:04] moving these to seleniu, written identiclly, we'd still run into exactly the same issues [20:17:16] except it'd take 5 times the amount of time to write and debug [20:17:28] i don't think selenium is a realistic option, an average selenium test takes several seconds, running hundreds of them while you are debugging is just not plausible [20:17:48] you just can't run them on commits because they're so slow, so that's a no-go for me [20:18:12] at one point in time QUnit had an option to run every test in its own iframe [20:18:23] they ended up dropping it, not sure why [20:18:27] it's not exactly rocket science to make disable async things/make them run instantly in our qunit integration tests [20:18:59] sure, but async is not the only way for tests to conflict [20:19:03] not to mention, async issues are not specific to UI things [20:19:47] it's pretty much the only one we've run into, though. with the cleanup of the codebase we've done, we rarely ever run into side effects between tests because of the UI, since we have the fixture, etc. [20:19:52] the most common cause for us used to be the mocking thing, i think, and it seems we have a clean solution for that [20:20:18] right, both refactoring code to avoid mocking global things and sinon when we can't help it [20:21:28] I think most people give up on doing integration tests with qunit because they don't have the discipline to get to the point where we're at [20:21:32] they don't use fixtures, etc. [20:21:40] so they end up with a ton of side effects and just give up [20:22:05] associating this with "it's a bad idea, it's dangerous" [20:22:14] when you are actually testing an async feature, the fake timer and xhr in sinon.js provides a very good way to do it, btw [20:22:16] doing it wrong is a bad idea, that's all :) [20:23:42] well, testing is hard [20:24:10] i don't think we are doing it wrong, but i am sure there is still a lot of place for improvement [20:25:41] for example, we have very few tests that actually need to be integration tests [20:25:57] i.e. they really test integration of different components [20:27:24] for most, i think it is just for the lack of dependency injection that we end up with so many objects in a single test [20:28:47] also, i was wondering if it is worth splitting up our ui components into a controller and a real ui object [20:29:13] where the controller is not allowed to touch the dom and the view is not allowed to have state [20:30:14] that way the contract-type tests could concentrate on the controllers and work with pure objects [20:31:24] (03CR) 10Gergő Tisza: [C: 032] "Thanks!" [extensions/MultimediaViewer] - 10https://gerrit.wikimedia.org/r/121909 (owner: 10Gergő Tisza) [20:31:54] (03Merged) 10jenkins-bot: Make $.animate a noop in some tests to avoid conflicts [extensions/MultimediaViewer] - 10https://gerrit.wikimedia.org/r/121909 (owner: 10Gergő Tisza) [21:00:31] gi11es: have you seen https://www.mediawiki.org/wiki/User:Rillke/Chemical_Markup_support_for_Wikimedia_Commons ? [21:00:40] seems pretty well thought out [21:07:41] (03PS1) 10Siebrand: Migrate to JSON i18n [extensions/TimedMediaHandler] - 10https://gerrit.wikimedia.org/r/121981 [21:08:41] (03PS2) 10Siebrand: Migrate to JSON i18n [extensions/TimedMediaHandler] - 10https://gerrit.wikimedia.org/r/121981 [21:09:05] (03PS3) 10Siebrand: Migrate to JSON i18n [extensions/TimedMediaHandler] - 10https://gerrit.wikimedia.org/r/121981 [21:17:23] tgr: yes, he emailed me and br(y|i)an [21:18:41] I think we're going to select him [21:19:03] he got a grant for finishing pronunciation recording too [21:52:54] (03PS4) 10Gergő Tisza: Replay early thumb link clicks when bootstrap is ready to receive them [extensions/MultimediaViewer] - 10https://gerrit.wikimedia.org/r/121629 (owner: 10Gilles)