Make tests for bug 663695 and friends - 'Reorder Attachments' feature set (incl. Bug 1417856, Bug 1426344, Bug 1425891, Bug 1427037)

RESOLVED FIXED in Thunderbird 60.0

Status

enhancement
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: aceman, Assigned: aceman)

Tracking

(Blocks 1 bug)

Trunk
Thunderbird 60.0
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 10 obsolete attachments)

21.79 KB, patch
aceman
: review+
Details | Diff | Splinter Review
5.67 KB, patch
aceman
: review+
Details | Diff | Splinter Review
Assignee

Description

2 years ago
I'll make a test for the new attachment reordering feature I promised in bug 663695 comment 81.
Flags: in-testsuite?
Assignee

Comment 1

2 years ago
Posted patch 1416474-test.patch (obsolete) — Splinter Review
I created a test which checks all actions available in the popup panel.

Try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c350b218281edaa2746d4beb7ca7ff972fbfb413

Thomas, should I add anything else?
Attachment #8929439 - Flags: feedback?(bugzilla2007)

Comment 2

2 years ago
Comment on attachment 8929439 [details] [diff] [review]
1416474-test.patch

Thank you for writing up the test! :)
Looks right so far.

But you skipped all the really interesting cases of multiple, non-adjacent selections (let's call them "munas")...

* move-top must fold munas at the top on first click
* move-up must move munas as they are (every block moving exactly one item up), then as they reach top border, they start to fold at the top, so every item which can still move will move exactly one item up until it reaches the top block.
* move-together-up looks good
* move-together down is available via keyboard alt+cursor right so it should also be tested
* move-down must move munas as they are (every block moving exactly one item down), then as they reach bottom border, they start to fold at the bottom, every item which can still move will move exactly one item up until it reaches the bottom block
* move-bottom must fold munas at the bottom on first click
* toggle-sort should also have a testcase with munas; if munas are inherently sorted (but with gaps), click on sort must bundle them up but preserve the current sort order; if it's a single sorted block, must sort the other way round; and if it's a single unsorted block, sort ascending (you tested only the last one).

I think we also need to move around an adjacent multiple-items block in every direction, because moving blocks uses a different algorithm compared to moving single items.

It really depends how deep we want to test this, but the more complex cases are definitely the more interesting ones, because moving a single item is much simpler than moving disjunct selections. Also, there are different types of munas, you can also have an adjacent block between non-adjacent blocks, and that should still move around correctly:

1
2#
3
4#
5#
6
7#
8

I did spend a lot of time to make all the edge cases work, as we can't stop users from having bizarre selections, and it should still work in a predictable way. I don't like disabling things which can work and might be useful to someone.

Furthermore: do we want to test enabling/disabling of commands?
There's also a lot of sophistication in that.

For sorting, we might want to add testcase for the cases of bug 1417856 after that lands.
Attachment #8929439 - Flags: feedback?(bugzilla2007)

Comment 3

2 years ago
(In reply to Thomas D. from comment #2)

> * move-down must move munas as they are (every block moving exactly one item
> down), then as they reach bottom border, they start to fold at the bottom,
> every item which can still move will move exactly one item up until it
> reaches the bottom block

typo: ...will move exactly one item *down* until...
Assignee

Comment 4

2 years ago
Yeah, this is a rudimentary test that checks the panel at least appears and the basic commands work.
We do not need to test every possible option.

But we can if you can provide the test cases.
If you can write me sets of
[ <which items to select>, <which command in the menu to select>, <what the outcome is (order of names)> ] I can encode that in the test.

Yes, test for bug 1417856 will be added in that bug.
Assignee

Updated

2 years ago
Depends on: 1417856
Assignee

Comment 5

2 years ago
Posted patch 1416474-test.patch v2 (obsolete) — Splinter Review
This is what I mean. Do you want to add some more actions?
Attachment #8929439 - Attachment is obsolete: true
Attachment #8933064 - Flags: feedback?(bugzilla2007)

Comment 6

2 years ago
Comment on attachment 8933064 [details] [diff] [review]
1416474-test.patch v2

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

Smart abstraction! This makes adding more tests for complex cases of reordering really easy. I'll write them up soon.
Attachment #8933064 - Flags: feedback?(bugzilla2007) → feedback+

Comment 7

2 years ago
So here's a completely new sequence of reorder_actions which tests movements for simple and complex selections.

Pls embed this into your patch, and don't forget to initialize
> const initialAttachmentNames = ["a", "x", "C", "y1", "y2", "B", "z", "b", "bb"].

Tia.
Attachment #8934139 - Flags: review?(acelists)

Updated

2 years ago
Attachment #8934139 - Attachment filename: reorder-attach.test.js → reorder-attach.test.js.txt
Attachment #8934139 - Attachment mime type: application/x-javascript → text/plain

Comment 8

2 years ago
I recommend keeping this comment which I added right above reorder_actions array:
  // Initial list of attachments as defined in constant initialAttachmentNames:
  //          ["a", "x", "C", "y1", "y2", "B", "z", "b", "bb"]
The actual definition is quite far away in code, but is required to understand the starting point of the movement tests.

Comment 9

2 years ago
How hard would it be to add a test for this hidden complementary feature which is only keyboard-accessible?
> * move-together downwards is available via keyboard (Alt + Cursor right on Windows)

As it's not exposed in the UI, I guess it's more likely to break unnoticed so it makes sense to test.
Flags: needinfo?(acelists)
Assignee

Comment 10

2 years ago
Comment on attachment 8934139 [details]
reorder_actions array v.3: Testing simple and complex selections.

Great, this even passed at second run (after fixing the initial order).
I'll make a patch out of it.
Flags: needinfo?(acelists)
Attachment #8934139 - Flags: review?(acelists) → review+
Assignee

Comment 11

2 years ago
Yes, using hotkeys instead of buttons is possible to add :) I've added some examples.
Attachment #8933064 - Attachment is obsolete: true
Attachment #8934842 - Flags: feedback?(bugzilla2007)
Assignee

Comment 12

2 years ago
I converted your test cases into a patch. You can add testing of key presses into it now. Needs to be applied on top of the first patch "basic".
Attachment #8934139 - Attachment is obsolete: true
Attachment #8934843 - Flags: review+

Comment 13

2 years ago
Comment on attachment 8934842 [details] [diff] [review]
Patch Part1, V.3: Basic tests (Set 1), initial test infrastructure for simple and transparent test definitions

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

Thank you very much for including keyboard usage into your test framework, nice abstraction and so convenient for test action consumers :)
I have written up the respective keyboard actions for my upcoming patch, which also includes fixes for the nits mentioned below, including some code reshuffling.

