Closed Bug 835132 Opened 13 years ago Closed 12 years ago

Test failure "Current domain name matches target domain name" in /testSearch/testSearchSelection.js

Categories

(Mozilla QA Graveyard :: Mozmill Tests, defect, P1)

Product:
Mozilla QA Graveyard
Component:
Mozmill Tests
Type:
defect

Tracking

(firefox23 wontfix, firefox24 fixed, firefox25 fixed, firefox26 fixed, firefox-esr17 fixed, firefox-esr24 fixed)

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox23 --- wontfix
firefox24 --- fixed
firefox25 --- fixed
firefox26 --- fixed
firefox-esr17 --- fixed
firefox-esr24 --- fixed

People

(Reporter: daniela.p98911, Assigned: mario.garbi)

References

(
URL
)

Details

(Keywords: reproducible, Whiteboard: [mozmill-test-failure])

Attachments

(4 files, 12 obsolete files)

patch v1.3
10.26 KB, patch
AndreeaMatei
: review+
Details | Diff | Splinter Review
searchEngineFollowup_1009_v3.patch
7.56 KB, patch
andrei
: review+
andrei
: checkin+
Details | Diff | Splinter Review
searchEngine_Beta_v1.patch
11.17 KB, patch
AndreeaMatei
: review+
Details | Diff | Splinter Review
searchEngine_ESR17.patch
12.42 KB, patch
AndreeaMatei
: review+
Details | Diff | Splinter Review
Happened in 1/26 on Nightly FR and Windows 8 x64: http://mozmill-ci.blargon7.com/#/functional/report/72e15dc943833b8fcba70aeb51743d2f And Nightly FR and Windows Vista x86: http://mozmill-ci.blargon7.com/#/functional/report/72e15dc943833b8fcba70aeb51746935 This seems to be related to this line: http://hg.mozilla.org/qa/mozmill-tests/file/d9e0e694cbf2/lib/search.js#l500 where we check if the domain name is correct.
URL: http://mozmill-ci.blargon7.com/#/func...
status-firefox21: --- → affected
OS: Windows 8 → All
Whiteboard: [mozmill-test-failure]
This seems to happen for localized builds only. Sounds like a regression to me. Please get this investigated.
Priority: -- → P1
Whiteboard: [mozmill-test-failure] → [mozmill-test-failure] s=130128 u=failure c=search p=1
Priority: P1 → P2
I was able to reproduce it locally with builds from 1/24 and 1/25 FR on Windows XP http://mozmill-crowd.blargon7.com/#/functional/report/0dbaa964aec88a2f5637c68c961902fb It seems that the issue is related to Bug 833882: - In 1/24 the changes for 833882 were backed out due to bug 834432 - In 1/26, the issue stopped reproducing when the changes for bug 833882 where re-added: Report on CI: http://mozmill-ci.blargon7.com/#/functional/report/72e15dc943833b8fcba70aeb5193657f
Assignee: nobody → dpetrovici
Status: NEW → ASSIGNED
Thanks Daniela! Sounds like we are good then and close this bug as fixed by the follow-up patch on bug 833882.
Blocks: 833882
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
status-firefox21: affected → verified
Resolution: --- → FIXED
Happened again today with an ondemand_functional job on Linux Ubuntu 12.10 (x86_64) with Firefox 21.0 zh-TW: View the build in Jenkins: http://mm-ci-master.qa.scl3.mozilla.com:8080/job/ondemand_functional/123/ View the results in the Mozmill Dashboard: http://mozmill-ondemand.blargon7.com/#/functional/report/ea82256a8ae9808d91b7e8145e068ee2 http://mozmill-ondemand.blargon7.com/#/functional/report/ea82256a8ae9808d91b7e8145e07bba0
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Please investigate this immediately. This issue should not persist for Firefox 21 and could be still a bug which hasn't been fixed with the patch on bug 833882. CC'ing Anthony and Juan, so that both are aware of the issue.
This also reproduces locally on Ubuntu 12.04 http://mozmill-crowd.blargon7.com/#/functional/report/ea82256a8ae9808d91b7e8145e17856a
(In reply to mario garbi from comment #7) > This also reproduces locally on Ubuntu 12.04 > http://mozmill-crowd.blargon7.com/#/functional/report/ > ea82256a8ae9808d91b7e8145e17856a And what are the results from your testing? That's something I'm interested in.
I am still investigating this, but I've noticed that we are first sent to tw.rd.yahoo.com/referurl/knowledge/fp/header/search_button/*http://tw.knowledge.yahoo.com/search/search_result?p=Building&mc=0 and from there redirected towards http://tw.knowledge.yahoo.com/search/search_result?p=Building&mc=0 Not sure if this is they way it went before or if it's something new, I will also look into this, but it would seem that the way our regex works is by taking the second element after "."
What do we actually check in this test? If we don't care about a redirect we have to do so with utils.assertURLEqual().
It would seem we never really fixed this issue with zh-TW locale, as seen in the ondemand_functional reports: http://mozmill-ondemand.blargon7.com/#/functional/reports?branch=All&platform=All&from=2013-02-01&to=2013-05-08
We check if the correct target URL has been opened for the search in checkSearchResultsPage method from search.js library. It seems to work ok for other locales or en-US.
To add to comment 12, we compare the URL of the currently selected search engine that we retrieved with "Services.search.currentEngine.getSubmission(searchTerm, null).uri" and the actual URL after search. For zh-TW they are not the same as tw.rd.yahoo.com doesn't seem to do anything else than redirect towards tw.yahoo.com and the search ui element takes us to tw.knowledge.yahoo.com. Could this be an issue with the Services.search and zh-TW locale?
You can check that manually in such a build. So you will see how it works outside of automation. If Yahoo is the default engine and they do such a redirect nothing seems wrong to me to that behavior.
The problem here is that Services.search returns a value (tw.rd.yahoo.com) and the search UI takes us to a different URL (tw.knowledge.yahoo.com). I will look into why this happenes this afternoon, it's going slowly due to laguage barriers in documentation and UI.
Steps to manually reproduce: 1.Open local page "mozilla_mission.html" in a Release 21 zh-TW build 2.Select the last search engine option from the UI search element (Yahoo) 3.Righ click the text element "goal" and select Search option from the context menu 4.Check the URL bar for the first url 5.Check the search page URL that we land on after a quick auto redirect Expected results: -They should be the same URL Actual results: -The first URL at step 4 is (tw.rd.yahoo.com) -The final URL at step 5 is (tw.knowledge.yahoo.com) We could come up with a fix patch that checks the locale and selects another search engine (it works fine with the others).
No, we don't want to go with workarounds. Those will never work. Please see my comment 10 how we should fix it for real.
Indeed "utils.assertLoadedUrlEqual" works perfect for the zh-TW and JA builds, but fails for en-US because here we have an ID in the url (Ebay) that changes every time we are redirected. rover.ebay.com/rover/1/711-47294-18009-3/4?mpre=http://shop.ebay.com/?_nkw=Building Could I use a conditional If to determine if the build is zh-TW and handle it separately since it's the only one that fails?
Where does the search engine URL point to? It looks like we can only compare the domain name then which should be always the same. We don't want to do any workaround regarding conditional statements as I already explained above.
Assignee: dpetrovici → mario.garbi
Comment on attachment 749232 [details] [diff] [review] Patch v1.0 - Release Branch Review of attachment 749232 [details] [diff] [review]: ----------------------------------------------------------------- We would need to start with nightly as I assume this will fail on any branch with those locales.
Attachment #749232 - Flags: review?(andreea.matei) → review-
Comment on attachment 749232 [details] [diff] [review] Patch v1.0 - Release Branch The same patch applies to all branches as shown in the reports above (Nightly builds reports). I have requested review again because the patch was rejected due to the fact that it was made for Release and since we didn't had to change the code for other branches I didn't thought a new patch was necessarily. If we need a new patch for Nightly I will upload one as soon as possible
Attachment #749232 - Flags: review?(dave.hunt)
Attachment #749232 - Flags: review?(andreea.matei)
Attachment #749232 - Flags: review-
Comment on attachment 749232 [details] [diff] [review] Patch v1.0 - Release Branch Review of attachment 749232 [details] [diff] [review]: ----------------------------------------------------------------- Could you please explain the fix here. In comment 16 you mention that the test fails because it's comparing tw.rd.yahoo.com to tw.knowledge.yahoo.com. How does this change fix that, and have you tested other locales to ensure the fix still works for them? Perhaps add a short comment in the code?
Attachment #749232 - Flags: review?(dave.hunt)
Attachment #749232 - Flags: review?(andreea.matei)
Attachment #749232 - Flags: review-
Of course, and I'm sorry for not explaining it a little in the comments. Before we used to check the second element in the domain name, which in zh-TW is "rd" instead of "knowledge". Our patch get's the second element from the end, which always works ok for the builds I've tested (I tested with zh-TW, en-US, JA, de, ro). The reason I went with this approach was because of the en-US build that get's as a search engine Ebay, that in turn has a dynamic id in url that prevents us from using "utils.assertLoadedUrlEqual" as I've stated in comment 18. To better answer your question, our patch solves the issue by retrieving the "yahoo" element from the domain name instead of "knowledge". This works with all other locales since it's common to have the domain name right before the ".com" element. Please let me know if there are any other issues I missed and I will reply as soon as I can.
Thank you, I understand now. This is already rather a weak check in my opinion, and we're making it even weaker with this change. Henrik: do you think this is okay, or should we find another way to check that we have used the appropriate search engine? Could we perhaps use a custom (local) search engine here?
Flags: needinfo?(hskupin)
Yeah, we already have a local search engine we could use for the test. And indeed, that will be much better. Even I'm not sure what else we extend from a possible mochitest. Having an external search provider makes us ready for testing real locales and there pre-installed search engines. Can we assume that the target domain will always be the domain as listed in the search engine url? We have faced issues with the TLD already given from where you run the tests, so that might not be that easy. On the other side we could blacklist specific search engines which make it hard for us to use. I most likely would prefer that.
Flags: needinfo?(hskupin)
Here are the possible options as I see them: 1. Land this patch, which increases the chance of matching the partial domain name, but decreases the value in the check. 2. Keep the original domain matching, but maintain a blacklist of search engines that we want to avoid checking. 3. Install and use a local search engine instead of relying on remote services that may change without notice. 4. Remove the test (only if doing 3 would not add benefit over any existing mochitest) What do you think, Henrik? I also think we could do 1 and open a new bug for improving the test.
Flags: needinfo?(hskupin)
I would go with option 3 so we don't toggle back and forth in how we do the checks. It should not be that hard to implement.
Flags: needinfo?(hskupin)
Comment on attachment 765326 [details] [diff] [review] Patch v1.1 - Default Review of attachment 765326 [details] [diff] [review]: ----------------------------------------------------------------- ::: lib/search.js @@ +293,5 @@ > + this._controller.click(addButton); > + md.waitForDialog(TIMEOUT_INSTALL_DIALOG); > + > + //Check if the local engine has been installed > + var searchBarInstance = new searchBar(this._controller); A whitespace after // @@ +300,5 @@ > + }, "Search engine '" + aName + "' has been installed"); > + > + // The engine should not be selected by default > + expect.notEqual(searchBarInstance.selectedEngine, aName, > + "New search engine is not selected"); The message should be positive and express the wrong behavior so you'll get "Search engine has been selected - x should not equal y" when it fails. @@ +311,5 @@ > + * MozMillController of the engine manager > + * > + * @private > + */ > + _localEngineHandler : function engineManager_localeEngineHandler(aController) { I would call it "handleEngineInstall" as before, sounds better. @@ +325,5 @@ > + > + expect.equal(title.windowTitle, confirmTitle.addEngineTitle, > + "Window contains search engine title"); > + > + //Check that the correct domain is shown Whitespace needed ::: tests/functional/testSearch/testAddMozSearchProvider.js @@ +9,3 @@ > var modalDialog = require("../../../lib/modal-dialog"); > var search = require("../../../lib/search"); > var utils = require("../../../lib/utils"); There's assert and expect imported here and are not needed anymore. @@ +17,5 @@ > var setupModule = function(aModule) { > aModule.controller = mozmill.getBrowserController(); > > aModule.searchBar = new search.searchBar(aModule.controller); > + aModule.engineManager = new search.engineManager(aModule.controller); No need for the extra line and please move engineManager above searchBar. ::: tests/functional/testSearch/testSearchSelection.js @@ +16,5 @@ > > const PREF_LOAD_IN_BACKGROUND = "browser.search.context.loadInBackground"; > > +const SEARCH_ENGINE = {name: "mozqa.com", > + url : BASE_URL + "search/mozsearch.html"}; Please move this to new lines, intended with 2 spaces: https://developer.mozilla.org/en-US/docs/Mozmill_Tests/Mozmill_Style_Guide#Code_Blocks Also you can change this in the other test, since we're there. @@ +27,2 @@ > aModule.tabs = new tabs.tabBrowser(aModule.controller); > aModule.tabs.closeAllTabs(); No need for that blank line but we need one to separate declarations from calling methods. Also please move engineManager above searchBar.
Attachment #765326 - Flags: review?(andreea.matei) → review-
Happened today with Firefox 23.0b1 zh-TW on Ubuntu 12.10 64-bit: http://mozmill-ondemand.blargon7.com/#/functional/report/91511d5536e962e0de7f57ed1ed6bdf8
status-firefox23: --- → affected
Attached patch Patch v1.2 - Default (obsolete) — — Splinter Review
Made the changes requested in the review and tested it on Linux. If we are happy with this patch I will come with full reports on all platforms. Linux Reports: http://mozmill-crowd.blargon7.com/#/functional/report/a1b02004612785c13cf7c6bf1e5d135d http://mozmill-crowd.blargon7.com/#/functional/report/a1b02004612785c13cf7c6bf1e5d2389
Attachment #765326 - Attachment is obsolete: true
Attachment #770074 - Flags: review?(andreea.matei)
Comment on attachment 770074 [details] [diff] [review] Patch v1.2 - Default Review of attachment 770074 [details] [diff] [review]: ----------------------------------------------------------------- I think the next round should be final so please add all reports. Thanks ::: lib/search.js @@ +301,5 @@ > + }, "Search engine '" + aName + "' has been installed"); > + > + // The engine should not be selected by default > + expect.notEqual(searchBarInstance.selectedEngine, aName, > + "Search engine has been selected "); Trailing whitespace in the message. @@ +324,5 @@ > + var title = aController.window.document.title; > + } > + > + expect.equal(title.windowTitle, confirmTitle.addEngineTitle, > + "Window contains search engine title"); Both items return undefined to me, only title and confirmTitle seem to really be equal.
Attachment #770074 - Flags: review?(andreea.matei) → review-
Comment on attachment 771256 [details] [diff] [review] patch v1.3 Review of attachment 771256 [details] [diff] [review]: ----------------------------------------------------------------- Landed as: http://hg.mozilla.org/qa/mozmill-tests/rev/41d9c94575c4 (default) Please check if other branches are affected and update the flags. Thanks!
Attachment #771256 - Flags: review?(andreea.matei) → review+
Comment on attachment 771256 [details] [diff] [review] patch v1.3 Review of attachment 771256 [details] [diff] [review]: ----------------------------------------------------------------- I would love if we could further improve the new code. It's a bit confusing as of now. Please see my comments, which I put inline. ::: lib/search.js @@ +280,5 @@ > + * Local engine url value > + * @param {String} aName > + * Local engine name > + */ > + installLocalEngine : function engineManager_installLocalEngine(aUrl, aName) { Why does this method contain 'Local'. We do not enforce that the URL is local. The method should better be named 'installFromURL' and would also need an extra parameter for the element to issue the click against. Or even better it should have a callback which we would call to start the installation process. @@ +310,5 @@ > + * > + * @param {MozMillController} controller > + * MozMillController of the engine manager > + * > + * @private Given that this method is private it should have been added to the top of the class right behind the properties, getters, and setters.
Should I do a followup to the patch for Default and then provide backports for the rest of the branches with a final patch or should I come with backports and then a followup patch for default and branches?
Just a question back, what would make more sense and cause lesser work? As you know we do not want to start backporting not finished changes.
I only wanted to check that it would be ok to work on the followup and only after that on backports. That also sounded more logical to me too but I wanted to make sure it was ok. Thank you for the fast reply Henrik.
Firefox 23.0 (23.0, zh-TW, 20130718163513) on Linux Ubuntu 12.10 (x86_64): http://mozmill-release.blargon7.com/#/functional/report/180cf2548ef2865af3ae441d611e2c8a Firefox 23.0 (23.0, zh-TW, 20130718163513) Linux Ubuntu 12.10 (x86): http://mozmill-release.blargon7.com/#/functional/report/180cf2548ef2865af3ae441d611e12a2
Mario, we are waiting for a week now on a follow-up patch. When will we be able to get this landed, so a backport can happen?
Status: REOPENED → ASSIGNED
status-firefox24: --- → affected
I was working on Australis bugs as they had priority. I will work on the patch for this bug today.
Firefox 23.0 (23.0, zh-TW, 20130722172257) - Linux Ubuntu 12.10 (x86_64): > http://mozmill-release.blargon7.com/#/functional/report/180cf2548ef2865af3ae441d61a1c0e6 Firefox 23.0 (23.0, zu, 20130722172257) - Linux Ubuntu 12.10 (x86): > http://mozmill-release.blargon7.com/#/functional/report/180cf2548ef2865af3ae441d61a1dc6b Firefox 23.0 (23.0, zh-TW, 20130722172257) - Linux Ubuntu 12.10 (x86): > http://mozmill-release.blargon7.com/#/functional/report/180cf2548ef2865af3ae441d61a1babf
Comment on attachment 779209 [details] [diff] [review] searchEngineFollowup_2207.patch Review of attachment 779209 [details] [diff] [review]: ----------------------------------------------------------------- Looks fine for me, but I'd like to leave the final look for Henrik as he requested these changes.
Attachment #779209 - Flags: review?(hskupin)
Attachment #779209 - Flags: review?(andreea.matei)
Attachment #779209 - Flags: review+
status-firefox21: verified → ---
Whiteboard: [mozmill-test-failure] s=130128 u=failure c=search p=1 → [mozmill-test-failure]
This consistently fails with zh-TW and ja on Linux
This is interfering with the release signoff. So lets bump it to P1.
Keywords: reproducible
Priority: P2 → P1
We have a fix patch here, if this looks good to you Henrik we could get it landed as Andreea already gave her R+.
Comment on attachment 779209 [details] [diff] [review] searchEngineFollowup_2207.patch Review of attachment 779209 [details] [diff] [review]: ----------------------------------------------------------------- ::: lib/search.js @@ +166,5 @@ > /** > + * Handles the search engine installation procedure > + * > + * @param {MozMillController} controller > + * MozMillController of the engine manager This only reordered the method alphabetically? I would love if you could comment that, so we don't have to closely compare the code. While you are at it please fix the jsdoc. @@ +203,5 @@ > + * @private > + */ > + _localMozSearch : function engineManager_localMozSearch() { > + var addButton = new elementslib.Name(this._controller.tabs.activeTab, "add"); > + this._controller.click(addButton); This should never be part of the library. It's test code. As mentioned below please move it out to a closure in the test. @@ +318,5 @@ > this._controller.click(link); > }, > > /** > + * Installs a custom search engine from url nit: 'an URL' @@ +323,3 @@ > * > * @param {String} aUrl > + * Custom engine url value nit: URL @@ +326,2 @@ > * @param {String} aName > + * Custom engine name Custom doesn't seem to be necessary here. @@ +326,4 @@ > * @param {String} aName > + * Custom engine name > + * @param {String} aInstallCallback > + * Name of the element to issue the click against You misinterpreted what I wrote in my last review. A callback is a function and not a string. So it should end-up to be a closure in the test itself. @@ +336,5 @@ > var md = new modalDialog.modalDialog(this._controller.window); > md.start(this._handleEngineInstall); > > // Add the search engine > + aInstallCallback.call(this); Something is mixed-up here. Whether the above jsdoc string or the handling of a string variable. Please fix that.
Attachment #779209 - Flags: review?(hskupin) → review-
Attached patch searchEngineFollowup_2108.patch (obsolete) — — Splinter Review
The "_handleEngineInstall" has been moved up in the library file since it's @private and the localMozSearch callback function has been moved in the tests as suggested. We now call a function and not a string and the comments have been updated. Linux - Nightly 26 zh-TW reports http://mozmill-crowd.blargon7.com/#/functional/report/e503b7b0a70a3839c66c64572ebd61d4 http://mozmill-crowd.blargon7.com/#/functional/report/e503b7b0a70a3839c66c64572ebdc1d4
Attachment #779209 - Attachment is obsolete: true
Attachment #793451 - Flags: review?(hskupin)
Comment on attachment 793451 [details] [diff] [review] searchEngineFollowup_2108.patch Review of attachment 793451 [details] [diff] [review]: ----------------------------------------------------------------- ::: lib/search.js @@ +167,5 @@ > > /** > + * Handles the search engine installation procedure > + * > + * @param {MozMillController} controller You missed to fix the jsdoc as pointed out in my last review. @@ +317,4 @@ > * @param {String} aName > + * Engine name > + * @param {Function} aInstallCallback > + * Callback function that handles the install link element of the engine This description should be more general. Keep in mind that on a web page it's not always a link. There can be various ways how the engine can be installed. So something like the following would be better: 'Callback to trigger the installation of the engine' @@ +327,5 @@ > var md = new modalDialog.modalDialog(this._controller.window); > md.start(this._handleEngineInstall); > > // Add the search engine > + aInstallCallback.call(this); There is no need to execute the callback via ´call()´. Also ´this´ is not necessary given that the test already has an instance of the class. Further you want to check if the specified callback is a valid function first. ::: tests/functional/testSearch/testAddMozSearchProvider.js @@ +40,5 @@ > + * Handles the install link element of the custom engine > + */ > +var localMozSearch = function () { > + var addButton = new elementslib.Name(controller.tabs.activeTab, "add"); > + controller.click(addButton); As requested in my last review this should be a closure function. ::: tests/functional/testSearch/testSearchSelection.js @@ +41,5 @@ > * Use a search engine to search for the currently selected text. > */ > var testSearchSelectionViaContextMenu = function() { > + engineManager.installFromUrl(SEARCH_ENGINE.url, SEARCH_ENGINE.name, > + localMozSearch); As requested in my last review this should be a closure function.
Attachment #793451 - Flags: review?(hskupin) → review-
Attached patch searchEngineFollowup_2208.patch (obsolete) — — Splinter Review
Moved the callback function into the test and checked that we have a typeof == function in the library. Also fixed the JSDoc from "controller" to "aController", if there was another mistake please let me know and I will fix it. Sample report for Linux with Nightly 26 de: http://mozmill-crowd.blargon7.com/#/functional/report/e503b7b0a70a3839c66c64572edede54 If this looks ok now I will provide full reports on all platforms.
Attachment #793451 - Attachment is obsolete: true
Attachment #793964 - Flags: review?(hskupin)
Attachment #793964 - Flags: review?(andreea.matei)
Comment on attachment 793964 [details] [diff] [review] searchEngineFollowup_2208.patch Review of attachment 793964 [details] [diff] [review]: ----------------------------------------------------------------- ::: lib/search.js @@ +332,5 @@ > + aInstallCallback(); > + } > + else { > + throw new Error("Callback is not a valid function"); > + } There is no need to have a if/else here. Please make use of assert.equal() at the beginning of the method. ::: tests/functional/testSearch/testAddMozSearchProvider.js @@ +35,5 @@ > searchBar.selectedEngine = SEARCH_ENGINE.name; > searchBar.search({text: "Firefox", action: "goButton"}); > + > + // Callback to trigger the installation of the engine > + function localMozSearch () { Sorry, if that was not clear enough. Probably my fault. In this case it should even be an inline function, similar to all the ´waitFor()´ call we have.
Attachment #793964 - Flags: review?(hskupin)
Attachment #793964 - Flags: review?(andreea.matei)
Attachment #793964 - Flags: review-
Attached patch searchEngineFollowup_2508.patch (obsolete) — — Splinter Review
Updated the patch with the requested changes.
Attachment #793964 - Attachment is obsolete: true
Attachment #795316 - Flags: review?(andrei.eftimie)
Attachment #795316 - Flags: review?(andreea.matei)
Attachment #795316 - Flags: review?(hskupin)
Comment on attachment 795316 [details] [diff] [review] searchEngineFollowup_2508.patch Review of attachment 795316 [details] [diff] [review]: ----------------------------------------------------------------- ::: lib/search.js @@ +321,2 @@ > */ > + installFromUrl : function engineManager_installFromUrl(aInstallCallback, aUrl, aName) { I don't understand why the callback has been moved as first parameter here. It's necessary to have yes, but given the purpose of the method the url and name of the engine is more important. So those two have to be listed first. @@ +321,4 @@ > */ > + installFromUrl : function engineManager_installFromUrl(aInstallCallback, aUrl, aName) { > + // Check if the callback parameter is a valid function > + assert.equal(typeof aInstallCallback, "function"); Why do you add a comment above? You know that equal() accepts a message. @@ +330,5 @@ > var md = new modalDialog.modalDialog(this._controller.window); > md.start(this._handleEngineInstall); > > // Add the search engine > + aInstallCallback(); To remove the comment we could rename the callback variable to 'aAddEngineCallback'.
Attachment #795316 - Flags: review?(hskupin)
Attachment #795316 - Flags: review?(andrei.eftimie)
Attachment #795316 - Flags: review?(andreea.matei)
Attachment #795316 - Flags: review-
Attached patch searchEngineFollowup_3008.patch (obsolete) — — Splinter Review
Made the requested changes.
Attachment #795316 - Attachment is obsolete: true
Attachment #797828 - Flags: review?(hskupin)
Attachment #797828 - Flags: review?(andrei.eftimie)
Attachment #797828 - Flags: review?(andreea.matei)
Comment on attachment 797828 [details] [diff] [review] searchEngineFollowup_3008.patch Review of attachment 797828 [details] [diff] [review]: ----------------------------------------------------------------- I would really come to an end on that patch. So please work together with Andreea and Andrei before uploading another version of the patch. See my inline comments for what has to be fixed. Also don't forget to update the commit message. ::: lib/search.js @@ +179,5 @@ > + > + var title = aController.window.document.title; > + > + if (mozmill.isMac) > + title = aController.window.document.getElementById("info.title").textContent; This change here is totally irrelevant to the problem we are trying to fix. Why are you doing all those extra work? Please revert that. @@ +188,5 @@ > + var infoBody = aController.window.document.getElementById("info.body"); > + assert.waitFor(function () { > + return infoBody.textContent.indexOf('localhost') !== -1; > + }, "Search Engine URL contains correct domain - got '" + > + infoBody.textContent + "', expected 'localhost'"); Even with this latest change fixes the line length, it's also irrelevant. But here is a problem. The message cannot contain the got value because that will be from before waitFor is called and will be wrong. File a new bug so we can get waitFor calls like those fixed across the whole repository. @@ +312,3 @@ > * > + * @param {Function} aAddEngineCallback > + * Callback to trigger the installation of the engine Wrong order of parameters.
Attachment #797828 - Flags: review?(hskupin)
Attachment #797828 - Flags: review?(andrei.eftimie)
Attachment #797828 - Flags: review?(andreea.matei)
Attachment #797828 - Flags: review-
Attached patch searchEngineFollowup_0209.patch (obsolete) — — Splinter Review
In the last patch I changed the if block in order to make it look more elegant and set a default value, I have reverted the changes as you requested. I also sorted the parameters correctly and changed the comment so that we no longer use an outdated variable for the message. Hope it's all good now.
Attachment #797828 - Attachment is obsolete: true
Attachment #798547 - Flags: review?(hskupin)
Attachment #798547 - Flags: review?(andrei.eftimie)
Attachment #798547 - Flags: review?(andreea.matei)
Comment on attachment 798547 [details] [diff] [review] searchEngineFollowup_0209.patch Review of attachment 798547 [details] [diff] [review]: ----------------------------------------------------------------- I wonder why no-one else reviewed this patch in the last 7 days!? So it looks like it's up to me again... I hope that the next version will be the final one. ::: lib/search.js @@ +320,4 @@ > */ > + installFromUrl : function engineManager_installFromUrl(aName, aUrl, aAddEngineCallback) { > + assert.equal(typeof aAddEngineCallback, "function", > + "The callback parameter is a valid function"); If the method is getting called without that parameter, would that message help you to understand the problem? Personally I feel it hard to understand. So please make it so that we have a relation to the usage of the parameter.
Attachment #798547 - Flags: review?(hskupin)
Attachment #798547 - Flags: review?(andrei.eftimie)
Attachment #798547 - Flags: review?(andreea.matei)
Attachment #798547 - Flags: review-
Attached patch searchEngineFollowup_1009.patch (obsolete) — — Splinter Review
Updated the message following other library examples to better reflect the failure in case somebody would call the method without specifying all the necessary parameters or if the callback is not a function. http://mozmill-crowd.blargon7.com/#/functional/report/26d5854914a68aa13b333f28150fdaf5 http://mozmill-crowd.blargon7.com/#/functional/report/26d5854914a68aa13b333f281510a13f
Attachment #798547 - Attachment is obsolete: true
Attachment #802128 - Flags: review?(andrei.eftimie)
Attachment #802128 - Flags: review?(andreea.matei)
Comment on attachment 802128 [details] [diff] [review] searchEngineFollowup_1009.patch Review of attachment 802128 [details] [diff] [review]: ----------------------------------------------------------------- It's more a wording thing we still have to fix. So please do so. Also don't forget to update the commit message given that I was always the reviewer here and not Andreea. ::: lib/search.js @@ +320,4 @@ > */ > + installFromUrl : function engineManager_installFromUrl(aName, aUrl, aAddEngineCallback) { > + assert.equal(typeof aAddEngineCallback, "function", arguments.callee.name + > + ": Callback has been specified and it's a valid function"); Ok, so to shorten the review cycle on that bug I will give you the message as it should be used: "Callback for adding an engine has been specified".
Attachment #802128 - Flags: review?(andrei.eftimie)
Attachment #802128 - Flags: review?(andreea.matei)
Attachment #802128 - Flags: review-
Updated the message as requested and updated the patch message to reflect the right reviewer.
Attachment #802128 - Attachment is obsolete: true
Attachment #802145 - Flags: review?(hskupin)
Attachment #802145 - Flags: review?(andrei.eftimie)
Attachment #802145 - Flags: review?(andreea.matei)
Comment on attachment 802145 [details] [diff] [review] searchEngineFollowup_1009_v2.patch Review of attachment 802145 [details] [diff] [review]: ----------------------------------------------------------------- Sorry that I missed the item as pointed out below. But with that fixed we can land the follow-up patch. ::: lib/search.js @@ +320,3 @@ > */ > + installFromUrl : function engineManager_installFromUrl(aName, aUrl, aAddEngineCallback) { > + assert.equal(typeof aAddEngineCallback, "function", arguments.callee.name + Sorry, that I missed to mention in the last review, but please remove arguments.callee.name. This is not necessary given that any assert method call will have the right stack trace included.
Attachment #802145 - Flags: review?(hskupin)
Attachment #802145 - Flags: review?(andrei.eftimie)
Attachment #802145 - Flags: review?(andreea.matei)
Attachment #802145 - Flags: review+
Updated the patch as requested. If this is ok I will backport it for the rest of the branches.
Attachment #802184 - Flags: review?(hskupin)
Attachment #802184 - Flags: review?(andrei.eftimie)
Attachment #802184 - Flags: review?(andreea.matei)
Comment on attachment 802184 [details] [diff] [review] searchEngineFollowup_1009_v3.patch Review of attachment 802184 [details] [diff] [review]: ----------------------------------------------------------------- Looks good now, lets get this in so we can do backports. Landed: http://hg.mozilla.org/qa/mozmill-tests/rev/995523fe8c1e (default)
Attachment #802184 - Flags: review?(hskupin)
Attachment #802184 - Flags: review?(andrei.eftimie)
Attachment #802184 - Flags: review?(andreea.matei)
Attachment #802184 - Flags: review+
Attachment #802184 - Flags: checkin+
Attachment #802145 - Attachment is obsolete: true
The 23 branch is gone given that we merged from beta to release yesterday. So we have to fix it for 24 and 25 still.
status-firefox23: affected → wontfix
Comment on attachment 802901 [details] [diff] [review] searchEngine_Beta_v1.patch Review of attachment 802901 [details] [diff] [review]: ----------------------------------------------------------------- http://hg.mozilla.org/qa/mozmill-tests/rev/752cba73b10d (aurora) http://hg.mozilla.org/qa/mozmill-tests/rev/e24f02fa3273 (beta) Hope it's all good now.
Attachment #802901 - Flags: review?(andrei.eftimie)
Attachment #802901 - Flags: review?(andreea.matei)
Attachment #802901 - Flags: review+
Status: ASSIGNED → RESOLVED
Closed: 13 years ago → 12 years ago
status-firefox24: affected → fixed
status-firefox25: affected → fixed
Resolution: --- → FIXED
Oh, we missed to backport on esr17 and esr24 here. Mario, please check everything works fine there as well.
Status: RESOLVED → REOPENED
status-firefox-esr17: --- → affected
status-firefox-esr24: --- → affected
Resolution: FIXED → ---
Comment on attachment 804336 [details] [diff] [review] searchEngine_ESR17.patch Review of attachment 804336 [details] [diff] [review]: ----------------------------------------------------------------- Landed: http://hg.mozilla.org/qa/mozmill-tests/rev/a3515b892c42 (release) http://hg.mozilla.org/qa/mozmill-tests/rev/2ba112d9afd7 (esr24) http://hg.mozilla.org/qa/mozmill-tests/rev/824906b4ee3b (esr17)
Attachment #804336 - Flags: review?(andrei.eftimie)
Attachment #804336 - Flags: review?(andreea.matei)
Attachment #804336 - Flags: review+
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
status-firefox-esr17: affected → fixed
status-firefox-esr24: affected → fixed
Resolution: --- → FIXED
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: