mirror of
https://gerrit.wikimedia.org/r/mediawiki/extensions/Echo
synced 2024-11-24 07:54:13 +00:00
5a76489be0
== Problem 1 == As of I09c27a084100b223, tests/qunit/index.js or equiv was used to load test files asynchronously from a using() callback. This was untracked by RL or QUnit, and thus sometimes ended up finishing after the test runner was already done executing all tests. In CI this means the tests are sometimes never loaded and the browser (or Node) process already killed before they even have a chance to arrive. Prior to QUnit 2.17, this was no way of detecting this. As of QUnit 2.17 (core upgraded last week) when running tests manually the following helpful warnings appear in the console: > [warning] Unexpected test after runEnd. > [warning] This is unstable and will fail in QUnit 3.0. > test @ qunit.js > tests/qunit/model/test_mw.echo.dm.SeenTimeModel.js > require There were about 1072 instances of this warning, all from Echo. Fix this problem by removing the async callbacks and specifying the two modules as normal dependencies instead. == Problem 2 == Class NotificationBadge was being loaded in a strange way out of bound. This was a violation of module boundaries and should not be needed other than for a temporary hack or other tech debt. More generally when a test uses `packageFiles` this is a likely sign of tech debt or misunderstandings. Instead, depend on `ext.echo.mobile` and export/import the class as normal. After this, the test module can use `scripts` instead. == Problem 3 == The `ext.echo.mobile` uses a Mustache template which the test was also duplicating a reference to. This is no longer needed now. Due to the `qunit/index.js` file carefully splitting the operations between template assignment and file loading, I wondered whether it was meaning to replace or mock it with something else, but it simply refers to the same file and only does this because it wasn't using the module directly. This is now resolved. If you do need to mock in the future, this can simply be done by assigning `NotificationBadge.prototype.template` from a beforeEach() callback in the test suite, or by supporting it property as a constructor option in NotificationBadge.js and assigning `this.template` there, which is supported by the mobile `View` class already it seems and would follow DI patterns more effectively. == Problem 4 == Most of the Echo tests were ignored sometimes and executed other times. The test for `ext.echo.mobile` in particular though was never executed in CI specifically because: > Undefined module: 'mobile.startup' This became a hard error with this patch, which is fixed by the CI config change with Ie9dabe3269c56fa76db8e51. Bug: T299780 Change-Id: Ie4a87f3b8085fd6ae53ec586c1782cc266d5288a |
||
|---|---|---|
| .. | ||
| list.js | ||
| NotificationBadge.js | ||
| NotificationBadge.mustache | ||
| notifications.js | ||
| notificationsFilterOverlay.js | ||
| notificationsFilterOverlay.less | ||
| overlay.js | ||
| overlay.less | ||