911465 - Add tests for thread-sorting behavior

Status: RESOLVED FIXED

(Thunderbird :: Folder and Message Lists, defect)

Description

[Andrew Buehler]

Attachment: thunderbird-threadsorting-tests-bydate-v1.diff

Here's a first stab at an xpcshell test to verify the sort order of threads, as distinct from the sort order of messages within threads. Currently only the case of sorting threads by date is tested.

This complements the pref introduced as a fix for bug 495946, and was requested under that bug. Both sides of the pref are tested.

Extending this to cover other sort types (e.g. by-subject or by-sender) should be relatively straightforward, particularly if there's no need to try to address all cases with a single message set. However, it's not clear whether there is any particular benefit to testing the other types, so I haven't touched them for now.

Comment 1

[Andrew Buehler]

Attachment: thunderbird-threadsorting-tests-bydate-v2.diff

Revised patch. Test descending as well as ascending sort order, and test the default pref state last so that any later tests don't end up running with the pref still in a non-default state.

Comment 2

[Mark Banner (:standard8)]

Comment on attachment 803029 [details] [diff] [review]
thunderbird-threadsorting-tests-bydate-v2.diff

Review of attachment 803029 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. r=Standard8 with the comment fixed.

::: mailnews/base/test/unit/test_nsMsgDBView.js
@@ +748,5 @@
> +  assert_view_message_at_indices(msg3, 0);
> +  assert_view_message_at_indices(msg2, 1);
> +  assert_view_message_at_indices(msg1, 2);
> +
> +  Services.prefs.setBoolPref("mailnews.sort_threads_by_root", false);

Its generally better to use clearUserPref to reset the value.

Comment 3

[Andrew Buehler]

Attachment: thunderbird-threadsorting-tests-bydate-v3.diff

Good point. Fixed.

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/7aca43233eaa

To make life easier for those checking in on your behalf, please make sure that you have Mercurial configured to generate patches with the needed commit information. Thanks!
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 5

[Andrew Buehler]

I already had most of those settings (except the 'e' of '-Ue' and the empty 'mq' setting), based on "how to best configure Mercurial for working with Mozilla trees" advice from someone in IRC; what I was missing appears to have been generating the patch with 'hg qnew' instead of the much more standard and obvious 'hg diff'.

I've just tried it out, with a test commit containing the same diff as what I had attached to this bug, and now I'm regretting it. It didn't just create a patch file, or even a patch file with additional information for a commit; it actually created a commit in the log, which fortunately turned out to be easy to get rid of since someone else already got an explanation of how to do it on stackoverflow. (Having to find the patch file under a predefined directory, rather than being able to specify where I want to put it at creation time, is also annoying.)

I do like the idea of being able to provide the commit message (et cetera) myself, and I can probably adjust my workflow to find a way to work with this for future reference - but I do not find this intuitive, and I would suggest that it would be a good idea to explain in the linked FAQ entry that this does *not* merely generate a patch file without lasting side effects.