::: mail/test/mozmill/composition/test-attachment.js
@@ +43,5 @@
>  
>  function setupModule(module) {
> +  for (let lib of MODULE_REQUIRES) {
> +    collector.getModule(lib).installInto(module);
> +  }

Amazing

@@ +324,5 @@
> +  }
> +}
> +
> +/**
> + * Check that the compose window has the attachments we expect.

This function does more than this comment.

@@ +331,5 @@
> + * @param aReorder_actions  An array of objects specifying reordering action:
> + *                          { select: array of attachment item indexes to select,
> + *                            button: ID of button to click in the reordering menu,
> + *                            key:    key to press instead of a click,
> + *                            key_modifiers: { shiftKey: bool, accelKey: bool, ctrlKey: bool },

altKey is also needed.

@@ +346,5 @@
> +    }
> +    // Take action.
> +    if ("button" in action)
> +      aController.click(aController.eid(action.button));
> +    else if ("key" in action)

Nice.

@@ +347,5 @@
> +    // Take action.
> +    if ("button" in action)
> +      aController.click(aController.eid(action.button));
> +    else if ("key" in action)
> +      aController.keypress(null, action.key, action.key_modifiers);

I was looking at mozmill documentation [1] and couldn't find what the first argument (null) is about. Pls enlighten me.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Mozmill/Mozmill_Element_Object#keypress.28.29

@@ +361,5 @@
> + */
> +function test_attachment_reordering() {
> +  let cwc = open_compose_new_mail();
> +
> +  // Create a set of attachments.

Depending on setup, the initial setup may be needed for more than one test sequence. So we could include this into subtest_check_reordering_actions() and just pass initialAttachmentNames as a parameter.

@@ +371,5 @@
> +
> +  assert_equals(cwc.window.attachmentsCount(), initialAttachmentNames.length);
> +  check_attachment_names(cwc, initialAttachmentNames);
> +
> +  // Bring up the reordering panel.

Dito, with more similar tests, this could go into the sub function, with a boolean flag, because we don't want this for every sub test.

@@ +392,5 @@
> +      button: "btn_moveAttachmentTop",
> +      result: ["x", "a", "b", "B", "bb", "C"] },
> +    { select: [0],
> +      key:    "VK_DOWN",
> +      key_modifiers: { altKey: true },

I think this would fail on MAC, where we use different modifiers (and sometimes even different keys).

@@ +408,5 @@
> +  ];
> +
> +  subtest_check_reordering_actions(cwc, reorder_actions_basic);
> +
> +  // Click on the editor which should close the panel.

