1683444 - Add some of the helper functions from browser_EditButton.js to CalendarTestUtils.jsm

Status: RESOLVED FIXED

(Calendar :: Dialogs, task)

Description

[Lasana Murray]

To cut down on duplication, I want to implement a method for grabbing and waiting for an event box to appear, one for opening the event box and waiting on its window and methods for opening events from items similar to invokeEventDialog etc.

Comment 1

[Lasana Murray]

Attachment: bug1683444.patch

This patch adds some functions to CalendarTestUtils that could be used in other tests. Specifically methods for grabbing event items from the month view and opening the viewing/editing dialogs.

The goal is to eventually replace some of the functionality in CalendarUtils here to use css selectors and promises where needed. I'll tackle those in follow up bugs but for now I'm using browser_eventDialogEditButton to observe the impact.

Comment 2

[Lasana Murray]

Try run:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&selectedTaskRun=Q4WKNpihQ2SCWC85q-iYqA.0

Comment 3

[Geoff Lankow (:darktrojan)]

I think it might be tidier if your edit*FromItem functions accepted a callback as CalendarUtils' horrible invoke functions do, so as to keep some conceptual separation between what happens in the main window and what happens in the dialog. They also handle waiting for the inner document and dialog closing.

Here's your first edit operation as I imagine it (setData and saveAndCloseItemDialog come from ItemEditingHelpers which is entirely Promise-based now.):

await CalendarTestUtils.editFromItem(
  window,
  await CalendarTestUtils.monthView.waitForItemAt(window),
  (eventWindow, iframeWindow) => {
    await setData(eventWindow, iframeWindow, {
      title: "Edited Non-Recurring Event"
    });
    saveAndCloseItemDialog(eventWindow);
  }
);

This would also make bringing the older tests up to speed much much easier. What do you think?

Comment 4

[Lasana Murray]

This would also make bringing the older tests up to speed much much easier. What do you think?

My plan was to work on getting rid of the invoke functions after this. The callbacks make the flow harder to follow when they start spanning lots lines. I can add an editItemAt method and have it return the same tuple the callbacks supply.

to keep some conceptual separation between what happens in the main window and what happens in the dialog

I was thinking of changing this to a class based approach where the util class has to be instantiated instead of being a static object. The window reference could be passed in to the constructor, reducing the parameter count of the methods and a separate class could be created for the event dialogs. Thoughts?

Comment 5

[Lasana Murray]

Attachment: bug1683444v2.patch

Here is an update version of this:

1. Renamed some of the methods.
2. Added editItem*At() methods to monthView so the item retrieval can be skipped in tests that can. These also provide both the window, iframe.contentWindow and iframe.contentDocument as the resolved value.
3. Removed dismiss and save methods, instead embedded ItemEditingHelpers in CalendarTestUtils.
4. Copied menulistSelect into ItemEditingHelpers, in a follow up bug I'll remove the duplication and update references.

Note: After monthView I'll most likely end up adding properties for day, week view etc. with similar methods.

Comment 6

[Geoff Lankow (:darktrojan)]

Comment on attachment 9202169 [details] [diff] [review]
bug1683444v2.patch

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

I like where this is heading, but it's not there yet.

::: calendar/test/browser/eventDialog/browser_eventDialogEditButton.js
@@ +89,5 @@
>        },
>      });
>    });
>  
> +  let [editWindow, , editDoc] = await CalendarTestUtils.monthView.editItemAt(window);

For consistency, if you're going to name something "fooWindow" you should call the document "fooDocument", or "fooWin" and "fooDoc".

But in this case they're referring to different things so one of them shouldn't even be called "edit". That's just confusing.

@@ +111,5 @@
> + * Tests the dropdown menu is displayed for a recurring event.
> + */
> +add_task(async function testRecurringEvent() {
> +  let event = await calendar.addItem(createRecurringEvent());
> +  registerCleanupFunction(() => calendar.deleteItem(event));

You're already deleting the calendar at the end of the test, not much need to delete the event in it.

@@ +142,5 @@
> + */
> +add_task(async function testEditThisOccurrence() {
> +  let event = createRecurringEvent();
> +  event = await calendar.addItem(event);
> +  registerCleanupFunction(async () => calendar.deleteItem(event));

Same.

::: calendar/test/modules/CalendarTestUtils.jsm
@@ +43,5 @@
> +     *
> +     * @throws If the day or week parameters are out of range.
> +     * @returns {MozCalendarMonthDayBox}
> +     */
> +    getDayBox(win, day = 1, week = 1) {

The caller should have to specify the day and week. Defaulting to the first day of the first week is a bit weird.

Also the week and day arguments should be the other way around.

@@ +54,5 @@
> +
> +      return win.document.documentElement.querySelector(
> +        `#month-view > .mainbox > .monthgrid > tr:nth-child(${week}) >` +
> +          `td:nth-child(${day}) > calendar-month-day-box`
> +      );

What about storing a reference to .monthgrid on the monthView object then doing:
this.grid.rows[week].cells[day].querySelector("calendar-month-day-box")

@@ +69,5 @@
> +     *
> +     * @return {MozCalendarMonthDayBoxItem}
> +     */
> +    async waitForItemAt(win, day = 1, week = 1, index = 1) {
> +      let month = CalendarTestUtils.monthView.getDayBox(win, day, week);

let dayBox?

@@ +200,5 @@
> +   * @param {string} - type
> +   *
> +   * @returns {Proxy}
> +   */
> +  createCalendar(name, type = "storage") {

You should call this createProxyCalendar.

It could add the clean-up function automatically. I'm not sure off the top of my head how it'd check if the calendar had already been removed but I'm sure there's a way.

@@ +315,5 @@
> +   *
> +   * @returns {Array} - An array with the following elements:
> +   *  1. The window of the dialog.
> +   *  2. The contentWindow property of the embeded iframe.
> +   *  3. The contentDocument of the embeded iframe.

This could return an object, so the caller can collect only the variables it wants without needing to know the order. Also I think you should either return both documents or neither.

Also you should define this as a type in the JSDoc and use that type for all the return values.

::: calendar/test/modules/ItemEditingHelpers.jsm
@@ +11,5 @@
>  
>  var { cal } = ChromeUtils.import("resource:///modules/calendar/calUtils.jsm");
>  var { Assert } = ChromeUtils.import("resource://testing-common/Assert.jsm");
>  var { BrowserTestUtils } = ChromeUtils.import("resource://testing-common/BrowserTestUtils.jsm");
> +var EventUtils = ChromeUtils.import("resource://testing-common/mozmill/EventUtils.jsm");

Not required. Look up a few lines.

Comment 7

[Lasana Murray]

For consistency, if you're going to name something "fooWindow" you should call the document "fooDocument", or "fooWin" and "fooDoc".
But in this case they're referring to different things so one of them shouldn't even be called "edit". That's just confusing.

editWindow is the window of the dialog for editing an item, but the actual document containing the fields is nested within a window -> dialog -> iframe -> window -> #document. I do agree it's confusing however.

You're already deleting the calendar at the end of the test, not much need to delete the event in it.

Leftover code from when I thought registerCleanUpFunction worked differently.

Also the week and day arguments should be the other way around.

Are you sure about this?
The order of the arguments is (x,y) that is, specify the horizontal number first (day), then the vertical (week of the month).

What about storing a reference to .monthgrid on the monthView object then doing:
this.grid.rows[week].cells[day].querySelector("calendar-month-day-box")

I don't mind, but this is a static object with the window instance passed to most methods, not a good idea to hold on to anything.
If we turn it into a class that needs to be instantiated with a window however...

This could return an object, so the caller can collect only the variables it wants without needing to know the order. Also I think you should either return both documents or neither.

Comment 8

[Lasana Murray]

Attachment: bug1683444v3.patch

Removed the duplicate clean up handlers, default parameters. Changed return type of editItemAt to an object and provided a jsdoc type. Included changes from D104909. Removed extra EventUtils import.

Comment 9

[Lasana Murray]

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

Comment 10

[Lasana Murray]

Attachment: bug1683444v3.patch

Just fixing some typos. I was running the wrong test locally.

Comment 11

[Geoff Lankow (:darktrojan)]

(In reply to Lasana Murray from comment #7)

Are you sure about this?
The order of the arguments is (x,y) that is, specify the horizontal number first (day), then the vertical (week of the month).

Yes. Because weeks are more significant than days, and days are more significant than hours. Also when it comes to tables it's more common to talk about rows then columns.

What about storing a reference to .monthgrid on the monthView object then doing:
this.grid.rows[week].cells[day].querySelector("calendar-month-day-box")

I don't mind, but this is a static object with the window instance passed to most methods, not a good idea to hold on to anything.
If we turn it into a class that needs to be instantiated with a window however...

Fair point.

Comment 12

[Lasana Murray]

Attachment: bug1683444v4.patch

Argument order changed: day, week -> week, day.

I guess pitching A1 syntax here won't get me far.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/0d9b6a00fffa
Move helper functions of browser_eventDialogEditButton.js into CalendarTestUtils. r=darktrojan DONTBUILD