1109140 - Create a remote Mozmill test for "What is this page?" links in "about:newtab"

Status: RESOLVED WONTFIX

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Teodor Druta]

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

1. Open Firefox using a clean profile
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
4. Click on "What is this page"
5. Visit both links from the pop-up message
  >>  "The ""Learn more..."" link will lead to Click on the gear (preferences) button located in the top right corner of the page. ""Privacy notice"" will open https://www.mozilla.org/en-US/privacy/ in the same tab."

The new test should be a mozmill remote test.

Comment 1

[Teodor Druta]

Attachment: testnewtabv1.2.patch

This test will open "about:newtab", click on the "gear" element, checking if its functionality is correct, then will open the "What is this page?" panel by clicking the associated link. From the panel it will go through "Learn more..." and "Privacy Notice", checking if the links open the correct pages.

Comment 2

[Mihaela Velimiroviciu (:mihaelav)]

Comment on attachment 8534324 [details] [diff] [review]
testnewtabv1.2.patch

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

This patch is v1 :)

The name of the test should be more suggestive for what the test does, testNewTab doesn't say much about what we test there.

As discussed with people from manual desktop team, step 3 is redundant for this test, so please remove that code from the test (lines 40-60)

::: firefox/tests/remote/testNewTab/testNewTab.js
@@ +16,5 @@
> +
> +function setupModule(aModule) {
> +  aModule.bw = new browser.BrowserWindow();
> +  // Closing all tabs should open about:newtab
> +  // bw.tabs.closeAllTabs();

Please remove these 2 commented lines

@@ +104,5 @@
> +
> +/**
> + * Opens the "What is this page?" panel
> + */
> +function openWISTP() {

nit: There is an extra "S" in the name if this function. 
(You can also leave the entire name as it's not that long (openWhatIsThisPagePanel) or find something else more appropriate without using abbreviations)

@@ +106,5 @@
> + * Opens the "What is this page?" panel
> + */
> +function openWISTP() {
> +  // Click on "What is this page?" element
> +  var wistp = findElement.ID(bw.controller.window.document, "newtab-intro-what");

Nit: extra "s" in the variable name

Comment 3

[Teodor Druta]

Attachment: testintro_v1.4.patch

Removed the relevant code for step 3 :(

Rename the test file to testIntro.js, removed unnecessary comments, renamed openWISTP function to openWhatIsThisPage.

Comment 4

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8535599 [details] [diff] [review]
testintro_v1.4.patch

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

::: firefox/tests/remote/testNewTab/testIntro.js
@@ +16,5 @@
> +  "https://www.mozilla.org/.*/privacy/"
> +];
> +
> +function setupModule(aModule) {
> +  aModule.bw = new browser.BrowserWindow();

browserWindow please, as we use it in other tests.

@@ +77,5 @@
> + *         Current opened page url
> + */
> +function getURL() {
> +  var URL = bw.controller.tabs.activeTabWindow.location.href;
> +  return URL;

Don't think we need this, we have in toolbars.locationbar a similar method.

@@ +91,5 @@
> +
> +  // Check if panel is open
> +  var panel = findElement.ID(bw.controller.window.document, "newtab-intro-panel");
> +  assert.waitFor(() => !!panel.getNode().getAttribute("panelopen"),
> +                 "'What is this page?' panel is open");

This fails on OSX:
ERROR | Test Failure | {
  "exception": {
    "message": "'What is this page?' panel is open", 
    "lineNumber": 27, 
    "name": "TimeoutError", 
    "fileName": "resource://mozmill/modules/errors.js"
  }
}

Comment 5

[Teodor Druta]

Attachment: testintro_v2.patch

(In reply to Andreea Matei [:AndreeaMatei] from comment #4)
> Comment on attachment 8535599 [details] [diff] [review]
> testintro_v1.4.patch
> 
> Review of attachment 8535599 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: firefox/tests/remote/testNewTab/testIntro.js
> @@ +16,5 @@
> > +  "https://www.mozilla.org/.*/privacy/"
> > +];
> > +
> > +function setupModule(aModule) {
> > +  aModule.bw = new browser.BrowserWindow();
> 
> browserWindow please, as we use it in other tests.
> 

Updated to browserWindow everywhere in the test

> @@ +77,5 @@
> > + *         Current opened page url
> > + */
> > +function getURL() {
> > +  var URL = bw.controller.tabs.activeTabWindow.location.href;
> > +  return URL;
> 
> Don't think we need this, we have in toolbars.locationbar a similar method.
> 

Actually the locationbar value method returns the value typed in the location bar, we can't use it here to test against about:newtab. And also it will work slower than getting the location directly from the window.

> @@ +91,5 @@
> > +
> > +  // Check if panel is open
> > +  var panel = findElement.ID(bw.controller.window.document, "newtab-intro-panel");
> > +  assert.waitFor(() => !!panel.getNode().getAttribute("panelopen"),
> > +                 "'What is this page?' panel is open");
> 
> This fails on OSX:
> ERROR | Test Failure | {
>   "exception": {
>     "message": "'What is this page?' panel is open", 
>     "lineNumber": 27, 
>     "name": "TimeoutError", 
>     "fileName": "resource://mozmill/modules/errors.js"
>   }
> }

It seems that the code for the about:newtab page has been modified in Nightly, I don't think there is a proper way to handle if the 'What is this page?' panel is open, we should just handle the failure further in the code.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Teodor Druta from comment #5)
> It seems that the code for the about:newtab page has been modified in
> Nightly, I don't think there is a proper way to handle if the 'What is this
> page?' panel is open, we should just handle the failure further in the code.

Then request to get it fixed if it is barely hard to get a state of an element.

Comment 7

[Teodor Druta]

Attachment: testintro_v3.patch

Actually this was the regressor https://hg.mozilla.org/mozilla-central/rev/396b4f8be85b
I don't think that there is something wrong with the nightly code.
It's just that retrieving a hidden element node NEW attribute always returns an empty string.
Anyway, it's more correct to use an event listener in this case.

Comment 8

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8538420 [details] [diff] [review]
testintro_v3.patch

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

::: firefox/tests/remote/testNewTab/testIntro.js
@@ +45,5 @@
> +  var learnMore = findElement.Selector(browserWindow.controller.window.document,
> +                                       "#newtab-intro-panel > p:first-of-type a");
> +  learnMore.waitThenClick();
> +
> +  browserWindow.controller.waitForPageLoad();

Fails on OSX with waitForPageLoad(). So the click failed, I see the panel open but nothing else happens on it.

Comment 9

[Teodor Druta]

It seems like there are some differences in how the intro panel elements are handled on different platforms.

Comment 10

[Teodor Druta]

Attachment: testintro_v4.patch

We need to wait for changes in the DOM tree.

Comment 11

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8538567 [details] [diff] [review]
testintro_v4.patch

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

Henrik, can you please give us an input for the test too? We're close to finish this.

::: firefox/tests/remote/testNewTab/testIntro.js
@@ +74,5 @@
> + *         Current opened page url
> + */
> +function getURL() {
> +  var URL = browserWindow.controller.tabs.activeTabWindow.location.href;
> +  return URL;

Lets move this inside a library though, maybe tabs or use the base-in-content lib method for get url?

@@ +92,5 @@
> +  try {
> +    // Click on "What is this page?" element
> +    var link = findElement.ID(browserWindow.controller.window.document, "newtab-intro-what");
> +    link.waitThenClick();
> +    assert.waitFor(() => (flagPopupShow && flagDomTree), "'What is this page?' popup is open");

Please move the message below.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8538567 [details] [diff] [review]
testintro_v4.patch

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

f+ functionality wise even the implementation needs a bit of tweaking.

::: firefox/tests/remote/manifest.ini
@@ +5,5 @@
>  [include:testAddons/manifest.ini]
>  [include:testDownloading/manifest.ini]
>  [include:testGeolocation/manifest.ini]
>  [include:testLayout/manifest.ini]
> +[include:testNewTab/manifest.ini]

This is still tabbedbrowsing. I don't see a reason why to start a new top-level group.

::: firefox/tests/remote/testNewTab/testIntro.js
@@ +26,5 @@
> +
> +/**
> + * Tests functionality of "What is this page?" in about:newtab page
> + */
> +function testIntro() {

Be more specific in the testname or the method name in that to test. There is no need to duplicate names, lets make it more obvious instead.

@@ +33,5 @@
> +  browserWindow.controller.waitForPageLoad();
> +
> +  // Check if "about:newtab" is open
> +  assert.waitFor(() => getURL() === TEST_DATA[0],
> +                 "'" + TEST_DATA[0] + "' is open");

We never check if the page has been the right URL. Why is that necessary here?

@@ +45,5 @@
> +  browserWindow.controller.waitForPageLoad();
> +
> +  // Check if "Learn more..." link opened the correct url
> +  assert.waitFor(() => !!getURL().match(TEST_DATA[1]),
> +                 "'Learn more...' link opened the correct page, expected: " + TEST_DATA[1]);

please use utils.assertLoadedUrlEqual() to check for locations.

@@ +63,5 @@
> +  browserWindow.controller.waitForPageLoad();
> +
> +  // Check if Privacy Notice opened the correct url
> +  assert.waitFor(() => !!getURL().match(TEST_DATA[2]),
> +                 "'Privacy Notice' link opened the correct page, expected: " + TEST_DATA[2]);

Mostly the same code twice. Make it data driven so we can safe lots of those lines.

@@ +70,5 @@
> +/**
> + * Returns current page URL
> + *
> + * @return {string} URL
> + *         Current opened page url

This reads strange.

@@ +74,5 @@
> + *         Current opened page url
> + */
> +function getURL() {
> +  var URL = browserWindow.controller.tabs.activeTabWindow.location.href;
> +  return URL;

This is the locationbar so it should be a property of it, if we really need that. Not sure how tabs or even base-in-content should be related here. :)

@@ +97,5 @@
> +  }
> +  finally {
> +    browserWindow.controller.window.removeEventListener("popupshowing", onPopupShow);
> +    browserWindow.controller.window.removeEventListener("DOMAttrModified", onDOMTree);
> +  }

What you want here is a new in-content page class, and move all in there. This should not be part of any test.

Comment 13

[Teodor Druta]

Attachment: testintro.patch

This is the updates test using the new AboutNewtab page class, so it needs the patch from Bug 1117745 to be applied first.

Comment 14

[Mihaela Velimiroviciu (:mihaelav)]

Comment on attachment 8558538 [details] [diff] [review]
testintro.patch

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

The test contains duplicate code (open about:newtab, open "what is this page" panel, click on link, wait for page load, close page) which can be gathered under a loop, so it will be easier to read.
There are also some other things you missed from Henrik's review, please address them:

(In reply to Henrik Skupin (:whimboo) from comment #12)
> ::: firefox/tests/remote/manifest.ini
> @@ +5,5 @@
> >  [include:testAddons/manifest.ini]
> >  [include:testDownloading/manifest.ini]
> >  [include:testGeolocation/manifest.ini]
> >  [include:testLayout/manifest.ini]
> > +[include:testNewTab/manifest.ini]
> 
> This is still tabbedbrowsing. I don't see a reason why to start a new
> top-level group.

 
> ::: firefox/tests/remote/testNewTab/testIntro.js
> @@ +26,5 @@
> > +
> > +/**
> > + * Tests functionality of "What is this page?" in about:newtab page
> > + */
> > +function testIntro() {
> 
> Be more specific in the testname or the method name in that to test. There
> is no need to duplicate names, lets make it more obvious instead.

> @@ +45,5 @@
> > +  browserWindow.controller.waitForPageLoad();
> > +
> > +  // Check if "Learn more..." link opened the correct url
> > +  assert.waitFor(() => !!getURL().match(TEST_DATA[1]),
> > +                 "'Learn more...' link opened the correct page, expected: " + TEST_DATA[1]);
> 
> please use utils.assertLoadedUrlEqual() to check for locations.
 
> @@ +63,5 @@
> > +  browserWindow.controller.waitForPageLoad();
> > +
> > +  // Check if Privacy Notice opened the correct url
> > +  assert.waitFor(() => !!getURL().match(TEST_DATA[2]),
> > +                 "'Privacy Notice' link opened the correct page, expected: " + TEST_DATA[2]);
> 
> Mostly the same code twice. Make it data driven so we can safe lots of those
> lines.
 
> @@ +74,5 @@
> > + *         Current opened page url
> > + */
> > +function getURL() {
> > +  var URL = browserWindow.controller.tabs.activeTabWindow.location.href;
> > +  return URL;
> 
> This is the locationbar so it should be a property of it, if we really need
> that. Not sure how tabs or even base-in-content should be related here. :)

::: firefox/tests/remote/testNewTab/testIntro.js
@@ +41,5 @@
> +  });
> +
> +  // Check if "Learn more..." link opened the correct firefox tiles URL
> +  assert.waitFor(() => !!getURL().match(TEST_DATA[0]),
> +                 "'Learn more...' link opened the correct page, expected: " + TEST_DATA[1]);

I think that should be TEST_DATA[0]

@@ +56,5 @@
> +  });
> +
> +  // Check if "Privacy Notice" link opened the correct privacy URL
> +  assert.waitFor(() => !!getURL().match(TEST_DATA[1]),
> +                 "'Privacy Notice' link opened the correct page, expected: " + TEST_DATA[2]);

There is no TEST_DATA[2], I think that it should be TEST_DATA[1]

@@ +63,5 @@
> +/**
> + * Returns current page URL
> + *
> + * @return {string} URL
> + *         Current opened page URL

"The URL of the opened page"

Comment 15

[Teodor Druta]

Attachment: testnewtabintro_new.patch

I refactored a bit the test, addressed all the nits.

Comment 16

[Mihaela Velimiroviciu (:mihaelav)]

Comment on attachment 8561355 [details] [diff] [review]
testnewtabintro_new.patch

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

It looks better now, r=me with the nit addressed.

::: firefox/tests/remote/testLayout/testNewTabIntro.js
@@ +49,5 @@
> +      link.waitThenClick();
> +      browserWindow.controller.waitForPageLoad();
> +    });
> +    assert.waitFor(() => !!locationBar.value.match(aData.URL),
> +                 "'" + aData.linkName + "' link opened the correct page, expected: " + aData.URL);

nit: This line is much longer than 80 chars.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8561355 [details] [diff] [review]
testnewtabintro_new.patch

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

::: firefox/tests/remote/testLayout/manifest.ini
@@ +1,4 @@
>  [parent:../manifest.ini]
>  
>  [testNavigateFTP.js]
> +[testNewTabIntro.js]

Why is this test being put into layout? It is a totally different component. Also for me this test is more a unit test as something we have to put into the remote testrun.

::: firefox/tests/remote/testLayout/testNewTabIntro.js
@@ +49,5 @@
> +      link.waitThenClick();
> +      browserWindow.controller.waitForPageLoad();
> +    });
> +    assert.waitFor(() => !!locationBar.value.match(aData.URL),
> +                 "'" + aData.linkName + "' link opened the correct page, expected: " + aData.URL);

It's even useless to pass in the link name because it will never have the value you expect and only gives wrong information. It should totally be ripped off here.

Comment 18

[Teodor Druta]

(In reply to Henrik Skupin (:whimboo) from comment #17)
> Also for me this test is more a unit test as something we have to put into
> the remote testrun.

This is a testcase requested by the QA team. We should have this in production.

> Why is this test being put into layout?
This seemed as the best component for this test, because it tests the layout of the 'about:newtab' page.
If you do not agree then we should make a new component => 'testAboutNewtab'.

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This test doesn't cover the layout for the page but checks that the links are doing the right thing. Layout tests are reserved for tests which cover remote web pages only.

So please put this into a new subfolder and call it as you proposed.

Comment 20

[Teodor Druta]

Attachment: testnewtabintro_new_v2.patch

Moved the test into a new remote sub-folder.

Comment 21

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8561984 [details] [diff] [review]
testnewtabintro_new_v2.patch

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

::: firefox/tests/remote/testAboutNewtab/testNewTabIntro.js
@@ +50,5 @@
> +      browserWindow.controller.waitForPageLoad();
> +    });
> +    assert.waitFor(() => !!locationBar.value.match(aData.URL),
> +                 "'" + aData.linkName + "' link opened the correct page, got: "
> +                 + "'" + locationBar.value + "'");

This should be shifted to the right a bit and also we're not using "got/expected" values in the waitFors as they won't be accurate, i think Henrik also mentioned this above.

Comment 22

[Teodor Druta]

Attachment: testnewtabintro_new_v2.1.patch

Sorry, I missed that out. Henrik, can you please give a review ?

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8562790 [details] [diff] [review]
testnewtabintro_new_v2.1.patch

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

::: firefox/tests/remote/testAboutNewtab/testNewTabIntro.js
@@ +32,5 @@
> +}
> +
> +function teardownModule(aModule) {
> +  aModule.browserWindow.tabs.closeAllTabs();
> +  windows.closeAllWindows(aModule.browserWindow);

I don't see that any code is opening a new window. So why we need that?

@@ +49,5 @@
> +      link.waitThenClick();
> +      browserWindow.controller.waitForPageLoad();
> +    });
> +    assert.waitFor(() => !!locationBar.value.match(aData.URL),
> +                   "Correct page has been opened");

I do not understand this assert. Shouldn't it be located after waitForPageLoad() inside the callback? At this point the tab which showed the target url is already closed. Does that not fail for you?

Comment 24

[Teodor Druta]

(In reply to Henrik Skupin (:whimboo) from comment #23)
> I do not understand this assert. Shouldn't it be located after
> waitForPageLoad() inside the callback? At this point the tab which showed
> the target url is already closed. Does that not fail for you?

actually aboutNewTab.close(), won't necessarily close the tab it will close the aboutNewTab page, so in this case it opens another remote page in the same tab.

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The logic of this test is too hard to understand, and I think that you do not need the aboutNewTab.close() call at all. Reason is that we automatically navigate away from that page. We should not start to involve opening and closing of an extra tab due to how close() is implemented.

Comment 26

[Teodor Druta]

(In reply to Henrik Skupin (:whimboo) from comment #25)
> We should not start to involve
> opening and closing of an extra tab due to how close() is implemented.

But we actually don't.

Consider this scenario:
We have an AboutNewtab class instance, let's call it aboutNewtab object.
We open a new about:newtab page using aboutNewTab.open(), using the default menu entry, which will open a new tab. At this moment let's say that we have two tabs the first one is something, anything this is irrelevant, the second tab is the 'about:newtab' page opened with the aboutNewTab object, this tab is the activeTab. We want to open another page in the same tab, ok we open it, at this moment there are 1 aboutNewTab opened pages, when there are actually none, this surely is not the desired behaviour here, so it's normal that we need to close the aboutNewTab first before opening another page.

Comment 27

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Teodor Druta from comment #26)
> is the activeTab. We want to open another page in the same tab, ok we open
> it, at this moment there are 1 aboutNewTab opened pages, when there are
> actually none, this surely is not the desired behaviour here, so it's normal
> that we need to close the aboutNewTab first before opening another page.

Right and I tested this locally! aboutNewTab.isOpen() should not assume that it is open when another URL has been loaded meanwhile. This is a bug in the AboutNewtabPage class. Something what this class is not doing is to define its url, so isOpen() used .tabs() which simply returns the current href.

So what this test should do is:
1. open an about:newtab page (which opens a new tab)
2. open the what is that panel and click the link
3. the current tab will show the remote page
4. Do steps 1-3 again

In all those cases there is no need to close the about:newtab page, which in this state even doesn't exist anymore.

Checking other in-content classes, all of them seem to be affected by that isOpen() bug.

Comment 28

[Teodor Druta]

Then we need to investigate BICP isOpen() method ASAP, I highly suspect that actually there is a problem with the getTabsWithURL() function from the tabs lib and the in content pages urls. Somehow this function may be failing in returning the correct tabs for the in-content urls.

Comment 29

[Henrik Skupin [:whimboo][⌚️UTC+1]]

No, the problem is here... see the stack:

http://hg.mozilla.org/qa/mozmill-tests/file/195befdcc229/firefox/lib/ui/base-in-content-page.js#l61
http://hg.mozilla.org/qa/mozmill-tests/file/195befdcc229/firefox/lib/ui/base-in-content-page.js#l72
http://hg.mozilla.org/qa/mozmill-tests/file/195befdcc229/firefox/lib/ui/base-in-content-page.js#l85

We are going to find the tab by the currently loaded URL. This may is fine for the abstract class but not for any other specific in-content class. this.url has to return 'about:newtab' etc.

Comment 30

[Sylvestre Ledru [:Sylvestre]]

Mozmill is dead, WONTFIX the remaining bugs.