Closed Bug 793092 Opened 12 years ago Closed 12 years ago

Replace controller.waitFor with assert/expect.waitFor in remaining mozmill tests and modules

Categories

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

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

Tracking

(firefox18 fixed, firefox19 fixed, firefox20 fixed, firefox21 fixed, firefox-esr10 fixed, firefox-esr17 fixed)

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox18 --- fixed
firefox19 --- fixed
firefox20 --- fixed
firefox21 --- fixed
firefox-esr10 --- fixed
firefox-esr17 --- fixed

People

(Reporter: vladmaniac, Assigned: daniela.p98911)

Details

(Whiteboard: s=121029 u=enhancement c=assertions p=1)

Attachments

(6 files, 7 obsolete files)

patch v3.4
94.63 KB, patch
AndreeaMatei
: review+
whimboo
: checkin+
Details | Diff | Splinter Review
follow-up patch v1.0 for Default branch
2.36 KB, patch
whimboo
: review+
whimboo
: checkin+
Details | Diff | Splinter Review
patch v1.1 for Aurora branch
96.15 KB, patch
whimboo
: review+
whimboo
: checkin+
Details | Diff | Splinter Review
patch v1.0 for Beta and Release branches
98.47 KB, patch
whimboo
: review+
whimboo
: checkin+
Details | Diff | Splinter Review
patch v1.0 for ESR17 branch
101.68 KB, patch
whimboo
: review+
whimboo
: checkin+
Details | Diff | Splinter Review
patch v1.0 for ESR10 branch
99.08 KB, patch
whimboo
: review+
whimboo
: checkin+
Details | Diff | Splinter Review
See bug 787024 We have addressed this refactoring there for functional restart tests, update tests and the software-update module and finally the remote restart tests. We need to continue in this bug with the remaining mozmill tests and module * endurance tests * addons tests * functional tests which are not restart tests * files under /lib (shared modules) * ..etc
Priority: -- → P2
This should be on our plate for the next two weeks.
Whiteboard: s=121008 u=enhancement c=assertions p=1
Whiteboard: s=121008 u=enhancement c=assertions p=1
Whiteboard: s=121029 u=enhancement c=assertions p=1
Assignee: nobody → alex.lakatos
Attached patch patch v1.0 (obsolete) — — Splinter Review
edited every occurrence of waitFor from all folders/files
Attachment #677345 - Flags: review?(hskupin)
Attachment #677345 - Flags: review?(dave.hunt)
Comment on attachment 677345 [details] [diff] [review] patch v1.0 Review of attachment 677345 [details] [diff] [review]: ----------------------------------------------------------------- Please provide a link to a Mozmill dashboard report that has run with this patch applied. ::: lib/search.js @@ +675,5 @@ > } > > // Get suggestions in an array if the popup with suggestions is opened > if (popup.getNode().state === 'open') { > + expect.waitForElement(treeElem, TIMEOUT); There is no such function 'waitForElement' in assertions.js ::: lib/tabs.js @@ +507,5 @@ > // Wait for the top margin to be 0px - ie. has stopped animating > // XXX: A notification bar starts at a negative pixel margin and drops down > // to 0px. This creates a race condition where a test may click > // before the notification bar appears at it's anticipated screen location > + expect.waitFor(function () { This should be an assert. ::: tests/endurance/testTabbedBrowsing_PinAndUnpinAppTab/test1.js @@ +45,5 @@ > var lastTabIndex = tabBrowser.length-1; > > // Switch to the last tab and wait for it to scroll into view if necessary > controller.tabs.selectTabIndex(lastTabIndex); > + assert.waitFor(function () { This should be an expect. ::: tests/functional/testAwesomeBar/testAccessLocationBar.js @@ +56,5 @@ > return locationBar.autoCompleteResults.isOpened; > }, "Autocomplete results should be visible"); > > controller.keypress(locationBar.urlbar, "VK_DOWN", {}); > + expect.waitFor(function () { This should be an assert. ::: tests/functional/testAwesomeBar/testEscapeAutocomplete.js @@ +56,5 @@ > // After the first Escape press > controller.keypress(locationBar.urlbar, 'VK_ESCAPE', {}); > expect.contain(locationBar.value, TEST_STRING, > "Search string found in the locationbar"); > + expect.waitFor(function () { This should be an assert. ::: tests/functional/testAwesomeBar/testPasteLocationBar.js @@ +55,5 @@ > var contextMenuEntry = locationBar.getElement({type: "contextMenu_entry", subtype: "paste"}); > controller.click(contextMenuEntry); > > // Get contents of the location bar and compare it to the expected result > + expect.waitFor(function () { This should be an assert. ::: tests/functional/testAwesomeBar/testVisibleItemsMax.js @@ +63,5 @@ > > // Get the visible results from the autocomplete list. Verify it is equal to maxrows > var autoCompleteResultsList = locationBar.autoCompleteResults.getElement({type:"results"}); > var maxRows = locationBar.urlbar.getNode().getAttribute("maxrows"); > + expect.waitFor(function () { This should be an assert. ::: tests/functional/testPasswordManager/testPasswordNotificationBar.js @@ +55,5 @@ > // Close the notification and check its state > controller.keypress(passwordNotification, "VK_ESCAPE", {}); > > var notification = locationBar.getNotification(); > + expect.waitFor(function () { This should be an assert. ::: tests/functional/testPrivateBrowsing/testGeolocation.js @@ +53,5 @@ > controller.keypress(null, shortcut, {ctrlKey: mozmill.isMac, altKey: !mozmill.isMac}); > > try { > var result = new elementslib.ID(controller.tabs.activeTab, "result"); > + expect.waitFor(function () { This should be an assert. ::: tests/functional/testSearch/testRemoveSearchEngine.js @@ +30,5 @@ > > // Remove the first engine in the list > searchBar.openEngineManager(handleEngines); > > + expect.waitFor(function () { This should be an assert. ::: tests/functional/testSearch/testRestoreDefaults.js @@ +55,5 @@ > for (var i = manager.engines.length; i > 1; i--) { > var name = manager.engines[i - 1].name; > > manager.removeEngine(name); > + expect.waitFor(function () { This should be an assert. ::: tests/functional/testSecurity/testDVCertificate.js @@ +40,5 @@ > controller.click(identityBox); > > // Make sure the doorhanger is "open" before continuing > var doorhanger = new elementslib.ID(controller.window.document, "identity-popup"); > + expect.waitFor(function () { This should be an assert. ::: tests/functional/testSecurity/testGreenLarry.js @@ +55,5 @@ > controller.click(identityBox); > > // Make sure the doorhanger is "open" before continuing > var doorhanger = new elementslib.ID(controller.window.document, "identity-popup"); > + expect.waitFor(function () { This should be an assert. @@ +134,5 @@ > > // Check the Web Site label against the Cert CName > var webIDDomainLabel = new elementslib.ID(controller.window.document, > "security-identity-domain-value"); > + expect.waitFor(function () { This should be an assert. ::: tests/functional/testSecurity/testGreyLarry.js @@ +33,5 @@ > controller.click(new elementslib.ID(controller.window.document, "identity-box")); > > // Make sure the doorhanger is "open" before continuing > var doorhanger = new elementslib.ID(controller.window.document, "identity-popup"); > + expect.waitFor(function () { This should be an assert. ::: tests/functional/testSecurity/testIdentityPopupOpenClose.js @@ +36,5 @@ > controller.click(identityBox); > > // Check the popup state > var popup = new elementslib.ID(controller.window.document, "identity-popup"); > + expect.waitFor(function () { This should be an assert. @@ +51,5 @@ > // Press Escape to close the popup > controller.keypress(popup, 'VK_ESCAPE', {}); > > // Check the popup state again > + expect.waitFor(function () { This should be an assert. ::: tests/functional/testSecurity/testSecurityInfoViaMoreInformation.js @@ +32,5 @@ > controller.click(identityBox); > > // Make sure the doorhanger is "open" before continuing > var doorhanger = new elementslib.ID(controller.window.document, "identity-popup"); > + expect.waitFor(function () { This should be an assert. ::: tests/functional/testTabbedBrowsing/testBackgroundTabScrolling.js @@ +90,5 @@ > > // and is displayed inside the all tabs popup menu > controller.click(allTabsButton); > > + expect.waitFor(function () { This should be an assert. @@ +113,5 @@ > }, "Link 2 title is visible for the last tab"); > > // Close the all tabs menu > controller.click(allTabsButton); > + expect.waitFor(function () { This should be an assert. ::: tests/functional/testTabbedBrowsing/testOpenInBackground.js @@ +74,5 @@ > tabBrowser.selectedIndex = 3; > tabBrowser.closeTab("closeButton"); > > // Verify that the last tab is selected: > + expect.waitFor(function () { This should be an assert. @@ +79,4 @@ > return tabBrowser.length === 3; > }, "A tab has been closed via the close button"); > > + expect.waitFor(function () { This should be an assert.
Attachment #677345 - Flags: review?(hskupin)
Attachment #677345 - Flags: review?(dave.hunt)
Attachment #677345 - Flags: review-
Please assign the bug when you start working on it. Thanks.
Status: NEW → ASSIGNED
Assignee: alex.lakatos → dpetrovici
Comment on attachment 681439 [details] [diff] [review] skip v2.0 Review of attachment 681439 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay in the review. As you can see in the following a lot of stuff to comment on. Nothing major but we have to clean-up the code so we do the right assert or expect, given on how we operate on elements or simply check values. ::: lib/dom-utils.js @@ +114,5 @@ > * [optional - default: no waiting] > */ > walk : function DOMWalker_walk(ids, root, waitFunction) { > if (typeof waitFunction == 'function') > + assert.waitFor(waitFunction()); Not completely sure about that one. But I think it shouldn't be an assert. @@ +323,2 @@ > return nodeToProcess.value == idSet.value; > }, "The menu item did not load in time: " + idSet.value); This is clearly an assert because we are waiting for the target state of an ui element to operate on. @@ +348,2 @@ > return nodeToProcess.loaded; > }, "The pane did not load in time: " + idSet.id); If the pane hasn't been loaded we cannot continue the test. So that's an assert too. ::: lib/prefs.js @@ +68,2 @@ > return selector.getNode().selectedItem.getAttribute('pane') === this.selectedPane.getNode().id; > }, "Pane has been changed - expected '" + this.selectedPane.getNode().id + "'", That's just a sanity check and can be an expect. @@ +91,2 @@ > return documentElement.lastSelected === id; > }, "Pane has been changed - expected '" + id + "'"); Why is that one an expect? If the pane has not been changed we cannot continue with the test. ::: lib/search.js @@ +469,2 @@ > return !this.enginesDropDownOpen; > }, "Search engines drop down has been closed", undefined, undefined, this); This is an UI update and has to be an assertion! @@ +669,2 @@ > return popup.getNode().state === 'open'; > }, "", TIMEOUT_REQUEST_SUGGESTIONS); When you change this to an expect which is right, please get rid of the surrounding try/catch. ::: lib/tabs.js @@ +484,2 @@ > return self.opened; > }, "New tab has been opened"); Why expect here? If the tab hasn't been opened it's clearly a stopper. ::: tests/endurance/testTabbedBrowsing_PinAndUnpinAppTab/test1.js @@ +50,2 @@ > return scrollButtonDown.getNode().hasAttribute("collapsed") || scrollButtonDown.getNode().disabled; > }, "Tab has scrolled into view."); I do not think that this can be an expect. If we fail to scroll the tab into view the following action for opening the context menu will fail. ::: tests/functional/testAwesomeBar/testAccessLocationBar.js @@ +2,5 @@ > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > // Include required modules > +var { assert, expect } = require("../../../lib/assertions"); Why you import expect? It's not used. ::: tests/functional/testAwesomeBar/testCheckItemHighlight.js @@ +2,5 @@ > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > // Include required modules > +var { assert, expect } = require("../../../lib/assertions"); Same here. ::: tests/functional/testAwesomeBar/testEscapeAutocomplete.js @@ +59,1 @@ > "Search string found in the locationbar"); Why this change? @@ +61,2 @@ > return !locationBar.autoCompleteResults.isOpened; > }, "Autocomplete list has been closed"); Again it's an UI change, so we have to assert that this list is closed given the following code in this test module. ::: tests/functional/testAwesomeBar/testSuggestBookmarks.js @@ +2,5 @@ > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > // Include required modules > +var { assert, expect } = require("../../../lib/assertions"); why expect? ::: tests/functional/testAwesomeBar/testVisibleItemsMax.js @@ +2,5 @@ > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > // Include required modules > +var { assert, expect } = require("../../../lib/assertions"); why expect? ::: tests/functional/testFormManager/testSaveFormInformation.js @@ +49,5 @@ > > // Verify form completion in each inputted field > var popDownAutoCompList = new elementslib.ID(controller.window.document, "PopupAutoComplete"); > > + assert.waitFor(function() { nit: while you are on it add a blank after 'function'. @@ +60,5 @@ > > lastName = new elementslib.ID(controller.tabs.activeTab, "ship_lname"); > controller.type(lastName, LNAME.substring(0,2)); > > + assert.waitFor(function() { Here too. ::: tests/functional/testPrivateBrowsing/testDownloadManagerClosed.js @@ +2,5 @@ > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > // Include the required modules > +var { assert, expect } = require("../../../lib/assertions"); why expect? ::: tests/functional/testSearch/testAddMozSearchProvider.js @@ +2,5 @@ > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > // Include necessary modules > +var { assert, expect } = require("../../../lib/assertions"); why expect? ::: tests/functional/testSearch/testGetMoreSearchEngines.js @@ +2,5 @@ > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > // Include necessary modules > +var { assert, expect } = require("../../../lib/assertions"); why expect? ::: tests/functional/testSearch/testOpenSearchAutodiscovery.js @@ +2,5 @@ > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > // Include necessary modules > +var { assert, expect } = require("../../../lib/assertions"); why expect? ::: tests/functional/testSearch/testRemoveSearchEngine.js @@ +2,5 @@ > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > // Include necessary modules > +var { assert, expect } = require("../../../lib/assertions"); why expect? ::: tests/functional/testSearch/testRestoreDefaults.js @@ +2,5 @@ > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > // Include necessary modules > +var { assert, expect } = require("../../../lib/assertions"); why expect? ::: tests/functional/testSearch/testSearchSelection.js @@ +2,5 @@ > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > // Include necessary modules > +var { assert, expect } = require("../../../lib/assertions"); why expect? ::: tests/functional/testSecurity/testDVCertificate.js @@ +2,5 @@ > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > // Include necessary modules > +var { assert, expect } = require("../../../lib/assertions"); why expect? ::: tests/functional/testSecurity/testGreenLarry.js @@ +139,2 @@ > return webIDDomainLabel.getNode().value.indexOf(cert.commonName) !== -1; > }, "Found certificate common name '" + cert.commonName + "'"); We do not operate on this element. Given that it's a simple check for it's value it has to be an expect. But why is that a waitFor call it all? That's not something I expect here. Looks like an accident we did in the past. ::: tests/functional/testSecurity/testGreyLarry.js @@ +2,5 @@ > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > // Include necessary modules > +var { assert, expect } = require("../../../lib/assertions"); why expect? ::: tests/functional/testSecurity/testIdentityPopupOpenClose.js @@ +6,5 @@ > * Litmus Test 8579: Display and close Larry > */ > > // Include necessary modules > +var { assert, expect } = require("../../../lib/assertions"); why expect? ::: tests/functional/testSecurity/testSecurityInfoViaMoreInformation.js @@ +2,5 @@ > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > // Include necessary modules > +var { assert, expect } = require("../../../lib/assertions"); why expect? ::: tests/functional/testToolbar/testBackForwardButtons.js @@ +52,5 @@ > } > > // Click on the Forward button for the number of websites visited > for (var j = 1; j < LOCAL_TEST_PAGES.length; j++) { > + assert.waitFor(function() { nit: please add a blank after function.
Attachment #681439 - Flags: review?(hskupin)
Attachment #681439 - Flags: review?(dave.hunt)
Attachment #681439 - Flags: review-
Attached patch patch v3.0 (obsolete) — — Splinter Review
(In reply to Henrik Skupin (:whimboo) from comment #6) > Comment on attachment 681439 [details] [diff] [review] > skip v2.0 > ::: tests/functional/testAwesomeBar/testEscapeAutocomplete.js > @@ +59,1 @@ > > "Search string found in the locationbar"); > > Why this change? There is no change there. > > // Include required modules > > +var { assert, expect } = require("../../../lib/assertions"); > > why expect? For most of these questions the answer is "Because it is used already in the test file, it was imported there already." For the other ones, I removed the import.
Attachment #681439 - Attachment is obsolete: true
Attachment #684688 - Flags: review?(hskupin)
Attachment #684688 - Flags: review?(dave.hunt)
Comment on attachment 684688 [details] [diff] [review] patch v3.0 Review of attachment 684688 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Alex Lakatos[:AlexLakatos] from comment #7) > > ::: tests/functional/testAwesomeBar/testEscapeAutocomplete.js > > @@ +59,1 @@ > > > "Search string found in the locationbar"); > > > > Why this change? > There is no change there. Splinter review didn't quote that right. Next time please check the whole diff. My comment references line 58 in that file. > > > // Include required modules > > > +var { assert, expect } = require("../../../lib/assertions"); > > > > why expect? > For most of these questions the answer is "Because it is used already in the > test file, it was imported there already." For the other ones, I removed the > import. No, that's not true. Here you have added this line and it wasn't used before. And this is true for mostly all of my comments. So in the future you should wait with updating a patch until it's clear what has to be done. It's hard to check interim work.
Attachment #684688 - Flags: review?(hskupin)
Attachment #684688 - Flags: review?(dave.hunt)
Attachment #684688 - Flags: review-
What's the status of this bug?
It's in my queue, but P1 remains the private browsing tests. I'll update a patch asap.
Attached patch patch v3.1 (obsolete) — — Splinter Review
Modified patch and also removed the waitFor in the code below: @@ +139,2 @@ > return webIDDomainLabel.getNode().value.indexOf(cert.commonName) !== -1; > }, "Found certificate common name '" + cert.commonName + "'");
Attachment #684688 - Attachment is obsolete: true
Attachment #697929 - Flags: review?(hskupin)
Attachment #697929 - Flags: review?(dave.hunt)
Attachment #697929 - Flags: review?(andreea.matei)
Comment on attachment 697929 [details] [diff] [review] patch v3.1 Review of attachment 697929 [details] [diff] [review]: ----------------------------------------------------------------- Checked all imported assert and expect, we only miss one at the test below. Also at my testrun, these failures appeared, although I don't see you changing testAddons or testSearchSuggestions, maybe from the shared modules: http://mozmill-crowd.blargon7.com/#/functional/report/23d8fbdd0190d4b0496d6b129f9211dd ::: tests/functional/testAwesomeBar/testEscapeAutocomplete.js @@ +2,5 @@ > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > // Include required modules > +var { assert } = require("../../../lib/assertions"); We need an expect here as well, since the test uses expect.contain at line 58.
Attachment #697929 - Flags: review?(hskupin)
Attachment #697929 - Flags: review?(dave.hunt)
Attachment #697929 - Flags: review?(andreea.matei)
Attachment #697929 - Flags: review-
Attached patch patch v3.2 (obsolete) — — Splinter Review
It was an issue with lib/addons.js file where the assert was not added. I have run the functional tests and the results are here: http://mozmill-crowd.blargon7.com/#/functional/report/23d8fbdd0190d4b0496d6b129f93fac9 http://mozmill-crowd.blargon7.com/#/functional/report/23d8fbdd0190d4b0496d6b129f9453cb
Attachment #697929 - Attachment is obsolete: true
Attachment #698628 - Flags: review?(hskupin)
Attachment #698628 - Flags: review?(dave.hunt)
Attachment #698628 - Flags: review?(andreea.matei)
Comment on attachment 698628 [details] [diff] [review] patch v3.2 Review of attachment 698628 [details] [diff] [review]: ----------------------------------------------------------------- All looks good to me now. Thanks Daniela and Alex!
Attachment #698628 - Flags: review?(hskupin)
Attachment #698628 - Flags: review?(dave.hunt)
Attachment #698628 - Flags: review?(andreea.matei)
Attachment #698628 - Flags: review+
Comment on attachment 698628 [details] [diff] [review] patch v3.2 Review of attachment 698628 [details] [diff] [review]: ----------------------------------------------------------------- This patch doesn't apply cleanly anymore and needs to be updated.
Attachment #698628 - Flags: review-
Attached patch patch v3.3 (obsolete) — — Splinter Review
Fixed the problems with applying the patch
Assignee: alex.lakatos → dpetrovici
Attachment #699115 - Flags: review?(hskupin)
Attachment #699115 - Flags: review?(dave.hunt)
Attachment #699115 - Flags: review?(andreea.matei)
Attachment #698628 - Attachment is obsolete: true
Comment on attachment 699115 [details] [diff] [review] patch v3.3 Review of attachment 699115 [details] [diff] [review]: ----------------------------------------------------------------- ::: lib/search.js @@ +663,5 @@ > // Typing too fast can cause several issue like the suggestions not to appear. > // Lets type the letters one by one and wait for the popup or the timeout > for (var i = 0; i < searchTerm.length; i++) { > + this.type(searchTerm[i]); > + expect.waitFor(function () { This is failing for me. Not every time, but regularly enough. I suspect it's the reason for the previous try/catch. See http://mozmill-crowd.blargon7.com/#/functional/report/bddfde7ab90491e4945eb7577c01a81e
Attachment #699115 - Flags: review?(hskupin)
Attachment #699115 - Flags: review?(dave.hunt)
Attachment #699115 - Flags: review?(andreea.matei)
Attachment #699115 - Flags: review-
Attached patch patch v3.4 — — Splinter Review
Changed patch to use assert.waitFor in lib/search.js and due to the conversation below. Reports are: http://mozmill-crowd.blargon7.com/#/functional/report/bddfde7ab90491e4945eb7577c13586a http://mozmill-crowd.blargon7.com/#/functional/report/bddfde7ab90491e4945eb7577c136986 <danielapetrovici> whimboo, davehunt, I managed to reproduce the issue dave saw with the patch for bug 793092... the only problem is that I cannot figure out how to fix it... I added back the try catch block as dave suggested, but the issue still reproduces <_AutomationBot> Bug https://bugzilla.mozilla.org/show_bug.cgi?id=793092 normal, ASSIGNED , Replace controller.waitFor with assert/expect.waitFor in remaining mozmill tests and modules <danielapetrovici> when I am adding controller.waitFor it works, but with expect.waitFor it fails and I did not change anything else now <danielapetrovici> can you please tell me how I can investigate further to find a solution? <davehunt> danielapetrovici: maybe look at the differences between controller.waitFor and expect.waitFor? Does it also occur with assert.waitFor? <davehunt> whimboo: I've confirmed that we'll be at FOSDEM, and booked my travel and accomodation <whimboo> davehunt: oh damn. i forgot that <whimboo> i also have to do it <whimboo> thanks for the reminder <danielapetrovici> davehunt, with assert it does not fail <danielapetrovici> and also when expect is called it does not enter in the catch part of try, when assert is called it catches the exception... I am not sure if this is related... <davehunt> danielapetrovici: expect does not throw, that's the point of expect vs assert <danielapetrovici> ok, so can I use assert there then? <davehunt> expect is a 'soft' assertion <davehunt> danielapetrovici: it depends on the reason for ignoring the timeout, if the reasoning is still valid, then using assert and catching it wfm, but it's not something I would generally encourage * AutomatedTester|away is now known as AutomatedTester <danielapetrovici> the reason was that // Typing too fast can cause several issue like the suggestions not to appear. ... and it is still valid, suggestions do not appear when typing to fast <davehunt> but why is it safe to ignore that timeout? <davehunt> timeout failure <danielapetrovici> I this that due to this: bug 542990... which is still new and it is added in the test also (a comment above the one I sent you) <_AutomationBot> Bug https://bugzilla.mozilla.org/show_bug.cgi?id=542990 normal, NEW , search suggestion drop-down disappears at inconvenient times <danielapetrovici> is it ok to change this to an assert due to this bug, or should I investigate something else? <davehunt> danielapetrovici: as it was originally an assert, please go ahead and upload a new patch. Reference this conversation in the bug, and then the reviewers can comment.
Attachment #699115 - Attachment is obsolete: true
Attachment #701056 - Flags: review?(hskupin)
Attachment #701056 - Flags: review?(dave.hunt)
Attachment #701056 - Flags: review?(andreea.matei)
Comment on attachment 701056 [details] [diff] [review] patch v3.4 Review of attachment 701056 [details] [diff] [review]: ----------------------------------------------------------------- Works for me now and still applying cleanly.
Attachment #701056 - Flags: review?(hskupin)
Attachment #701056 - Flags: review?(dave.hunt)
Attachment #701056 - Flags: review?(andreea.matei)
Attachment #701056 - Flags: review+
Comment on attachment 703226 [details] [diff] [review] patch v1.0 for Aurora branch Review of attachment 703226 [details] [diff] [review]: ----------------------------------------------------------------- Hm, I did a deeper search which I missed before and found the following left over lines of code: ./tests/endurance/testBookmarks_OpenAndClose/test1.js: aController.waitFor(function () { This should be fixed in this patch. If default is also affected please create a follow-up for all the necessary changes mentioned in this review request. Also I can see those lines which we should most likely cover in a follow-up bug: $ grep -r 'utils.waitFor(' . ./lib/addons.js: mozmill.utils.waitFor(function() { ./lib/modal-dialog.js: mozmill.utils.waitFor(function () { ./lib/places.js: mozmill.utils.waitFor(function () { ./lib/private-browsing.js: mozmill.utils.waitFor(function () { ./lib/tabview.js: mozmill.utils.waitFor(function () { ./lib/toolbars.js: mozmill.utils.waitFor(function() { ./lib/utils.js: mozmill.utils.waitFor(function () { ::: lib/tabs.js @@ +9,5 @@ > * @version 1.0.0 > */ > > // Include required modules > +var { assert } = require("assertions"); On default this also includes `expect`. Even if we don't need it we should keep the patches in sync. Otherwise it can cause trouble during the merge. Please make sure to get this updated.
Attachment #703226 - Flags: review?(hskupin)
Attachment #703226 - Flags: review?(dave.hunt)
Attachment #703226 - Flags: review?(andreea.matei)
Attachment #703226 - Flags: review-
This is the follow-up patch for Default branch. I have fixed the missing aController.waitFor() instance I have changed the /lib/tabs.js by removing "expect" from: var { assert, expect } = require("assertions"); - expect was never used by the library and I think I forgot about it in a previous version of the patch. - Since we are adding a follow up patch for default, I thought it would be better to fix this, too. Reports for default branch - I have ran testruns for all the affected tests since I have removed something in /lib/tabs.js: Addons: http://mozmill-crowd.blargon7.com/#/addons/report/9e41582ed5e806fa373351d9d3183b69 Endurance: http://mozmill-crowd.blargon7.com/#/endurance/report/9e41582ed5e806fa373351d9d3184921 Functional: http://mozmill-crowd.blargon7.com/#/functional/report/9e41582ed5e806fa373351d9d3189968 http://mozmill-crowd.blargon7.com/#/functional/report/9e41582ed5e806fa373351d9d3187707 Also logged bug 831714 for the remaining mozmill.utils.waitFor() instances
Attachment #703285 - Flags: review?(hskupin)
Attachment #703285 - Flags: review?(dave.hunt)
Attachment #703285 - Flags: review?(andreea.matei)
I fixed the missing instance of aController.waitFor. I think that it is better to fix default to not have "expect" in /lib/tabs.js - per previous patch (since the /lib/tabs.js on default is not using it). So, this is why I made the change on default, not aurora branch. Reports are added below: Functional: http://mozmill-crowd.blargon7.com/#/functional/report/9e41582ed5e806fa373351d9d3188992 http://mozmill-crowd.blargon7.com/#/functional/report/9e41582ed5e806fa373351d9d31876f1 Endurance: http://mozmill-crowd.blargon7.com/#/endurance/report/9e41582ed5e806fa373351d9d3196bbc Addons: http://mozmill-crowd.blargon7.com/#/addons/report/9e41582ed5e806fa373351d9d31975bb
Attachment #703226 - Attachment is obsolete: true
Attachment #703290 - Flags: review?(hskupin)
Attachment #703290 - Flags: review?(dave.hunt)
Attachment #703290 - Flags: review?(andreea.matei)
Comment on attachment 703285 [details] [diff] [review] follow-up patch v1.0 for Default branch Review of attachment 703285 [details] [diff] [review]: ----------------------------------------------------------------- The original patch has not been checked in yet, so I don't see a reason for a follow-up patch. Please combine both.
Attachment #703285 - Flags: review?(hskupin)
Attachment #703285 - Flags: review?(dave.hunt)
Attachment #703285 - Flags: review?(andreea.matei)
Attachment #703285 - Flags: review?(hskupin)
Attachment #703285 - Flags: review?(dave.hunt)
Attachment #703285 - Flags: review?(andreea.matei)
Attachment #703285 - Flags: review?(hskupin)
Attachment #703285 - Flags: review?(dave.hunt)
Attachment #703285 - Flags: review?(andreea.matei)
Attachment #703285 - Flags: review+
Comment on attachment 703290 [details] [diff] [review] patch v1.1 for Aurora branch Review of attachment 703290 [details] [diff] [review]: ----------------------------------------------------------------- http://hg.mozilla.org/qa/mozmill-tests/rev/a36fb4895794 (aurora) Please follow-up with further patches for the other branches.
Attachment #703290 - Flags: review?(hskupin)
Attachment #703290 - Flags: review?(dave.hunt)
Attachment #703290 - Flags: review?(andreea.matei)
Attachment #703290 - Flags: review+
Attachment #703290 - Flags: checkin+
The patch for Beta release. It applies clearly on Release branch too. NOTES: 1) It would apply clearly on ESR17, but some instances of waitFor will remain in tests so for ESR17 I will provide a new patch in a few moments. 2) It would take some time to add the ESR10 patch as none of the patches clearly apply on this branch. I can add it by tomorrow morning in the worst case. Reports for Beta: Functional: http://mozmill-crowd.blargon7.com/#/functional/report/9e41582ed5e806fa373351d9d31a6675 http://mozmill-crowd.blargon7.com/#/functional/report/9e41582ed5e806fa373351d9d31aba8b Endurance: http://mozmill-crowd.blargon7.com/#/endurance/report/9e41582ed5e806fa373351d9d31c74b1 Addons: http://mozmill-crowd.blargon7.com/#/addons/report/9e41582ed5e806fa373351d9d31cecc8 Reports for Release: Functional: http://mozmill-crowd.blargon7.com/#/functional/report/9e41582ed5e806fa373351d9d31dffe2 http://mozmill-crowd.blargon7.com/#/functional/report/9e41582ed5e806fa373351d9d31e1755 Endurance: http://mozmill-crowd.blargon7.com/#/endurance/report/9e41582ed5e806fa373351d9d31e33be Addons: http://mozmill-crowd.blargon7.com/#/addons/report/9e41582ed5e806fa373351d9d31e3a37
Attachment #703330 - Flags: review?(hskupin)
Attachment #703330 - Flags: review?(dave.hunt)
Attachment #703330 - Flags: review?(andreea.matei)
Comment on attachment 703330 [details] [diff] [review] patch v1.0 for Beta and Release branches Review of attachment 703330 [details] [diff] [review]: ----------------------------------------------------------------- http://hg.mozilla.org/qa/mozmill-tests/rev/b9d820445e89 (beta) http://hg.mozilla.org/qa/mozmill-tests/rev/1d75c8555b1a (release)
Attachment #703330 - Flags: review?(hskupin)
Attachment #703330 - Flags: review?(dave.hunt)
Attachment #703330 - Flags: review?(andreea.matei)
Attachment #703330 - Flags: review+
Attachment #703330 - Flags: checkin+
Comment on attachment 703346 [details] [diff] [review] patch v1.0 for ESR17 branch Review of attachment 703346 [details] [diff] [review]: ----------------------------------------------------------------- http://hg.mozilla.org/qa/mozmill-tests/rev/6d8235385a80 (esr17)
Attachment #703346 - Flags: review?(hskupin)
Attachment #703346 - Flags: review?(dave.hunt)
Attachment #703346 - Flags: review?(andreea.matei)
Attachment #703346 - Flags: review+
Attachment #703346 - Flags: checkin+
Comment on attachment 703408 [details] [diff] [review] patch v1.0 for ESR10 branch Review of attachment 703408 [details] [diff] [review]: ----------------------------------------------------------------- You forgot to update the commit message. I will correct it but be sure next time to check it before uploading the patch. http://hg.mozilla.org/qa/mozmill-tests/rev/10c431bd3230 (esr10)
Attachment #703408 - Flags: review?(hskupin)
Attachment #703408 - Flags: review?(dave.hunt)
Attachment #703408 - Flags: review?(andreea.matei)
Attachment #703408 - Flags: review+
Attachment #703408 - Flags: checkin+
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
status-firefox-esr10: affected → fixed
status-firefox18: affected → fixed
status-firefox19: affected → fixed
status-firefox20: affected → fixed
status-firefox-esr17: 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: