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

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, enhancement)

Description

[:aceman]

I'll make a test for the new attachment reordering feature I promised in bug 663695 comment 81.

Comment 1

[:aceman]

Attachment: 1416474-test.patch

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?

Comment 2

[Thomas D. (:thomas8)]

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.

Comment 3

[Thomas D. (:thomas8)]

(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...

Comment 4

[:aceman]

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.

Comment 5

[:aceman]

Attachment: 1416474-test.patch v2

This is what I mean. Do you want to add some more actions?

Comment 6

[Thomas D. (:thomas8)]

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.

Comment 7

[Thomas D. (:thomas8)]

Attachment: reorder_actions array v.3: Testing simple and complex selections.

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.

Comment 8

[Thomas D. (:thomas8)]

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

[Thomas D. (:thomas8)]

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.

Comment 10

[:aceman]

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.

Comment 11

[:aceman]

Attachment: Patch Part1, V.3: Basic tests (Set 1), initial test infrastructure for simple and transparent test definitions

Yes, using hotkeys instead of buttons is possible to add :) I've added some examples.

Comment 12

[:aceman]

Attachment: Patch Part2, V.1: Include Thomas' advanced test matrix (Set2)

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".

Comment 13

[Thomas D. (:thomas8)]

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.

Comment 14

[Thomas D. (:thomas8)]

Attachment: Patch Part2, V.2: Improved test infrastructure for even more test transparency with less code; added Set3: keyboard tests with and without panel.

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).

Comment 15

[Thomas D. (:thomas8)]

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

[Thomas D. (:thomas8)]

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).

Comment 17

[Thomas D. (:thomas8)]

Attachment: 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

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.

Comment 18

[Thomas D. (:thomas8)]

Attachment: 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)

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.

Comment 19

[Thomas D. (:thomas8)]

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

Comment 20

[:aceman]

Attachment: Patch Part2, V. 4.1: 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)

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).

Comment 21

[:aceman]

Attachment: Patch Part1, V.3.1

Looks like this finally passes on try, at least on Taskcluster:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=9aaddf2efaee65d35c65f71993905c056d3a6eb7

Comment 22

[:aceman]

Attachment: Patch Part2, V.4.2

Comment 23

[Jorg K (CEST = GMT+2)]

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.

Comment 24

[:aceman]

Attachment: Patch Part1, V.3.2

Yeah, thanks.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
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

Comment 26

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #21)
> Looks like this finally passes on try, at least on Taskcluster:
On buildbot, too :-)