Based on boolean panel flag, this can also go into the sub test.
Attachment #8934842 - Flags: feedback?(bugzilla2007) → feedback+

Comment 14

2 years ago
To be applied on top of the first part, attachment 8934842 [details] [diff] [review].

This reshuffles/streamlines the existing test framework to allow for more sub tests with less code and more transparency.
Includes new subtests for keyboard reordering with/without panel.

I deliberately excluded sorting from the keyboard tests because we have bugs with access key usage, which is required for sorting via keyboard (panel closes after sorting via access key, and for some, but not all other reordering actions via access key).
Attachment #8934843 - Attachment is obsolete: true
Attachment #8937274 - Flags: review?(acelists)

Comment 15

2 years ago
We may or may not want to wait for bug 1426344 which seeks to change all the MAC keyboard shortcuts, which will translate to some minor changes here.

Comment 16

2 years ago
Given the delicacy of bug 1425891, it would certainly be nice to test that too (new keyboard shortcut Alt+Y for sorting with and without panel).
Depends on: 1426344, 1425891

Comment 17

2 years ago
Requires bug 1426344 and bug 1425891.

This adjusts the tests to the new MAC order of bug 1426344, and rudimentary test for bug 1425891 which we can't fully test because the root cause isn't fixed yet.
Attachment #8937274 - Attachment is obsolete: true
Attachment #8937274 - Flags: review?(acelists)
Attachment #8938813 - Flags: review?(acelists)

Updated

2 years ago
Attachment #8934843 - Attachment description: 1416474-test.patch advanced → Patch Part2, V.1 (Advanced tests Set 1, incl. infrastructure)

Updated

2 years ago
Attachment #8937274 - Attachment description: 1416474.test-reorder.part2.patch advanced (reshuffled code for even more test transparency with less code) → Patch Part2, V.2: Improved test infrastructure for even more test transparency with less code; added Set3: keyboard tests with and without panel.

Updated

2 years ago
Attachment #8934843 - Attachment description: Patch Part2, V.1 (Advanced tests Set 1, incl. infrastructure) → Patch Part2, V.1: Include Thomas' advanced test matrix (Set2)

Updated

2 years ago
Attachment #8938813 - Attachment description: 1416474.test-reorder.part2.patch advanced (more tests, incl. bug 1426344, bug 1425891, and various panel open/close) → Patch Part2, V.3: add more advanced tests (Set4), incl. bug 1426344 (new shortcuts MAC), bug 1425891 (Alt+Y for sorting), and various panel open/close

Comment 18

2 years ago
To be applied on top of attachment 8934842 [details] [diff] [review] (basic tests, initial infrastructure).

Aceman, pls review this now so that we can get the tests landed to ensure the quality of attachment reordering feature set.

I think it's mostly about reviewing the new test infrastructure, as I added some more abstraction in the process which makes the actual test definitions simpler and more transparent. The test definitions (Sets 1 - 4, sometimes tested more than once with a different flavor) should review themselves when the tests are running... ;)

There were some bugs in the last patch, should be fixed now (forgot to add attachments before calling reordering panel, outdated shortcuts MAC). Unfortunately, I can't test this, so it's brain-tested only which may fail ;)

I also included tests for Bug 1427037: Alt+X (Win/Linux) | Ctrl+X (MAC) from msg body to open the 'Reorder Attachments' panel.
Underlying behaviour has changed quite a lot since we started, but I hope that this is now up to date.
Attachment #8938813 - Attachment is obsolete: true
Attachment #8938813 - Flags: review?(acelists)
Attachment #8939489 - Flags: review?(acelists)

Updated

2 years ago
Summary: Make test for bug 663695 - attachment reordering feature → Make tests for bug 663695 and friends - 'Reorder Attachments' feature set (incl. Bug 1417856, Bug 1426344, Bug 1425891, Bug 1427037)

Updated

2 years ago
Attachment #8939489 - Attachment description: Patch Part2, V. 4: Bugfixes, new infrastructure for panel open/close keyboard tests, incl. Bug 1427037 (Alt+X/Ctrl+X from msg body for reordering) → Patch Part2, V. 4: Advanced Tests (Sets 1-4) & new test infrastructure. Bugfixes, new function for panel open/close keyboard tests, incl. Bug 1427037 (Alt+X/Ctrl+X from msg body for reordering)

