1666997 - Refactor common calendar mochitest helper functions into single CalendarTestUtils

Status: RESOLVED FIXED

(Calendar :: General, task)

Description

[Lasana Murray]

Some of the more recent calendar mochitest style tests have similar helper functions either in head.js files or in the test file itself.

We could probably improve the experience of writing tests for calendar by adding a CalendarTestUtils.jsm object that will contain typical boilerplate associated with calendar albeit, not using mozmill.

Comment 1

[Lasana Murray]

Attachment: 1666997.patch

I took some of functions from head.js and added a function for removing a calendar with its items for now. I can move more stuff in there later.

Comment 2

[Lasana Murray]

In a follow up bug I can replace the functions in head.js with the ones from CalendarTestUtils.jsm but may need to observe this a little more first.

Comment 3

[Lasana Murray]

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

Comment 4

[Geoff Lankow (:darktrojan)]

Comment on attachment 9182485 [details] [diff] [review]
1666997.patch

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

::: calendar/test/browser/contextMenu/browser_edit.js
@@ +47,5 @@
>    let calendarProxy = cal.async.promisifyCalendar(calendar);
>    calendar.name = "Editable";
>    manager.registerCalendar(calendar);
> +
> +  registerCleanupFunction(async () => CalendarTestUtils.removeCalendarAndItems(manager, calendar));

You don't need the "async" if you're returning the result of an async function. It's no big deal, but I thought you'd like to know.

::: calendar/test/modules/CalendarTestUtils.jsm
@@ +64,5 @@
> +   * Ensures the calendar tab is open
> +   *
> +   * @param {Window} win
> +   */
> +  async openCalendarTab(win) {

These come from calendar/test/browser/head.js right? Let's remove them from there so there's only one copy.

@@ +158,5 @@
> +    });
> +  },
> +
> +  /**
> +   * Removes a calendar but first ensures its items have been deleted.

Why? Deleting a calendar removes its items. I can't see how this helps other than making the tests do more work.

Comment 5

[Lasana Murray]

Attachment: 1666997v2.patch

I remove the removeCalendarAndItems function, I thought the items were not deleted but I realize now why I did. In the head.js file I replaced the functions with bound versions from CalendarTestUtils.

Comment 6

[Lasana Murray]

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

Comment 7

[Geoff Lankow (:darktrojan)]

Comment on attachment 9182733 [details] [diff] [review]
1666997v2.patch

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

::: calendar/test/browser/contextMenu/browser_edit.js
@@ +49,5 @@
>    manager.registerCalendar(calendar);
> +
> +  registerCleanupFunction(() => {
> +    manager.removeCalendar(calendar);
> +  });

This (and the others like it) can go back to how it was.

::: calendar/test/browser/head.js
@@ +12,4 @@
>  
> +const openCalendarTab = CalendarTestUtils.openCalendarTab.bind(CalendarTestUtils, window);
> +const setCalendarView = CalendarTestUtils.setCalendarView.bind(CalendarTestUtils, window);
> +const closeCalendarTab = CalendarTestUtils.closeCalendarTab.bind(CalendarTestUtils, window);

I would rather have these removed from head.js completely and the references updated. I guess you did this to avoid making a lot of changes, but the changes are justified IMO.

::: calendar/test/modules/CalendarTestUtils.jsm
@@ +121,5 @@
> +   * Removes every item found in the provided calendar.
> +   *
> +   * @param {calICalendar} calendar
> +   */
> +  async removeCalendarItems(calendar) {

This is now unused.

Comment 8

[Lasana Murray]

I would rather have these removed from head.js completely and the references updated. I guess you did this to avoid making a lot of changes, but the changes are justified IMO.

Correct, I wanted to avoid changing those tests directly in this patch, I'll update them now.

Are jsm imports preferred over the head.js files?

Comment 9

[Lasana Murray]

Attachment: 1666997v3.patch

I made the changes. I ran into a few test failures locally using an artifact build particularly browser_calendarList timing out. I get the same failures without the patch though.

Maybe browser_calendarList needs to be changed to clean up on failure?

Comment 10

[Geoff Lankow (:darktrojan)]

Comment on attachment 9183029 [details] [diff] [review]
1666997v3.patch

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

Nice work. Thanks. Just a few things to tidy and you're good to go.

::: calendar/test/browser/browser_localICS.js
@@ -16,5 @@
>  var { setData } = ChromeUtils.import("resource://testing-common/mozmill/ItemEditingHelpers.jsm");
>  var { plan_for_modal_dialog, wait_for_modal_dialog } = ChromeUtils.import(
>    "resource://testing-common/mozmill/WindowHelpers.jsm"
>  );
> -

Please keep this empty line. I try to keep a distinction between imports of testing modules and imports of core modules. (There's no rule that says you have to, and I probably haven't in lots of places, but it's a good idea.)

::: calendar/test/browser/browser_todayPane_visibility.js
@@ +6,3 @@
>     openNewCalendarEventTab, openNewCalendarTaskTab, openPreferencesTab,
>     openTasksTab, selectCalendarEventTab, selectCalendarTaskTab,
>     selectFolderTab */

Tidy these up.

Comment 11

[Geoff Lankow (:darktrojan)]

(In reply to Lasana Murray from comment #9)

I made the changes. I ran into a few test failures locally using an artifact build particularly browser_calendarList timing out. I get the same failures without the patch though.

Maybe browser_calendarList needs to be changed to clean up on failure?

Yeah, it does this. I've no idea why. Bug 1604369 if you want to work on it.

Comment 12

[Lasana Murray]

Attachment: 1666997v4.patch

Tidied up!

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/258debd6c817
Add CalendarTestUtils module for common calendar testing helpers. r=darktrojan