Closed Bug 1328219 Opened 7 years ago Closed 7 years ago

[DateTimePicker] Add browser chrome test for date picker

Categories

(Core :: Layout: Form Controls, defect, P1)

Product:
Core
Component:
Layout: Form Controls
Version:
54 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: scottwu, Assigned: scottwu)

References

(Blocks 1 open bug)

Details

(Whiteboard: [milestone5])

Attachments

(2 files, 2 obsolete files)

Bug 1328219 - (Part 1) Add a PickerReady event for testing and check if picker exists before updating.
59 bytes, text/x-review-board-request
mconley
: review+
Details
Bug 1328219 - (Part 2) Add browser chrome test for date picker.
59 bytes, text/x-review-board-request
mconley
: review+
Details 
      No description provided.
Whiteboard: [milestone5]
Component: XUL Widgets → Layout: Form Controls
Product: Toolkit → Core
Version: unspecified → 54 Branch
Priority: -- → P1
Attachment #8824026 - Attachment is obsolete: true
Attachment #8830618 - Attachment is obsolete: true
I hit a road block writing the tests. The tests that involve clicking on elements are failing on Windows, but I've waited for all the events I could think of. When the event `PickerReady` is fired, the calendar elements should be ready, but I could be wrong.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=75a5695ffa0be207ebeab99a949b55bcea1f8383
The tests would pass if I add a 300ms delay before clicking on elements:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7b16ac279dece5af904713667b6adcf6f35c654
Comment on attachment 8888712 [details]
Bug 1328219 - (Part 1) Add a PickerReady event for testing and check if picker exists before updating.

https://reviewboard.mozilla.org/r/159742/#review171738

Thanks!
Attachment #8888712 - Flags: review?(mconley) → review+
Comment on attachment 8893627 [details]
Bug 1328219 - (Part 2) Add browser chrome test for date picker.

https://reviewboard.mozilla.org/r/164714/#review171740

Thanks for the great tests, scottwu!

The only thing I'm really concerned about are those timeouts. Generally speaking, introducing timeouts like that in tests is a great way to introduce non-determinism / intermittent failures (either now, or down the road). We should try to figure out what we're waiting for and perhaps move the PickerReady event there instead.

::: toolkit/content/tests/browser/browser_datetime_datepicker.js:15
(Diff revision 5)
> +
> +const MONTH_YEAR = ".month-year",
> +      DAYS_VIEW = ".days-view",
> +      BTN_PREV_MONTH = ".prev",
> +      BTN_NEXT_MONTH = ".next";
> +const dateFormat = new Intl.DateTimeFormat("en-US", { year: "numeric", month: "long", timeZone: "UTC" }).format;

Nit: if we're going to have consts be in ALL_CAPS, we should probably do that everywhere. Perhaps this should be DATE_FORMAT.

::: toolkit/content/tests/browser/browser_datetime_datepicker.js:18
(Diff revision 5)
> +const w = "weekend",
> +      o = "outside",
> +      s = "selection",
> +      r = "out-of-range",
> +      t = "today",
> +      p = "off-step";

W, O, S, R, T, P

I guess

::: toolkit/content/tests/browser/browser_datetime_datepicker.js:106
(Diff revision 5)
> +add_task(async function test_datepicker_prev_month_btn() {
> +  const inputValue = "2016-12-15";
> +  const prevMonth = "2016-11-01";
> +
> +  await helper.openPicker(`data:text/html, <input type="date" value="${inputValue}">`);
> +  await new Promise(resolve => setTimeout(resolve, 300));

Timeouts like this are a "solution of last resort" to me. We should probably figure out why this is necessary. Is something occurring after PickerReady that we need to wait for?

::: toolkit/content/tests/browser/browser_datetime_datepicker.js:165
(Diff revision 5)
> +  let inputChangePromise = new Promise(resolve => {
> +    content.document.querySelector("input").addEventListener("input", resolve, {once: true});
> +  });

Should use a ContentTask for this in order to avoid CPOW usage (which you'll get if Mochitest attempts to access contentDocument / contentWindow synchronously).

something like:

```js
let inputChangePromise = ContentTask.spawn(helper.tab.linkedBrowser, (), async function() {
  let inputEl = content.document.querySelector("input");
  await ContentUtils.waitForEvent(inputEl, "input");
});
```

::: toolkit/content/tests/browser/common/datetimePickerHelper.js:1
(Diff revision 5)
> +/* This Source Code Form is subject to the terms of the Mozilla Public

This seems like something you can probably put into head.js pretty safely, instead of introducing its own separate file. That is, unless you're planning on putting more tests that use this helper outside of this directory?

::: toolkit/content/tests/browser/common/datetimePickerHelper.js:7
(Diff revision 5)
> +function DateTimeTestHelper() {
> +  this.panel = document.getElementById("DateTimePickerPanel");
> +}
> +
> +DateTimeTestHelper.prototype = {

If you're feeling fancy, you can also use:

```js

class DateTimeTestHelper {
  constructor() {
    this.panel = ...
    this.tab = null;
    this.frame = null;
  }
  
  async openPicker...
  
}
```
```

::: toolkit/content/tests/browser/common/datetimePickerHelper.js:14
(Diff revision 5)
> +}
> +
> +DateTimeTestHelper.prototype = {
> +  tab: null,
> +  frame: null,
> +  async openPicker(pageUrl) {

Would you mind quickly adding some header block comments to these functions briefly describing what they do?

::: toolkit/content/tests/browser/common/datetimePickerHelper.js:27
(Diff revision 5)
> +    await BrowserTestUtils.waitForEvent(this.frame, "load", true);
> +    // Wait for picker elements to be ready
> +    await BrowserTestUtils.waitForEvent(this.frame.contentDocument, "PickerReady");
> +  },
> +  closeTab() {
> +    return BrowserTestUtils.removeTab(this.tab);

We should probably drop the reference to the tab here as well.

::: toolkit/content/tests/browser/common/datetimePickerHelper.js:49
(Diff revision 5)
> +      await pickerClosePromise;
> +    }
> +    return this.closeTab();
> +  },
> +  removeFrame() {
> +    this.frame.remove();

Should we also drop the reference to the frame here as well?
Attachment #8893627 - Flags: review?(mconley) → review-
Comment on attachment 8893627 [details]
Bug 1328219 - (Part 2) Add browser chrome test for date picker.

https://reviewboard.mozilla.org/r/164714/#review171740

> Timeouts like this are a "solution of last resort" to me. We should probably figure out why this is necessary. Is something occurring after PickerReady that we need to wait for?

I think I've found the problem. The panel opens with animation and sometimes click events happen before the transition ends. Tests should pass now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d41daf614830e6bb9f9327e65d17da746833ed2

> Should use a ContentTask for this in order to avoid CPOW usage (which you'll get if Mochitest attempts to access contentDocument / contentWindow synchronously).
> 
> something like:
> 
> ```js
> let inputChangePromise = ContentTask.spawn(helper.tab.linkedBrowser, (), async function() {
>   let inputEl = content.document.querySelector("input");
>   await ContentUtils.waitForEvent(inputEl, "input");
> });
> ```

Ok I'll make the change. Thanks.

> This seems like something you can probably put into head.js pretty safely, instead of introducing its own separate file. That is, unless you're planning on putting more tests that use this helper outside of this directory?

Ok I'll move it to head.js. There will be tests that could reuse this but will be in this directory as well.

> We should probably drop the reference to the tab here as well.

Ok. Also moved the removeTab call into tearDown since it's the only place it's been used.

> Should we also drop the reference to the frame here as well?

Got it.
Comment on attachment 8893627 [details]
Bug 1328219 - (Part 2) Add browser chrome test for date picker.

https://reviewboard.mozilla.org/r/164714/#review172454

Glad you found the transition problem! I have another idea of how we could side-step it, but I'd be fine landing it like this as well. Thanks for the tests!

::: toolkit/content/tests/browser/head.js:154
(Diff revision 6)
> +    this.frame = this.panel.dateTimePopupFrame;
> +    await BrowserTestUtils.waitForEvent(this.frame, "load", true);
> +    // Wait for picker elements to be ready and open panel transition to end
> +    await Promise.all([
> +      BrowserTestUtils.waitForEvent(this.frame.contentDocument, "PickerReady"),
> +      BrowserTestUtils.waitForEvent(this.panel, "transitionend"),

Alternatively, when opening the picker, we could set the `animate` attribute on the panel node to false:

http://searchfox.org/mozilla-central/rev/c329d562fb6c6218bdb79290faaf015467ef89e2/toolkit/content/xul.css#454-461

Maybe it makes sense to do that in the constructor, and then revert back to the original value in the cleanup function.
Attachment #8893627 - Flags: review?(mconley) → review+
Comment on attachment 8893627 [details]
Bug 1328219 - (Part 2) Add browser chrome test for date picker.

https://reviewboard.mozilla.org/r/164714/#review172454

> Alternatively, when opening the picker, we could set the `animate` attribute on the panel node to false:
> 
> http://searchfox.org/mozilla-central/rev/c329d562fb6c6218bdb79290faaf015467ef89e2/toolkit/content/xul.css#454-461
> 
> Maybe it makes sense to do that in the constructor, and then revert back to the original value in the cleanup function.

That's a good idea. I'll do it this way.
Looks fine on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ad2e47b101a9788e1e02bb65c1c038180040d0f
Checking it in. Thanks Mike!
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/506e362a6098
(Part 1) Add a PickerReady event for testing and check if picker exists before updating. r=mconley
https://hg.mozilla.org/integration/autoland/rev/ee15ce194655
(Part 2) Add browser chrome test for date picker. r=mconley
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: