1110677 - Add automated test for "Change newtab page appearance"

Status: RESOLVED FIXED

(Firefox :: New Tab Page, defect)

Description

[Teodor Druta]

https://moztrap.mozilla.org/manage/case/14576/

1. Open Firefox
2. Go to about:newtab or open a new tab
3. Click on the gear (preferences) button located in the top right corner of the page
  >> "You are able to change between 3 modes: Enhanced, Classic and Blank. Also, you will have a fourth option, which is ""What is this page?"""

The new test should be chrome, and should live under:
https://github.com/mozilla/gecko-dev/tree/master/browser/base/content/test/newtab

Comment 1

[Teodor Druta]

Tim, can you please provide your input on this ? Will this test be helpful ?

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

I thought we have a test for this? Ed, what of this is covered already?

Comment 3

[Ed Lee :Mardak]

There's https://github.com/mozilla/gecko-dev/blob/master/browser/base/content/test/newtab/browser_newtab_enhanced.js that tests if the tiles are showing history vs enhanced tiles depending on the enhanced/classic state.

I don't think there's a test for the click event checking to make sure the menu options switches prefs correctly.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

We could cover that by instead of toggling |NewTabUtils.allPages.enhanced| actually opening the popup and calling .click() on the desired menu item. That would just be updating the existing test instead of writing a new one.

Comment 5

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v1.0

I created an helper method which opens newtab customization panel, changes the option and closes the panel, the I replaced the direct configuration changes with that method in test itself.

With this patch we will have full coverage for the moztrap
https://moztrap.mozilla.org/manage/case/14576/

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8540651 [details] [diff] [review]
patch v1.0

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

This looks great, just a few comments we should address. Thanks!

::: browser/base/content/test/newtab/browser_newtab_enhanced.js
@@ +75,5 @@
>    is(title, "site#-1");
>  
>    is(getData(1), null, "directory link still pushed out by pinned history link");
> +
> +  ok(!!getContentDocument().getElementById("newtab-intro-what"),

Nit: the !! isn't needed, we'll cast it to a bool anyway.

::: browser/base/content/test/newtab/head.js
@@ +690,5 @@
> + *
> + * @param {string} aOption
> + *        Option to be set("blank"|"classic"|"enhanced")
> + */
> +function customizeNewTabPage(aOption) {

"aOption" sound like we would accept options. Should rename it to maybe "aTheme" or something? We can surely find a better name.

@@ +691,5 @@
> + * @param {string} aOption
> + *        Option to be set("blank"|"classic"|"enhanced")
> + */
> +function customizeNewTabPage(aOption) {
> +  let deferred = new Promise.defer();

Should use the new Promise() API, without any Deferreds.

@@ +701,5 @@
> +    panel.removeEventListener("popupshown", onShown);
> +
> +    // Get the element for the specific option and click on it,
> +    // then trigger an escape to close the panel
> +    document.getElementById("newtab-customize-"+aOption).click();

Nit: I'd move that to a variable under "let panel = ...".

@@ +702,5 @@
> +
> +    // Get the element for the specific option and click on it,
> +    // then trigger an escape to close the panel
> +    document.getElementById("newtab-customize-"+aOption).click();
> +    EventUtils.synthesizeKey("VK_ESCAPE", {});

EventUtils have focus issues sometimes, can't we just call panel.hidePopup()? Maybe off executeSoon() because we're inside an event handler.

Comment 7

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v2.0

(In reply to Tim Taubert [:ttaubert] from comment #6)
> EventUtils have focus issues sometimes, can't we just call
> panel.hidePopup()? Maybe off executeSoon() because we're inside an event
> handler.
I don't think we use executeSoon in this test, maybe there is something I don't understand here.

Comment 8

[Ed Lee :Mardak]

(In reply to Cosmin Malutan from comment #7)
> (In reply to Tim Taubert [:ttaubert] from comment #6)
> > EventUtils have focus issues sometimes, can't we just call
> > panel.hidePopup()? Maybe off executeSoon() because we're inside an event
> > handler.
> I don't think we use executeSoon in this test, maybe there is something I
> don't understand here.
The concern is that synthesizing ESC might send it to the wrong place, i.e., not the panel, so it would end up never dismissing. We're not testing the functionality of hiding the panel, so it would be good enough to just call hidePopup() to get rid of it. We might need an executeSoon() to delay the hiding of the popup just momentarily to allow the currently triggering popup event to complete before changing state of the popup.

Comment 9

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v2.1

(In reply to Ed Lee :Mardak from comment #8)
> The concern is that synthesizing ESC might send it to the wrong place, i.e.,
> not the panel, so it would end up never dismissing. We're not testing the
> functionality of hiding the panel, so it would be good enough to just call
> hidePopup() to get rid of it. We might need an executeSoon() to delay the
> hiding of the popup just momentarily to allow the currently triggering popup
> event to complete before changing state of the popup.
Makes sense, thanks for explanation.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8547402 [details] [diff] [review]
patch v2.1

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

::: browser/base/content/test/newtab/head.js
@@ +685,5 @@
>    return deferred.promise;
>  }
> +
> +/**
> + * Change the newtab customization option and waits for panel to open and close

Nit: "Changes..." and "... for the panel to..."

@@ +688,5 @@
> +/**
> + * Change the newtab customization option and waits for panel to open and close
> + *
> + * @param {string} aTheme
> + *        Option to be set("blank"|"classic"|"enhanced")

Nit: Maybe s/Option to be/Can be any of/?

@@ +702,5 @@
> +
> +    // Get the element for the specific option and click on it,
> +    // then trigger an escape to close the panel
> +    document.getElementById("newtab-customize-"+aTheme).click();
> +    panel.hidePopup();

executeSoon(() => panel.hidePopup());

We should wait for the event to be fully processed before closing the popup again.

@@ +708,5 @@
> +
> +  let promise = new Promise((resolve) => {
> +    // Attache the listener for panel closing, this will resolve the promise
> +    panel.addEventListener("popuphidden", function onShown() {
> +      panel.removeEventListener("popuphidden", onShown);

The function name should be "onHidden".

@@ +709,5 @@
> +  let promise = new Promise((resolve) => {
> +    // Attache the listener for panel closing, this will resolve the promise
> +    panel.addEventListener("popuphidden", function onShown() {
> +      panel.removeEventListener("popuphidden", onShown);
> +      resolve();

Instead of using a Promise at all, we should do:

executeSoon(TestRunner.next);

That effectively does the same but is more straightforward.

Comment 11

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v3.0

(In reply to Tim Taubert [:ttaubert] from comment #10)
> > +    // then trigger an escape to close the panel
> > +    document.getElementById("newtab-customize-"+aTheme).click();
> > +    panel.hidePopup();
> 
> executeSoon(() => panel.hidePopup());
> 
> We should wait for the event to be fully processed before closing the popup
> again.
I thought I did that, but it seems I forget to refresh before exporting the patch, thanks.

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8547507 [details] [diff] [review]
patch v3.0

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

Great, thanks! Can you please push it to try before requesting checkin-needed?

Comment 13

[Cosmin Malutan, [:cosmin-malutan]]

Result 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f33fbd5dec25

Comment 14

[Cosmin Malutan, [:cosmin-malutan]]

There was some failures on one build but on tests that has nothing to do with the test I enhanced.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/e5b52cac8853

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e5b52cac8853