Sortable tables broken in 1.18 & trunk; needs tests & fixes or revert

List overview All Threads
Download

newer

older

Code review stats (was Re: Code...

Code Review for 1.18

Brion Vibber

22 Jun 2011 22 Jun '11

8:27 p.m.

The sortable table system in MediaWiki was completely rewritten in r86088; unfortunately this was done without benefit of any unit testing or regression testing, and there seem to be a lot of new bugs introduced. I've started adding test cases for table sorting in r90595; this adds a qunit test suite that creates some short tables (listing planets Mercury through Saturn and their radii) and tries setting up and triggering sorting, then compares the resulting order to the expected value. The ascending sort by name usually works, but if called twice on two tables, the second table usually gets sorted *completely* incorrectly. This can be easily confirmed by manual inspection by copying a page such as http://test.wikipedia.org/wiki/Planets_of_doom to your trunk wiki and sorting first one table, then the other. Numeric sorting on the radius column is also incorrect; the data set includes values with two different orders of magnitude (> 10k and < 10k), and they don't sort correctly. You can confirm on the test.wikipedia page that these sort as expected in 1.17. These specific bugs will also need to have test cases added, all of which are claimed to be fixed by r86088 or its follow-ups: * https://bugzilla.wikimedia.org/show_bug.cgi?id=8028 * https://bugzilla.wikimedia.org/show_bug.cgi?id=8115 * https://bugzilla.wikimedia.org/show_bug.cgi?id=15406 * https://bugzilla.wikimedia.org/show_bug.cgi?id=17141 * https://bugzilla.wikimedia.org/show_bug.cgi?id=8732 * https://bugzilla.wikimedia.org/show_bug.cgi?id=28775 As a reminder, you can run the qunit tests from your browser by simply going to the tests/qunit/ subdirectory of your wiki installation. General info: http://www.mediawiki.org/wiki/Manual:JavaScript_unit_testing -- brion

Show replies by date

Krinkle

22 Jun 22 Jun

9:32 p.m.

Brion Vibber wrote:

...

The legacy script was removed from svn without deprecation or quarantine phase, that is in contrary to the descriptoin and expectation set here [1]. Although a few legacy methods from wikibits.js were nuked right away because they can not co-exist at the same time, in general we should keep it around. For example addPortletLink and mw.util.addPorletLink. There's no need to remove the other. I suggest restoring the legacy ts_makeSortable functions in wikibits.js, except for one thing, which is sortables_init, it should not be applied to any table on-load (like nothing in a module should be executed on load, modules should only contain the plugins themselfs!) The new jQuery plugin would be in jquery.tablesorter.js which, like any module, also doesn't do anything on-load. Instead it is called in a lazy-loader for tables on the wiki page, this means on a unit test page both can be used seperately on indivudual tables since neither the legacy or the new one is called on load. This also makes sure our behaviour is in harmony with the expectation set by our javascript deprecation page [1] and will not break gadgets that are usiung (parts of) the legacy ts_* functions (which are global functions). Facts up front: this would mean adding back a method that is not used or going to be used on any page by default. On the other hand, if you take the introduction of the jquery.tablesorter out of the equation and compare to the previous release of MediaWiki, it means it wasn't touched at all and depracated by a modern module (like we've done for many things, normal procedure afaik). So summarized proposal: * Restore legacy script to comply with the deprecation guide (ie. not break gadgets that use (parts of) it) * Write unit tests for the legacy script * Write unit tests for hte new script * Fix the new script -- Krinkle [1] http://www.mediawiki.org/wiki/ResourceLoader/JavaScript_Deprecations

Brion Vibber

9:40 p.m.

New subject: Sortable tables broken in 1.18 & trunk; needs tests & fixes or revert

On Wed, Jun 22, 2011 at 2:32 PM, Krinkle <krinklemail(a)gmail.com> wrote:

...

Brion Vibber wrote:

The legacy script was removed from svn without deprecation or quarantine phase, that is in contrary to the descriptoin and expectation set here [1].

It's a reimplementation of an existing system that never gets manually called; since there's no API for it that doesn't really apply -- there's nothing to deprecate. It needs to *actually work* and not introduce regressions for having been refactored.

...

I suggest restoring the legacy ts_makeSortable functions in wikibits.js, except for one thing, which is sortables_init, it should not be applied to any table on-load (like nothing in a module should be executed on load, modules should only contain the plugins themselfs!)

