Closed
Bug 570706
Opened 14 years ago
Closed 14 years ago
--browser-chrome Mochitests on Fennec [post navigation]
Categories
(Firefox for Android Graveyard :: General, defect)
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">
Reporter | ||
Comment 1•14 years ago
|
||
You might be able to add these tests to browser_navigation.js
Assignee | ||
Comment 2•14 years ago
|
||
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
Assignee | ||
Comment 3•14 years ago
|
||
Comment on attachment 457733 [details] [diff] [review] pach Also it's been added to browser_navigation.js
Comment 4•14 years ago
|
||
: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
Assignee | ||
Comment 5•14 years ago
|
||
Attachment #457733 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Attachment #457739 -
Flags: review?(21)
Comment 6•14 years ago
|
||
(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?
Assignee | ||
Comment 7•14 years ago
|
||
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 8•14 years ago
|
||
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-
Comment 9•14 years ago
|
||
to add a file to your patch do a: hg add <filename>
Comment 10•14 years ago
|
||
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.
Comment 11•14 years ago
|
||
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.
Assignee | ||
Comment 12•14 years ago
|
||
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
Assignee | ||
Updated•14 years ago
|
Attachment #460871 -
Flags: review?(21)
Assignee | ||
Comment 13•14 years ago
|
||
Attachment #460871 -
Attachment is obsolete: true
Attachment #460881 -
Flags: review?(21)
Attachment #460871 -
Flags: review?(21)
Comment 14•14 years ago
|
||
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-
Assignee | ||
Comment 15•14 years ago
|
||
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
Assignee | ||
Comment 16•14 years ago
|
||
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
Assignee | ||
Comment 17•14 years ago
|
||
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)
Comment 18•14 years ago
|
||
The patch is based on an old version of the test could you please update it?
Assignee | ||
Comment 19•14 years ago
|
||
Attachment #472376 -
Attachment is obsolete: true
Attachment #472400 -
Flags: review?(21)
Attachment #472376 -
Flags: review?(21)
Comment 20•14 years ago
|
||
(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!
Depends on: 594426
Comment 21•14 years ago
|
||
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)
Comment 22•14 years ago
|
||
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.
Assignee | ||
Comment 23•14 years ago
|
||
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)
Comment 24•14 years ago
|
||
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: 14 years ago
Resolution: --- → FIXED
Comment 25•14 years ago
|
||
And obviously thanks for the patch :)
Comment 26•14 years ago
|
||
I've forgot to push the tests files on the previous commit. http://hg.mozilla.org/mobile-browser/rev/81ce9f4adee2
Attachment #473148 -
Flags: review?(21)
Comment 27•14 years ago
|
||
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.
Reporter | ||
Comment 28•14 years ago
|
||
(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.
Description
•