Closed Bug 911465 Opened 8 years ago Closed 8 years ago

Add tests for thread-sorting behavior

Categories

(Thunderbird :: Folder and Message Lists, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 26.0

People

(Reporter: wanderer, Assigned: wanderer)

Details

Attachments

(1 file, 2 obsolete files)

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.
Attachment #798183 - Flags: review?(mbanner)
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.
Attachment #798183 - Attachment is obsolete: true
Attachment #798183 - Flags: review?(mbanner)
Attachment #803029 - Flags: review?(mbanner)
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.
Attachment #803029 - Flags: review?(mbanner) → review+
Good point. Fixed.
Attachment #803029 - Attachment is obsolete: true
Assignee: nobody → wanderer
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: checkin-needed
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
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 26.0
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.
You need to log in before you can comment on or make changes to this bug.