Closed Bug 1694631 Opened 3 months ago Closed 2 months ago

calendar tests should not look up elements by path

Categories

(Calendar :: General, task)

Tracking

(thunderbird_esr78 wontfix)

RESOLVED FIXED
88 Branch
Tracking Status
thunderbird_esr78 --- wontfix

People

(Reporter: mkmelin, Assigned: henry)

References

Details

Attachments

(2 files)

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

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

(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)?

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!

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

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.

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.

(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);

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.

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.

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

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!

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

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?

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

Attachment #9206501 - Attachment description: Bug 1694631 - Remove Lookup → Bug 1694631 - Stop using Lookup in CalendarUtils
Attachment #9206501 - Attachment description: Bug 1694631 - Stop using Lookup in CalendarUtils → Bug 1694631 - Stop using Lookup in CalendarUtils. r=darktrojan

It was only used in one test.

Depends on D107005

Status: NEW → ASSIGNED

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

Blocks: 1697848

Pushed by geoff@darktrojan.net:
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

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