1694631 - calendar tests should not look up elements by path

Status: RESOLVED FIXED

(Calendar :: General, task)

Description

[Magnus Melin [:mkmelin]]

A bunch of calendar tests are looking up elements by path, which makes it very easy to break them.

This bug is about removing that useage: https://searchfox.org/comm-central/rev/05df26e6ab23c89819da68ec4828591278317cc8/calendar/test/modules/CalendarUtils.jsm#88-136

For the bulk of these, the needed elements can be obtained by document.getElementById() or document.querySelector(). I suggest trying to take the one one-by-one.

When you change a test, make sure it still works. You can run e.g.
./mach test comm/calendar/test/browser/views/browser_multiweekView.js
or
./mach test --headless comm/calendar/test/browser/views/browser_multiweekView.js

Comment 1

[Magnus Melin [:mkmelin]]

Lasana, if you had something wip that would clash with this, please let me know.

Comment 2

[Henry Wilkes (they/them) [:henry-x]]

(In reply to Magnus Melin [:mkmelin] from comment #0)

A bunch of calendar tests are looking up elements by path, which makes it very easy to break them.

This bug is about removing that useage: https://searchfox.org/comm-central/rev/05df26e6ab23c89819da68ec4828591278317cc8/calendar/test/modules/CalendarUtils.jsm#88-136

For the bulk of these, the needed elements can be obtained by document.getElementById() or document.querySelector(). I suggest trying to take the one one-by-one.

When you change a test, make sure it still works. You can run e.g.
./mach test comm/calendar/test/browser/views/browser_multiweekView.js
or
./mach test --headless comm/calendar/test/browser/views/browser_multiweekView.js

Thanks for the direction.

I have an initial fix for just the browser_multiweekView.js add_task test.

I was going to make this change:

-  let day = lookup(`
-        ${MULTIWEEK_VIEW}/{"class":"mainbox"}/{"class":"monthgrid"}/[0]/{"selected":"true"}/[0]
-    `);
-  controller.waitFor(() => day.getNode().mDate.icalString == "20090101");
+  controller.waitFor(() =>
+    controller.window.document.querySelector(
+      '#multiweek-view td[selected="true"] > calendar-month-day-box'
+    ).mDate.icalString == "20090101"
+  );

where previously MULTIWEEK_VIEW was imported from CalendarUtils.jsm as:

'/id("messengerWindow")/{"class":"body"}/id("tabmail-container")/id("tabmail")/id("tabmail-tabbox")/id("tabpanelcontainer")/id("calendarTabPanel")/id("calendarContent")/id("calendarDisplayBox")/id("calendar-view-box")/id("view-box")/id("multiweek-view")'

but since MULTIWEEK_VIEW isn't used anywhere else, I was going to drop it from CalendarUtils.jsm.

Is this the kind of approach you were expecting? A lot of the logic has been moved out of CalendarUtils.jsm (the MULTIWEEK_VIEW selection and the lookup(...).getNode method are no longer used).

Also, do you think the selector

'#multiweek-view td[selected="true"] > calendar-month-day-box'

has the right kind of informational balance (not too verbose, but still descriptive)?

Comment 3

[Lasana Murray]

Hello Henry, welcome aboard!

In bug 1683444 I made some changes to CalendarTestUtils that you can see here:
https://hg.mozilla.org/comm-central/rev/0d9b6a00fffa

My goal is to replace CalendarUtils with CalenderTestUtils eventually. Regarding the path like stuff, you'll see a property called monthView in CalendarTestUtils that has a method, getDayBox() for looking up calendar-month-day-box elements. This method is a replacement for some those "lookup" functions you are cleaning up and the goal is to have a property for each calendar view with similar methods (monthView,dayView,weekView etc).

https://searchfox.org/comm-central/rev/77a876e480a7d3367efc168652c91f5323feb32f/calendar/test/modules/CalendarTestUtils.jsm#57

I haven't yet looked at how the various tests are using this path syntax though so I'm not too certain what should go in CalendarTestUtils and what should not. So in short, it will be really helpful if you can incorporate your changes into CalendarTestUtils once it's appropriate.

You can take a look at browser_eventDialogEditButton.js to get a feel for the direction I was going in.

Feel free to ping me if I can be of assistance!

Comment 4

[Henry Wilkes (they/them) [:henry-x]]

(In reply to Lasana Murray from comment #3)

Hello Henry, welcome aboard!

In bug 1683444 I made some changes to CalendarTestUtils that you can see here:
https://hg.mozilla.org/comm-central/rev/0d9b6a00fffa

My goal is to replace CalendarUtils with CalenderTestUtils eventually. Regarding the path like stuff, you'll see a property called monthView in CalendarTestUtils that has a method, getDayBox() for looking up calendar-month-day-box elements. This method is a replacement for some those "lookup" functions you are cleaning up and the goal is to have a property for each calendar view with similar methods (monthView,dayView,weekView etc).

Ah, that's handy. Yeah, I was looking to replace the lookupEventBox calls.

it will be really helpful if you can incorporate your changes into CalendarTestUtils once it's appropriate.

Can you explain this a bit more? Are you planning on adding more to CalendarTestUtils soon? Do you want me to convert to using your new methods instead of CalendarUtils, and adding methods to CalendarTestUtils as needed?

Comment 5

[Henry Wilkes (they/them) [:henry-x]]

It seems that the verbose path behaviour belongs to the Lookup class (inherits from ElemBase) from elementslib.jsm

https://searchfox.org/comm-central/rev/77a876e480a7d3367efc168652c91f5323feb32f/mail/test/browser/shared-modules/elementslib.jsm#167

As far as I can tell, only mail/test/browser/content-tabs/browser_installXpi.js and calendar/test/modules/CalendarUtils.jsm use it directly (and hence the calendar tests are using it indirectly through the lookup and lookupEventBox helpers).

So I think I'll just drop this class entirely and clean up in its absence.

Comment 6

[Magnus Melin [:mkmelin]]

We want to get rid of all of elementslib.jsm. It doesn't serve any (good) purpose anymore. Instead prefer pure methods per comment 0.

Comment 7

[Henry Wilkes (they/them) [:henry-x]]

(In reply to Magnus Melin [:mkmelin] from comment #6)

We want to get rid of all of elementslib.jsm. It doesn't serve any (good) purpose anymore. Instead prefer pure methods per comment 0.

OK, that's good to know. I think I will sometimes still need to use the wrapper Elem class from elementslib.jsm because the augmented controller's wrapped methods expect a ElemBase object with the getNode method (https://searchfox.org/comm-central/rev/77a876e480a7d3367efc168652c91f5323feb32f/mail/test/browser/shared-modules/WindowHelpers.jsm#1333).

E.g. situations like

elemEl = new Elem(document.querySelector(select));
augmented_controller.click(elemEl);

Comment 8

[Lasana Murray]

Just giving a quick reply:

Can you explain this a bit more? Are you planning on adding more to CalendarTestUtils soon?

yes, eventually

Can you explain this a bit more? Are you planning on adding more to CalendarTestUtils soon? Do you want me to convert to using your new methods instead of CalendarUtils, and adding methods to CalendarTestUtils as needed?

Go ahead with the goal of this bug. We can always move what needs to into CalendarTestUtils afterwards. It should not be too hard once we are using css selectors etc.

Comment 9

[Magnus Melin [:mkmelin]]

Yeah it's generally good to to separate concerns, so this bug would be primarily about the removing the path nonsense. Getting rid of elementslib often needs other rewriting and it's easier to do all cases at once.

Comment 10

[Magnus Melin [:mkmelin]]

(In reply to Henry Wilkes [:henry] from comment #2)

I was going to make this change:

Looks about right.

has the right kind of informational balance (not too verbose, but still descriptive)?

Yes, much more readable than what we had.

Comment 11

[Lasana Murray]

Hey Henry, as I mentioned before I have pending patches that change CalendarTestUtils that may conflict with your changes. I'll probably pull out those changes into a separate bug to make resolving things easier. If you like, you can ping me with a preview of your patch to be sure. Sorry about that!

Comment 12

[Henry Wilkes (they/them) [:henry-x]]

Attachment: Bug 1694631 - Stop using Lookup in CalendarUtils. r=darktrojan

Comment 13

[Henry Wilkes (they/them) [:henry-x]]

(In reply to Lasana Murray from comment #11)

Hey Henry, as I mentioned before I have pending patches that change CalendarTestUtils that may conflict with your changes. I'll probably pull out those changes into a separate bug to make resolving things easier. If you like, you can ping me with a preview of your patch to be sure. Sorry about that!

OK. I don't mind resolving the conflicts on a rebase if need be. I'm not sure how much longer this will take me.

For now, see the phabricator link (https://phabricator.services.mozilla.com/differential/changeset/?ref=3658312) for my current changes to CalendarTestUtils.jsm. Most of it was adding the dayView, weekView and multiweekView submodules to replace the CalendaryUtils.lookupEventBox and waitForElementNotPresent methods. I only made small changes to the existing monthView.waitForItemAt and monthView.getDayBox methods (the only big functional change is that you can also specify week=6, since a few months need it).

Comment 14

[Henry Wilkes (they/them) [:henry-x]]

Regarding the Lookup class from elementslib.jsm. After the changes to the calendar tests, the only place Lookup is being used is in mail/test/browser/content-tabs/browser_installXpi.js on a single line, which can be replaced by getElementById.

Should I remove the Lookup class and edit browser_installXpi.js as part of this bug fix? Its a small change in terms of edits, but would it be considered too broad?

Comment 15

[Magnus Melin [:mkmelin]]

You could do it in a separate patch (but still in this same bug) I think.

Comment 16

[Henry Wilkes (they/them) [:henry-x]]

Attachment: Bug 1694631 - Drop the Lookup class. r=darktrojan

It was only used in one test.

Depends on D107005

Comment 17

[Geoff Lankow (:darktrojan)]

Removing the checkin-needed tag until the conflict between this and bug 1696913 is resolved.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/25d7207b2438
Stop using Lookup in CalendarUtils. r=darktrojan
https://hg.mozilla.org/comm-central/rev/841ff39242ca
Drop the Lookup class. r=darktrojan