To me this doesn't make sense; if it's not to be used, and there's no API designed for using it, and its existing implementation has been replaced, there's no need to keep unused code.

...

The new jQuery plugin would be in jquery.tablesorter.js which, like any module, also doesn't do anything on-load. Instead it is called in a lazy-loader for tables on the wiki page, this means on a unit test page both can be used seperately on indivudual tables since neither the legacy or the new one is called on load.

That's already how the jQuery module works, yes.

...

This also makes sure our behaviour is in harmony with the expectation set by our javascript deprecation page [1] and will not break gadgets that are usiung (parts of) the legacy ts_* functions (which are global functions).

Those are internal functions (just poorly namespaced), anything using them can be expected to fail at any time. So summarized proposal:

...

* Restore legacy script to comply with the deprecation guide (ie. not break gadgets that use (parts of) it)

Against -- no public API functions, doesn't apply.

...

* Write unit tests for the legacy script

Only needed if it's included and used, which is only needed if the new version is reverted

...

* Write unit tests for hte new script * Fix the new script

If it's reverted, these should be done during development of the new version prior to commit; if not, do them now. -- brion

Platonides

27 Jun 27 Jun

9:24 p.m.

New subject: Sortable tables broken in 1.18 & trunk; needs tests & fixes or revert

Brion Vibber wrote:

...

Those are internal functions (just poorly namespaced), anything using them can be expected to fail at any time.

I agree with you. I but note that the new code won't get the localization fixes done by overriding them, and the new system will apparently have bugs of not sorting correctly things (eg. months in content language) which the old script did. See for instance ts_resortTable at http://es.wikipedia.org/wiki/MediaWiki:Common.js I hope the new sorter provides a clean way to perform such localization, instead of the ugly way that ts_resortTable was.

Brion Vibber

9:33 p.m.

New subject: Sortable tables broken in 1.18 & trunk; needs tests & fixes or revert

On Mon, Jun 27, 2011 at 2:24 PM, Platonides <Platonides(a)gmail.com> wrote:

...

Brion Vibber wrote:

Those are internal functions (just poorly namespaced), anything using

them

can be expected to fail at any time.

In theory yes: * european-order date processing when the language file declares dmy order * explicit checks for month names in content language (confirmation tests welcome!) * I think it _should_ handle comma vs period, but need test cases! * handles language-specific custom character sorting if specified in the way needed for the new code (regex fragments to translate chars), though there doesn't seem to be re-ordering for digraphs or accented chars here. It's probably worth a look to see if any other wikis are doing this patching, if anything needs to be fixed or added to match. -- brion

Leo Koppelkamm

29 Jun 29 Jun

5:26 p.m.

New subject: Sortable tables broken in 1.18 & trunk; needs tests & fixes or revert

Additionally, a project can also cleanly define custom parsers (some unusual date/numbers or whatever format they have). $.tablesorter.addParser( { id: "specialFormat", is: function (s) { //Check if content is specialFormat return isSpecialFormat; }, format: function (s) { //Return normalized String/Number return s; }, type: "text" } ); Leo On Monday, June 27, 2011 at 11:33 PM, Brion Vibber wrote:

...

Brion Vibber

22 Jun 22 Jun

10:04 p.m.

On Wed, Jun 22, 2011 at 1:27 PM, Brion Vibber <brion(a)wikimedia.org> wrote:

...

[snip] The ascending sort by name usually works, but if called twice on two tables, the second table usually gets sorted *completely* incorrectly. This can be easily confirmed by manual inspection by copying a page such as http://test.wikipedia.org/wiki/Planets_of_doom to your trunk wiki and sorting first one table, then the other. Numeric sorting on the radius column is also incorrect; the data set includes values with two different orders of magnitude (> 10k and < 10k), and they don't sort correctly. You can confirm on the test.wikipedia page that these sort as expected in 1.17.

Awesome -- diebuche tracked those bugs down to the merge sort implementation and has just fixed it in r90612. The basic sort tests now run clean! I've marked r90612 for needing merge to 1.18 so we'll be good on branch; we'll still need more test cases to confirm fix of past known table sorting bugs so everybody, do feel free to help add those. :) Big thanks to everybody who's helped in getting the test systems running and the table code improved and fixed! -- brion

4713

days inactive

4720

days old

wikitech-l@lists.wikimedia.org

Manage subscription

6 comments

5 participants

tags (0)

participants (5)

Brion Vibber
Brion Vibber
Krinkle
Leo Koppelkamm
Platonides