Closed Bug 1666997 Opened 1 year ago Closed 11 months ago

Refactor common calendar mochitest helper functions into single CalendarTestUtils

Categories

(Calendar :: General, task)

Tracking

(thunderbird_esr78 wontfix, thunderbird83 wontfix)

RESOLVED FIXED
84 Branch
Tracking Status
thunderbird_esr78 --- wontfix
thunderbird83 --- wontfix

People

(Reporter: lasana, Assigned: lasana)

Details

Attachments

(1 file, 3 obsolete files)

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.

Summary: Refactor common calendar mochitest helper functions into single helper object → Refactor common calendar mochitest helper functions into single CalendarTestUtils
Assignee: nobody → lasana
Status: NEW → ASSIGNED
Attached patch 1666997.patch (obsolete) — Splinter Review

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.

Attachment #9182485 - Flags: review?(geoff)

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 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.
Attachment #9182485 - Flags: review?(geoff)
Attached patch 1666997v2.patch (obsolete) — Splinter Review

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.

Attachment #9182485 - Attachment is obsolete: true
Attachment #9182733 - Flags: review?(geoff)
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.
Attachment #9182733 - Flags: review?(geoff)

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?

Attached patch 1666997v3.patch (obsolete) — Splinter Review

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?

Attachment #9182733 - Attachment is obsolete: true
Attachment #9183029 - Flags: review?(geoff)
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.
Attachment #9183029 - Flags: review?(geoff) → review+

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

Attached patch 1666997v4.patchSplinter Review

Tidied up!

Attachment #9183029 - Attachment is obsolete: true
Attachment #9183185 - Flags: review?(geoff)
Attachment #9183185 - Flags: review?(geoff) → review+
Target Milestone: --- → 84 Branch

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/258debd6c817
Add CalendarTestUtils module for common calendar testing helpers. r=darktrojan

Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.