Updated

2 years ago
Attachment #8939489 - Attachment description: Patch Part2, V. 4: Advanced Tests (Sets 1-4) & new test infrastructure. Bugfixes, new function for panel open/close keyboard tests, incl. Bug 1427037 (Alt+X/Ctrl+X from msg body for reordering) → Patch Part2, V. 4: Advanced Tests (Sets 2-4) & new test infrastructure. Bugfixes, new function for panel open/close keyboard tests, incl. Bug 1427037 (Alt+X/Ctrl+X from msg body for reordering)

Updated

2 years ago
Attachment #8934842 - Attachment description: 1416474-test.patch simple v3 → Patch Part1, V.3: Basic tests (Set 1), initial test infrastructure for simple and transparent test definitions

Updated

2 years ago
Attachment #8939489 - Attachment description: Patch Part2, V. 4: Advanced Tests (Sets 2-4) & new test infrastructure. Bugfixes, new function for panel open/close keyboard tests, incl. Bug 1427037 (Alt+X/Ctrl+X from msg body for reordering) → Patch Part2, V. 4: Advanced Tests (Sets 2-4) & improved test infrastructure. Bugfixes, new function for panel open/close keyboard tests, incl. Bug 1427037 (Alt+X/Ctrl+X from msg body for reordering)

Comment 19

2 years ago
Before landing, if there are no more iterations, pls adjust the commit message:

Bug 1416474 - Add advanced tests for 'Reorder Attachments' feature set: bug 663695, plus bug 1417856, bug 1426344, bug 1425891, bug 1427037 (test algorithms by aceman and ThomasD). r?aceman
Assignee

Comment 20

2 years ago
Thanks for the great patch and many test cases.

I updated the patch for some typos to make it work.
It seems logically correct now, but passes for me locally only *sometimes*.

The failures can be seen on try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=f84f38b18648b61b076fd4c36cca207be7848e63

On Linux is chokes on the ESC to close the panel. This ought to work normally but fails in the test often. There may be some timing or focus problem here which we need to workaround.

On OS X it fails at some other place, which I haven't investigated yet. Maybe some pressed key is not right (does not match the real code).
Attachment #8940597 - Flags: feedback+
Assignee

Updated

2 years ago
Attachment #8939489 - Attachment is obsolete: true
Attachment #8939489 - Flags: review?(acelists)
Assignee

Comment 21

Last year
Posted patch Patch Part1, V.3.1 (obsolete) — Splinter Review
Looks like this finally passes on try, at least on Taskcluster:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=9aaddf2efaee65d35c65f71993905c056d3a6eb7
Attachment #8934842 - Attachment is obsolete: true
Attachment #8951892 - Flags: review?(jorgk)
Assignee

Comment 22

Last year
Attachment #8940597 - Attachment is obsolete: true
Attachment #8951893 - Flags: review+
Comment on attachment 8951892 [details] [diff] [review]
Patch Part1, V.3.1

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

Finally ;-)

::: mail/test/mozmill/composition/test-attachment.js
@@ +324,5 @@
> +  }
> +}
> +
> +/**
> + * Check that the compose window has the attachments we expect.

Hmm, same comment as the function right above. Maybe a little more related to what the function is doing would be good.
Attachment #8951892 - Flags: review?(jorgk) → review+
Assignee

Comment 24

Last year
Yeah, thanks.
Attachment #8951892 - Attachment is obsolete: true
Attachment #8951904 - Flags: review+
Assignee

Updated

Last year
Keywords: checkin-needed

Comment 25

Last year
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/58ab93600fa3
Add some basic tests for bug 663695 and bug 1417856. r=jorgk
https://hg.mozilla.org/comm-central/rev/72d5d5085685
Add advanced tests for 'Reorder Attachments' feature set: bug 663695, bug 1417856, bug 1426344, bug 1425891, bug 1427037 (test algorithms by aceman and ThomasD). r=aceman
Status: ASSIGNED → RESOLVED
Closed: Last year
Keywords: checkin-needed
Resolution: --- → FIXED

Updated

Last year
Target Milestone: --- → Thunderbird 60.0
(In reply to :aceman from comment #21)
> Looks like this finally passes on try, at least on Taskcluster:
On buildbot, too :-)
Assignee

Updated

Last year
Blocks: 1440951
You need to log in before you can comment on or make changes to this bug.