575986 - [mozmill] Timeout failure in testAccessLocationBar.js

Status: RESOLVED WORKSFORME

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Aaron Train [:aaronmt]]

MODULE: firefox\testAwesomeBar\testAccessLocationBar.js     
TEST: testAccessLocationBar
FAIL: timeout exceeded for waitForElement XPath: /html/body/div[@id='outer-wrapper']/div[@id='inner-wrapper']/div[@id='nav']/h1/a/img
BRANCH: 1.9.1 *

* Couldn't find the image,  a fix to take this test offline so it wont get stuck on finding an image with local pages can be patched across all branches.

Comment 1

[Aaron Train [:aaronmt]]

Attachment: Patch v1 - Takes test offline

This patch takes this test offline by loading local test files. 

In particular I am reusing the local HTML file in test-files\tabbedbrowsing. I open three different local web pages by passing in a different id. I also open an about:blank like the previous test. I scan for the last opened page, which would have a value (openinnewtab_target.html?id=3), and I do a comparison check against the contents of the URL (formerly 'getpersonas') and the contents of the page (formerly an image) and now the contents of a <span>.

Comment 2

[u279076]

Comment on attachment 455165 [details] [diff] [review]
Patch v1 - Takes test offline

>-const websites = ['http://www.google.com/',
>-                  'http://www.mozilla.org/',
>-                  'http://www.getpersonas.com/',
>-                  'about:blank'];
>+const localTestFolder = collector.addHttpResource('../test-files/');
Please use ALL_CAPS for constants (i.e. LOCAL_TEST_FOLDER)

>-  for each (website in websites) {
>-    locationBar.loadURL(website);
>+  for(var i = 0; i < 3; i++){
>+    locationBar.loadURL(localTestFolder + 
>+                   'tabbedbrowsing/openinnewtab_target.html?id=' + (i+1));
Why is locationBar.loadURL() used here instead of controller.open()?  Henrik, please advise when we should use controller.open() vs locationBar.loadURL().

>   // checks that the first item in the drop down list is selected.
>   controller.waitForEval("subject.selectedIndex == 0",
>-                         gTimeout, 100, locationBar.autoCompleteResults);
>-  locationBar.contains("getpersonas");
>+                         TIMEOUT, 100, locationBar.autoCompleteResults);
>+  locationBar.contains("openinnewtab_target.html?id=3");
We should probably wrap all locationBar.contains() in a waitForEval().

r- from me based on the above.  Aaron, please wait for Henrik's review before proceeding with these revisions.

Comment 3

[Aaron Train [:aaronmt]]

(In reply to comment #2)
> >-  for each (website in websites) {
> >-    locationBar.loadURL(website);
> >+  for(var i = 0; i < 3; i++){
> >+    locationBar.loadURL(localTestFolder + 
> >+                   'tabbedbrowsing/openinnewtab_target.html?id=' + (i+1));
> Why is locationBar.loadURL() used here instead of controller.open()?  Henrik,
> please advise when we should use controller.open() vs locationBar.loadURL().
> 

As per conforming with the original test and manual litmus instructions.

Comment 4

[u279076]

(In reply to comment #3)
> (In reply to comment #2)
> > >-  for each (website in websites) {
> > >-    locationBar.loadURL(website);
> > >+  for(var i = 0; i < 3; i++){
> > >+    locationBar.loadURL(localTestFolder + 
> > >+                   'tabbedbrowsing/openinnewtab_target.html?id=' + (i+1));
> > Why is locationBar.loadURL() used here instead of controller.open()?  Henrik,
> > please advise when we should use controller.open() vs locationBar.loadURL().
> > 
> 
> As per conforming with the original test and manual litmus instructions.
My point is, why was this used in the original test. From what I can tell, there is not much difference between controller.open() and locationBar.loadURL().  Henrik, please advise.

Comment 5

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

Comment on attachment 455165 [details] [diff] [review]
Patch v1 - Takes test offline

No need to ask for review when there are still open questions. I will check bugs in any way.

Comment 6

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

(In reply to comment #4)
> > As per conforming with the original test and manual litmus instructions.
> My point is, why was this used in the original test. From what I can tell,
> there is not much difference between controller.open() and
> locationBar.loadURL().  Henrik, please advise.

There is a big difference. controller.open doesn't use keypress or type to enter the url into the locationbar. It uses the backend function to open pages so we do not conflict when the navigation bar isn't shown. So keep using loadURL

Comment 7

[Aaron Train [:aaronmt]]

Thanks. As per locationBar.contains, the function was written in particular for this test, let's just use it. If that's alright, I'll put up a patch with the constant variable to caps.

Comment 8

[u279076]

(In reply to comment #7)
> Thanks. As per locationBar.contains, the function was written in particular for
> this test, let's just use it. If that's alright, I'll put up a patch with the
> constant variable to caps.

My concern is that locationBar.contains() does not have any timeout associated with it.  Employing a waitForEval either within contains() or around contains() would make this test a lot safer.

Comment 9

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

Why do we have to use contains when we have local test data. There will be no redirect and we can directly compare the values.

Comment 10

[Aaron Train [:aaronmt]]

(In reply to comment #9)
> Why do we have to use contains when we have local test data. There will be no
> redirect and we can directly compare the values.
Unless I'm mistaken, contains() would be certainly be wiser to use than comparing against a unique for local data, i.e, 

http://localhost:43336/73cdf57b445a-434c-wtab_target.html?id=2/openinne83ac-17e9f8a7b92a/tabbedbrowsing/...

Comment 11

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

Use something like addHttpResource('../test-files/', '') to add an empty namespace for now. It will stop httpd to use a GUID. Also you will type in the url for the target url. So it should be exactly the same when we test the auto-complete.

Comment 12

[Aaron Train [:aaronmt]]

Attachment: V1.1 - Added checks

+ Added locationBar url checks against the target URL
+ Renamed constant to caps

Comment 13

[u279076]

Comment on attachment 455237 [details] [diff] [review]
V1.1 - Added checks

>+const LOCAL_TEST_FOLDER = collector.addHttpResource('../test-files/', '');
Why the extra bits at the end?  
addHttpResource('../test-files/'); should suffice.

>+  for(var i = 0; i < 3; i++){
>+    locationBar.loadURL(LOCAL_TEST_FOLDER + 
>+                   'tabbedbrowsing/openinnewtab_target.html?id=' + (i+1));
We could/should use another constant here:
const TEST_SITE = LOCAL_TEST_FOLDER + 'tabbedbrowsing/openinnewtab_target.html?id=';
 ...
locationBar.loadURL(TEST_SITE + (i+1));

>+  var lastLoadedURL = "http://localhost:43336//tabbedbrowsing/openinnewtab_target.html?id=3";
We can use the above constant here too:
lastLoadedURL = TEST_SITE + '3';

An alternative to all of this would be to just declare TEST_SITE as an array:
const TEST_SITE = {
  LOCAL_TEST_FOLDER + 'tabbedbrowsing/openinnewtab_target.html?id=1',
  LOCAL_TEST_FOLDER + 'tabbedbrowsing/openinnewtab_target.html?id=2',
  LOCAL_TEST_FOLDER + 'tabbedbrowsing/openinnewtab_target.html?id=3'
}

Then in any for-loop you can just call TEST_SITE[i] and lastLoadedURL = TEST_SITE.length; This makes the test a lot more robust.
  
>+  // Finally - Check for the presenece of the last loaded page
>+  // by checking for the presence of the ID element's innerHTML
>+  controller.waitForElement(new elementslib.ID(controller.window.document, "id"), TIMEOUT, 100);
Please move the elementslib.ID into a variable.

Comment 14

[Aaron Train [:aaronmt]]

Attachment: V1.2 - Added checks

(In reply to comment #13)
> (From update of attachment 455237 [details] [diff] [review])
> >+const LOCAL_TEST_FOLDER = collector.addHttpResource('../test-files/', '');
> Why the extra bits at the end?  
> addHttpResource('../test-files/'); should suffice.

See Henrik's comment. The second parameter specifies a namespace. Without it, we are using the global namespace that of which includes a global unique identifier appended to our local test url. By specifying an empty string, I get a clean url as can be seen in LAST_LOADED_URL.

> An alternative to all of this would be to just declare TEST_SITE as an array:
> const TEST_SITE = {
>   LOCAL_TEST_FOLDER + 'tabbedbrowsing/openinnewtab_target.html?id=1',
>   LOCAL_TEST_FOLDER + 'tabbedbrowsing/openinnewtab_target.html?id=2',
>   LOCAL_TEST_FOLDER + 'tabbedbrowsing/openinnewtab_target.html?id=3'
> }
> 
> Then in any for-loop you can just call TEST_SITE[i] and lastLoadedURL =
> TEST_SITE.length; This makes the test a lot more robust.

Makes sense. Done.

> >+  // Finally - Check for the presenece of the last loaded page
> >+  // by checking for the presence of the ID element's innerHTML
> >+  controller.waitForElement(new elementslib.ID(controller.window.document, "id"), TIMEOUT, 100);
> Please move the elementslib.ID into a variable.

Done.

Comment 15

[u279076]

Comment on attachment 455293 [details] [diff] [review]
V1.2 - Added checks

>+const LAST_LOADED_URL = 'http://localhost:43336//tabbedbrowsing/openinnewtab_target.html?id=3';
> 
Please use LOCAL_TEST_FOLDER + 'tabbedbrowsing/openinnewtab_target.html?id=3'; if you can.

Comment 16

[Aaron Train [:aaronmt]]

Attachment: v.1.3 - Added checks

(In reply to comment #15)
> (From update of attachment 455293 [details] [diff] [review])
> >+const LAST_LOADED_URL = 'http://localhost:43336//tabbedbrowsing/openinnewtab_target.html?id=3';
> > 
> Please use LOCAL_TEST_FOLDER + 'tabbedbrowsing/openinnewtab_target.html?id=3';
> if you can.

Done.

Comment 17

[u279076]

Comment on attachment 455318 [details] [diff] [review]
v.1.3 - Added checks

r+ for me -- over to Henrik.

Comment 18

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

Comment on attachment 455318 [details] [diff] [review]
v.1.3 - Added checks

Sorry for being late here. This review request somehow got lost.

>+const LAST_LOADED_URL = LOCAL_TEST_FOLDER + 'tabbedbrowsing/openinnewtab_target.html?id=3';
[...] 
>+  // Check that the last loaded page is in the url bar
>+  controller.assertValue(locationBar.urlbar, LAST_LOADED_URL);

Please don't use a constant here. The last loaded URL you will get from the for loop. Also you do not have to store the complete URL, the index will be enough.

>+  // Finally - Check for the presenece of the last loaded page

nit: please fix that spelling error

>+  // by checking for the presence of the ID element's innerHTML
>+  var linkId = new elementslib.ID(controller.window.document, "id");

We check the textContent property and not innerHTML. Simply say that we check its content

>+  controller.waitForElement(linkId, TIMEOUT, 100);
>+  controller.assertText(linkId, "3")

No hard-coded value please.

Comment 19

[Aaron Train [:aaronmt]]

Attachment: V1.4 - Fixes

(In reply to comment #18)
> Comment on attachment 455318 [details] [diff] [review]
> >+const LAST_LOADED_URL = LOCAL_TEST_FOLDER + 'tabbedbrowsing/openinnewtab_target.html?id=3';
> [...] 
> >+  // Check that the last loaded page is in the url bar
> >+  controller.assertValue(locationBar.urlbar, LAST_LOADED_URL);
> 
> Please don't use a constant here. The last loaded URL you will get from the for
> loop. Also you do not have to store the complete URL, the index will be enough.

Done. Added url and id for each website in the array.

> >+  // Finally - Check for the presenece of the last loaded page
> 
> nit: please fix that spelling error

Done.

> >+  // by checking for the presence of the ID element's innerHTML
> >+  var linkId = new elementslib.ID(controller.window.document, "id");
> 
> We check the textContent property and not innerHTML. Simply say that we check
> its content

Done.

> >+  controller.waitForElement(linkId, TIMEOUT, 100);
> >+  controller.assertText(linkId, "3")
> 
> No hard-coded value please.

Done. I use the id of the website in the array.

Comment 20

[Aaron Train [:aaronmt]]

Forgot to mention that this was written for 1.9.2 and 1.9.1, currently on trunk there appears to be an awesomebar bug where you can't use the arrow keys i.e., up and down arrow to open the menu

Comment 21

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

Comment on attachment 458228 [details] [diff] [review]
V1.4 - Fixes

>+const WEBSITES = [
>+  {url: LOCAL_TEST_FOLDER + 'tabbedbrowsing/openinnewtab_target.html?id=1', id: 1},
>+  {url: LOCAL_TEST_FOLDER + 'tabbedbrowsing/openinnewtab_target.html?id=2', id: 2},
>+  {url: LOCAL_TEST_FOLDER + 'tabbedbrowsing/openinnewtab_target.html?id=3', id: 3},
>+  {url: 'about:blank'}
>+];

Well, as already said in my last review, there is no need to duplicate all that code with the full URL. Simply create an array with possible id and define the URL once (ending with  "?id=").

>+  for each (website in WEBSITES) {
>+    locationBar.loadURL(website.url);
>     controller.waitForPageLoad();

So that could be "WEB_PAGE + i". I would also suggest to start with id=0 for consistency to other tests and the real tab index.

>+  // Check that the last loaded page is in the url bar
>+  controller.assertValue(locationBar.urlbar, WEBSITES[2].url);

Make that flexible by appending the last used id from the loop above.

>+  // Finally - Check for the presence of the last loaded page
>+  // by checking for the presence of the page's content
>+  var linkId = new elementslib.ID(controller.window.document, "id");
>+  controller.waitForElement(linkId, TIMEOUT, 100);
>+  controller.assertText(linkId, WEBSITES[2].id)
>+  UtilsAPI.assertLoadedUrlEqual(controller, WEBSITES[2].url);

Same here. We really shouldn't be forced to update tests if the amount of tabs get changed.

Comment 22

[Aaron Train [:aaronmt]]

Attachment: V1.5 - More fixes

More fixes

Comment 23

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

Comment on attachment 459003 [details] [diff] [review]
V1.5 - More fixes

>-const gTimeout = 5000;
>-const gDelay = 100;
>+const TIMEOUT = 5000;

>+const LOCAL_TEST_FOLDER = collector.addHttpResource('../test-files/', '');
> 
>-const websites = ['http://www.google.com/',
>-                  'http://www.mozilla.org/',
>-                  'http://www.getpersonas.com/',
>-                  'about:blank'];
>+const LOCAL_WEBSITE = LOCAL_TEST_FOLDER + 'tabbedbrowsing/openinnewtab_target.html?id=';

Nit: please separate the httpd lines from the other global constants with an empty line and put both together. Also it should be named LOCAL_PAGE instead of LOCAL_WEBSITE.

>+  var id;

The declaration should be in-front of the comment at the top of the function.

>   locationBar.clear();
> 
>   // Second - Arrow down to open the autocomplete list (displays most recent visit first),
>-  // then arrow down again to the first entry, in this case www.getpersonas.com;
>+  // then arrow down again to the first entry, in this case tabbedbrowsing/openinnewtab_target.html?id=0
>   controller.keypress(locationBar.urlbar, "VK_DOWN", {});
>-  controller.sleep(gDelay);
>+  controller.sleep(100);
>   controller.keypress(locationBar.urlbar, "VK_DOWN", {});
>-  controller.sleep(gDelay);
>+  controller.sleep(100);

Here the test fails for me. After calling clear() and pressing the down key, no autocomplete popup opens for me with the latest Minefield build on OS X. I have to enter at least one single character to make it work. It looks like that localhost pages are handled differently. Does it really work for you?

>+  
>+  // Finally - Check for the presence of the last loaded page
>+  // by checking for the presence of the page's content
>+  var linkId = new elementslib.ID(controller.window.document, "id");

Well, you want to check the id within the loaded page, right? Then it should be controller.tabs.activeTab.

Comment 24

[Aaron Train [:aaronmt]]

Shall I post an updated patch, and you can mark as skip for default? I spoke to sdwilsh and he doesn't know anything about it, but if anything suspects it as a widget issue

Comment 25

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

I would like to wait until we have an idea what's causing bug 581469. A clean solution for our test would be preferred.

Comment 26

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

Mass move of Mozmill Test related project bugs to newly created components. You can filter out those emails by using "Mozmill-Tests-to-MozillaQA" as criteria.

Comment 27

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

Any update on that bug Aaron?

Comment 28

[Aaron Train [:aaronmt]]

This test was taken offline a while back for all branches and now does not timeout from online content. Not seeing any reports in last couple months.

Comment 29

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

What do you mean with taken offline? At least on nightly and aurora this test is enabled.

Comment 30

[Aaron Train [:aaronmt]]

(In reply to comment #29)
> What do you mean with taken offline? At least on nightly and aurora this
> test is enabled.

Simply mean that it's using data from local pages now