Closed Bug 570706 Opened 10 years ago Closed 9 years ago

--browser-chrome Mochitests on Fennec [post navigation]

Categories

(Firefox for Android Graveyard :: General, defect)

Fennec 1.1
x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mfinkle, Assigned: adifire)

References

Details

Attachments

(1 file, 7 obsolete files)

After loading a page in Fennec, we should test that the title and favicon are correctly set. Maybe some other things too.

* Check that the URL is displayed if the page has not <title>
* Check that the title is displayed if the page has a <title>
* Check that the title is updated if the <title> is changed after pageload
* Check that the favicon is defaulted correctly if the page has no <link rel="icon">
* Check that the favicon is correct if the page has a <link rel="icon">
You might be able to add these tests to browser_navigation.js
Attached patch pach (obsolete) — Splinter Review
All cases needed are reproducible. Few problems are
* Cannot use waitFor when there is no title for a page
* window.messageManager.addMessageListener does not work
Comment on attachment 457733 [details] [diff] [review]
pach

Also it's been added to browser_navigation.js
:adifire, this is great and super fast!  We had chatted about adding a library function to get the element id values (similar to the preferences stuff.)

odd that the window.messageManager stuff isn't working.  Glad you figure out the window.addEventListener though.  This would imply that the test case is not running in e10s (or without remote=true)  This might be something we need to look into and revisit later.  Maybe mfinkle can help us debug this a bit more.

Also, I don't see where the urls are defined in this patch:
+  run: function() {
+    this._tab = Browser.addTab(testURL_04, true);
+
+    // Wait for the tab to load, then do the test
+    waitFor(gCurrentTest.onPageReady, pageLoaded(testURL_04));
+  },

I assume testURL_04 points to english_title.html.



:adifire, have 21@vingtetun.org review this when you update it some more.  Overall, I think this covers the tests we want, but I would like to see one of the top front end guys make sure we are calling the right api's and getting coverage they care about.
Assignee: nobody → adicoolrao
Attached patch Added URLs (obsolete) — Splinter Review
Attachment #457733 - Attachment is obsolete: true
Attachment #457739 - Flags: review?(21)
(In reply to comment #2)
> Created attachment 457733 [details] [diff] [review]
> pach
> 
> All cases needed are reproducible. Few problems are
> * Cannot use waitFor when there is no title for a page
> * window.messageManager.addMessageListener does not work

What is the error when you use messageManager? Can you elaborate more on it?
When I use the messageManager, it does not wait for the page to load. Basically I use them to wait for those pages that have no titles ( I cannot use waitFor for this case ).

With window.addEventListener, it's able to wait for the page to be loaded.
Comment on attachment 457739 [details] [diff] [review]
Added URLs

>diff -r 0b40df211227 chrome/tests/browser_navigation.js
>--- a/chrome/tests/browser_navigation.js	Thu Jun 03 18:55:46 2010 -0400
>+++ b/chrome/tests/browser_navigation.js	Fri Jul 16 07:28:03 2010 +0530
>@@ -1,5 +1,8 @@
> var testURL_01 = "chrome://mochikit/content/browser/mobile/chrome/browser_blank_01.html";
> var testURL_02 = "chrome://mochikit/content/browser/mobile/chrome/browser_blank_02.html";
>+var testURL_03 = "chrome://mochikit/content/browser/mobile/chrome/no_title.html";
>+var testURL_04 = "chrome://mochikit/content/browser/mobile/chrome/english_title.html";
>+var pngURL = "chrome://mochikit/content/browser/mobile/chrome/bugzilla.png";
> 

These pages are not added into the Makefile.in and are missing when I try to run the tests

>+  onFocusReady: function() {
>+    window.removeEventListener("popupshown", gCurrentTest.onFocusReady, false);
>+    
>+    let go = document.getElementById("tool-go");
>+    EventUtils.synthesizeString(testURL_02, window);
>+    EventUtils.synthesizeMouse(go, go.clientWidth / 2, go.clientHeight / 2, {});
>+    window.addEventListener("pageshow", gCurrentTest.onPageFinish,false);
>+//    waitFor(gCurrentTest.onPageFinish, pageLoaded(testURL_02));
>+  },

Remove the commented line

Code seems good excepts that you need to add spaces between the arguments of the function (a space after a comma).

r- because the tests failed with a timeout on my laptop on some waitFor (I assume this is not your fault but I'll be happy if this can be fixed :) ) and because of the missing line into Makefile.in

TEST-INFO | chrome://mochikit/content/browser/testing/mochitest/tests/browser/browser_navigation.js | Console message: [JavaScript Error: "uncaught exception: waitFor timeout"]
Attachment #457739 - Flags: review?(21) → review-
to add a file to your patch do a: hg add <filename>
one thing I noticed while running this patch locally is waitFor defaults to 1000 ms, and adjusting this to a higher value (I used 10000 as a much higher first test) it worked.  I think that there was a lag in the time for browsing back.  I saw in the pagLoading() function that the url would change, but then isLoading() was true and we would fail.  

I suspect a smaller value of 3141 ms would be a lot more reasonable.  Remember this code will be running on small phones.  At least we can adjust this in 1 spot.
I think all the window.addEventListener("pageshow", ...) can be replace with messageManager.addMessageListener("pageshow", this).

I've use messageManager into bug 581139 for tests and it works very well.
Attached patch Patch update (obsolete) — Splinter Review
I couldn't get it working in my system because the bookmarks page doesn't load once the focus is on the urlbar. Otherwise this patch should work.
Attachment #457739 - Attachment is obsolete: true
Attachment #460871 - Flags: review?(21)
Attached patch Update (obsolete) — Splinter Review
Attachment #460871 - Attachment is obsolete: true
Attachment #460881 - Flags: review?(21)
Attachment #460871 - Flags: review?(21)
Comment on attachment 460881 [details] [diff] [review]
Update

>diff -r b0ebeecc4cda chrome/tests/browser_navigation.js
>--- a/chrome/tests/browser_navigation.js	Wed Jul 28 07:58:50 2010 -0700
>+++ b/chrome/tests/browser_navigation.js	Wed Jul 28 20:55:53 2010 +0530
>@@ -1,5 +1,8 @@
> var testURL_01 = "chrome://mochikit/content/browser/mobile/chrome/browser_blank_01.html";
> var testURL_02 = "chrome://mochikit/content/browser/mobile/chrome/browser_blank_02.html";
>+var testURL_03 = "chrome://mochikit/content/browser/mobile/chrome/no_title.html";
>+var testURL_04 = "chrome://mochikit/content/browser/mobile/chrome/english_title.html";

nit: put them in the order they are used in the file and update the relevant places

var testURL_03 = "chrome://mochikit/content/browser/mobile/chrome/english_title.html";
var testURL_04 = "chrome://mochikit/content/browser/mobile/chrome/no_title.html";

>+//------------------------------------------------------------------------------
>+// Bug 570706 - --browser-chrome Mochitests on Fennec [post navigation]  
>+
>+// Check for text in the url bar for no title, with title and title change after pageload
>+gTests.push({
>+  desc: "Check for text in the url bar for no title, with title and title change after pageload",
>+  _tab: null,
>+  
>+  run: function() {
>+    this._tab = Browser.addTab(testURL_04, true);
>+
>+    // Wait for the tab to load, then do the test
>+    waitFor(gCurrentTest.onPageReady, pageLoaded(testURL_04));
>+  },

I've run into some timeouts with some of the waitFor of this tests, could you please use jmaher trick:

messageManager.addMessageListener("pageshow", function() {
  if (gCurrentTest._tab.browser.currentURI.spec != "about:blank") {
    messageManager.removeMessageListener("pageshow", arguments.callee);
    gCurrentTest.onPageReady();
  }
}); 

It works very well.

>+
>+  onPageReady: function() {
>+    let urlbarEdit = document.getElementById("urlbar-edit");
>+    is(urlbarEdit.value, "English Title Page", "The title must be displayed in urlbar");
>+
>+    Browser.closeTab(gCurrentTest._tab);
>+    this._tab = Browser.addTab(testURL_03, true);

Since this is in a message callback this._tab should be gCurrentTest._tab;

nit: Also I would like to see _currentTab instead of _tab it is clearer to me

>+    
>+    // Wait for pageload, fails when messageManager is used because of no title in the page
>+    window.addEventListener("pageshow",  gCurrentTest.onPageReady2, false);
>+  },

same

>+  
>+  onPageReady2: function(){
>+  	window.removeEventListener("pageshow", gCurrentTest.onPageReady2, false);

If you use the was suggested you could removed this line

>+    let urlbarEdit = document.getElementById("urlbar-edit");
>+    is(urlbarEdit.value, testURL_03, "The url for no title must be displayed in urlbar");
>+    
>+    Browser.closeTab(gCurrentTest._tab);
>+    
>+    // Check whether title appears after a pageload
>+    this._tab = Browser.addTab(testURL_01, true);

Use gCurrentTest._tab;

>+    waitFor(gCurrentTest.onPageReady3, pageLoaded(testURL_01));
>+  },
>+    
>+  onPageReady3: function(){
>+    let urlbarEdit = document.getElementById("urlbar-edit");
>+    todo_is(urlbarEdit.value, "Browser Blank Page 01", "The title of the first page must be displayed");

you could probably use a "is" instead of "todo_is", with the previous changes, it worked for me

>+    EventUtils.synthesizeMouse(urlbarEdit, urlbarEdit.clientWidth / 2, urlbarEdit.clientHeight / 2, {});
>+
>+    // Wait for the awesomebar to load, then do the test
>+    window.addEventListener("popupshown", gCurrentTest.onFocusReady, false);
>+  },
>+  
>+  onFocusReady: function() {
>+    window.removeEventListener("popupshown", gCurrentTest.onFocusReady, false);
>+    
>+    let go = document.getElementById("tool-go");
>+    EventUtils.synthesizeString(testURL_02, window);
>+    EventUtils.synthesizeMouse(go, go.clientWidth / 2, go.clientHeight / 2, {});
>+    messageManager.addMessageListener("pageshow", gCurrentTest.onPageFinish);
>+  },

Use it the way describe above

>+
>+  onPageFinish: function() {
>+    messageManager.removeMessageListener("pageshow", gCurrentTest.onPageFinish);  

Remove this line

>+    let urlbarEdit = document.getElementById("urlbar-edit");
>+    todo_is(urlbarEdit.value, "Browser Blank Page 02", "The title of the second page must be displayed");

you could probably use a "is" instead of "todo_is", with the previous changes, it worked for me

>+    ok(true,urlbarEdit.value);   
>+    Browser.closeTab(gCurrentTest._tab);

nit: use _currentTab

>+    runNextTest();
>+  }
>+});
>+
>+// Case: Check for appearance of the fevicon
>+gTests.push({
>+  desc: "Check for appearance of the favicon",
>+  _tab: null,

nit: replace _tab by _currentTab

>+
>+  run: function() {
>+    this._tab = Browser.addTab(testURL_03, true);
>+    messageManager.addMessageListener("pageshow", gCurrentTest.onPageLoad);
>+  },

Use it the way described above.
>+  
>+  onPageLoad: function() {
>+    messageManager.removeMessageListener("pageshow", gCurrentTest.onPageLoad);

remove this line

>+    
>+    let favicon = document.getElementById("urlbar-favicon");
>+    is(favicon.src,"","The default favicon must be loaded");

add spaces between the arguments

>+    
>+    Browser.closeTab(gCurrentTest._tab);
>+    this._tab = Browser.addTab(testURL_04, true);
>+    messageManager.addMessageListener("pageshow", gCurrentTest.onPageReady);
>+  },

Use it the way describe above

>+  
>+  onPageReady: function(){
>+    messageManager.removeMessageListener("pageshow", gCurrentTest.onPageReady);
>+    favicon = document.getElementById("urlbar-favicon");
>+    is(favicon.src,pngURL,"The page favicon must be loaded");

nit: add spaces between the arguments

>+    
>+    Browser.closeTab(gCurrentTest._tab);
>+    runNextTest();
>+  }
>+});
>+
>diff -r b0ebeecc4cda chrome/tests/bugzilla.png
>Binary file chrome/tests/bugzilla.png has changed
>diff -r b0ebeecc4cda chrome/tests/english_title.html
>--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>+++ b/chrome/tests/english_title.html	Wed Jul 28 20:55:53 2010 +0530
>@@ -0,0 +1,6 @@
>+<html>
>+<head><title>English Title Page</title>
>+<link rel="shortcut icon" href="bugzilla.png" />
>+</head>
>+<body>This is english title page</body>
>+</html>
>diff -r b0ebeecc4cda chrome/tests/no_title.html
>--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>+++ b/chrome/tests/no_title.html	Wed Jul 28 20:55:53 2010 +0530
>@@ -0,0 +1,7 @@
>+<html>
>+<head>
>+</head>
>+<body>
>+Hi this is a no title page
>+</body>
>+</html>


It still miss englisht_title.html and no_title.html added to Makefile.in. I think you probably have them living into your build tests dir but they should be added to Makefile.in


Sorry for all the changes but that's the way to make them run in my case. I suggest you remove the closeTab calls for testing otherwise you will probably never be able to run the tests :s
Next time will be the right one!
Attachment #460881 - Flags: review?(21) → review-
Attached patch wip (obsolete) — Splinter Review
changes
_tab to _currentTab
more messageManager instead of waitFor in some places (It fails in my tests though)
replaced todo_is to is

** this hasn't been tested as the tests time out far too frequently.
Attachment #460881 - Attachment is obsolete: true
The awesome bar doesn't load properly during tests. 

* When focused on awesome bar, url is entered, and click on the go button, the page loads and it still remains on the awesomebar autocompletepopup (the one that shows the bookmarks and history). Also the page doesn't load properly, with the title still being the url though the page has title.

* When a page is loaded using addTab and awesomebar is focused, the autocompletepopup won't load fully, especially that the "See all bookmarks" won't be visible. This is seen for the tests that are already in browser_navigation.js
Attached patch Working Patch (obsolete) — Splinter Review
Now this patch seems to be working in my system. 

The messageManager still fails though when used to check the load of the page that has no title.
Attachment #461250 - Attachment is obsolete: true
Attachment #472376 - Flags: review?(21)
The patch is based on an old version of the test could you please update it?
Attached patch Working patch (updated) (obsolete) — Splinter Review
Attachment #472376 - Attachment is obsolete: true
Attachment #472400 - Flags: review?(21)
Attachment #472376 - Flags: review?(21)
(In reply to comment #19)
> Created attachment 472400 [details] [diff] [review]
> Working patch (updated)

Sorry for the time to review it. I've started yesterday but it looks like there is a bug somewhere that prevent the "English Title Page" to finish properly.

The good news is that I would have been unabled to seen it without your tests patch!

I would like to open a new bug and fix it before finishing my review for this one (this way you'll be able to run it till the end)

Thanks for the tests!
Comment on attachment 472400 [details] [diff] [review]
Working patch (updated)

>diff -r aeb8aa5a377f chrome/tests/Makefile.in
>--- a/chrome/tests/Makefile.in	Sun Sep 05 07:23:01 2010 -0700
>+++ b/chrome/tests/Makefile.in	Mon Sep 06 20:28:08 2010 +0530
>@@ -79,6 +79,9 @@
>   browser_viewport_08.html \
>   browser_viewport_09.html \
>   browser_viewport.js \
>+  browser_favicon.png \

As seen on IRC I would like to see the browser_favico.png image embedded as a base64 image instead of a real image.
Also I think using "browser_" in front of the name is a bad idea because mochitest will think this is a test too (maybe this is true only for .js or .html files). Joel?

>diff -r aeb8aa5a377f chrome/tests/browser_navigation.js

> 
> //------------------------------------------------------------------------------
> // Entry point (must be named "test")
>@@ -18,7 +22,7 @@
>   // The "runNextTest" approach is async, so we need to call "waitForExplicitFinish()"
>   // We call "finish()" when the tests are finished
>   waitForExplicitFinish();
>-
>+  

Nit: please ensure you're not adding extra whitespaces in your patch like this line

>     // Wait for the awesomebar to load, then do the test
>     window.addEventListener("popupshown", gCurrentTest.onFocusReady, false);
>   },
>-
>+  

Same as above

>   onFocusReady: function() {
>     window.removeEventListener("popupshown", gCurrentTest.onFocusReady, false);
>     // Test mode
>@@ -150,9 +154,9 @@
>     is(forward.disabled, !gCurrentTest._currentTab.browser.canGoForward, "Forward button check");
> 
>     Browser.closeTab(gCurrentTest._currentTab);
>-
>+    

Same as above

>     runNextTest();
>-  }
>+  }  

And same :)

> });
> 
> //------------------------------------------------------------------------------
>@@ -162,12 +166,12 @@
>   _currentTab: null,
> 
>   run: function() {
>-    this._currentTab = Browser.addTab(testURL_01, true);
>+    gCurrentTest._currentTab = Browser.addTab(testURL_01, true);
> 
>     // Wait for the tab to load, then do the test
>     waitFor(gCurrentTest.onPageReady, pageLoaded(testURL_01));
>   },
>-
>+  

Same as above

>   onPageReady: function() {
>     let urlIcons = document.getElementById("urlbar-icons");
>     is(urlIcons.getAttribute("mode"), "view", "URL Mode is set to 'view'");
>@@ -179,7 +183,7 @@
>     // Wait for the awesomebar to load, then do the test
>     window.addEventListener("popupshown", gCurrentTest.onFocusReady, false);
>   },
>-
>+  

Same as above

>   onFocusReady: function() {
>     window.removeEventListener("popupshown", gCurrentTest.onFocusReady, false);
>     let urlIcons = document.getElementById("urlbar-icons");
>@@ -230,3 +234,121 @@
>     runNextTest();
>   }
> });

>+
>+//------------------------------------------------------------------------------
>+// Bug 570706 - --browser-chrome Mochitests on Fennec [post navigation]  
>+
>+// Check for text in the url bar for no title, with title and title change after pageload
>+gTests.push({
>+  desc: "Check for text in the url bar for no title, with title and title change after pageload",
>+  _currentTab: null,
>+  
>+  run: function() {
>+    gCurrentTest._currentTab = Browser.addTab(testURL_03, true);
>+
>+    // Wait for the tab to load, then do the test
>+    messageManager.addMessageListener("pageshow", function() {
>+      if ( gCurrentTest._currentTab.browser.currentURI.spec == testURL_03 ) {

Remove the extra white space :
>+      if ( gCurrentTest._currentTab.browser.currentURI.spec == testURL_03 ) {

should be:
>+      if (gCurrentTest._currentTab.browser.currentURI.spec == testURL_03) {

>+    
>+    // Wait for pageload, fails when messageManager is used because of no title in the page
>+    window.addEventListener("pageshow",  gCurrentTest.onPageReady2, false);

Can you quickly check that the comment is still true?

>+  },
>+  
>+  onPageReady2: function(){
>+  	window.removeEventListener("pageshow", gCurrentTest.onPageReady2, false);

There is extra whitespaces at the beginning of the line

>+    gCurrentTest._currentTab = Browser.addTab(testURL_01, true);
>+    messageManager.addMessageListener("pageshow", function() {
>+      if ( gCurrentTest._currentTab.browser.currentURI.spec == testURL_01 ) {

Remove the extra whitespaces

>+    messageManager.addMessageListener("pageshow", function() {
>+      if ( gCurrentTest._currentTab.browser.currentURI.spec == testURL_02 ) {

Remove the extra whitespaces

>+  run: function() {
>+    gCurrentTest._currentTab = Browser.addTab(testURL_04, true);
>+    messageManager.addMessageListener("pageshow", function() {
>+      if ( gCurrentTest._currentTab.browser.currentURI.spec == testURL_04 ) {

Remove the extra whitespaces

>+    gCurrentTest._currentTab = Browser.addTab(testURL_03, true);
>+    messageManager.addMessageListener("pageshow", function() {
>+      if ( gCurrentTest._currentTab.browser.currentURI.spec == testURL_03 ) {

Remove the extra whitespace

>+  
>+  onPageFinish: function(){
>+    favicon = document.getElementById("urlbar-favicon");
>+    is(favicon.src, pngURL, "The page favicon must be loaded");

Check for the base64 encoded image here


We are really close to land, just remove all the extra spaces and use the a base64 encoded image instead of adding one to the build system and I'll r+ it :)
Attachment #472400 - Flags: review?(21)
browser_*.js is what we look for in browser-chrome tests.  I would recommend changing this to image_favicon.png or file_favicon.png. That keeps things more logically separated.

:vingtetun, could it be that some of the white space is removing spaces instead of adding them.  I will let :adifire ensure that we have no extra whitespaces.
Removed the whitespaces. In some parts though a line has been removed and added. I guess it's not a problem.

Also it turns out that messageManager can be used to check for loading of pages that has no title.

Added a 64base coded image. ( hope no one minds the image :) )
Attachment #472400 - Attachment is obsolete: true
Attachment #473148 - Flags: review?(21)
The image is perfect for me :)

I've done a few tweaks to the code before landing to remove the extra whitespace staying (I'm a kind of fanatic :)) but also to removes any references to the tool-go button that have gone yesterday (sigh) because of bug 591490

http://hg.mozilla.org/mobile-browser/rev/2a1c6a5d90b1
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
This bug is marked FIXED, but it appears here:

https://wiki.mozilla.org/Mobile/Notes/29-Sep-2010#Automation

saying it needs updating. Is that true? If so, new bugs should be filed on the issues with this patch, and this should remain closed.
(In reply to comment #27)
> This bug is marked FIXED, but it appears here:
> 
> https://wiki.mozilla.org/Mobile/Notes/29-Sep-2010#Automation
> 
> saying it needs updating. Is that true? If so, new bugs should be filed on the
> issues with this patch, and this should remain closed.

I'm not sure what the reference in the mobile notes is referring too. The current tests in browser_navigation.js are passing my local runs.
You need to log in before you can comment on or make changes to this bug.