848200 - Clean up and remove timeout constant where set to default 5000ms

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Jonathan French (:jfrench)]

Cruising through the default branch I came across what appears to be a misuse of the TIMEOUT constant, in functional/testSecurity/testGreenLarry.js

var TIMEOUT = 5000;

According to the code templates I believe it's supposed to be

const TIMEOUT = 5000;

I checked the whole repo, and this test was the only occurrence of it being set as 'var'. That being said, it doesn't appear to be directly used in testGreenLarry.js at all. So I could either correct it to const, or remove the line altogether. 

I suspect the patch could be landed in any branch if needed.

Comment 1

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

I would propose we get rid of those constants as best as possible instead. There is no need to keep them, especially for cases like waitForElement(). We should only keep them if a value other than 5s is specified, and then it should be a constant, right. Jonathan, do you want to fix that?

Comment 2

[Jonathan French (:jfrench)]

Sure, go ahead and assign me, and I will remove all instances of timeout where the time has been left at the default template value of 5000ms. I gather this is what mozmill sets it to internally, even when it isn't declared. In cases where timeout is set to 5000 but that constant is manipulated by some other var later in the same test, I will leave it in.

Comment 3

[Jonathan French (:jfrench)]

Adjusting bug title accordingly.

Comment 4

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

(In reply to Jonathan French (:jfrench) from comment #2)
> declared. In cases where timeout is set to 5000 but that constant is
> manipulated by some other var later in the same test, I will leave it in.

It would be better to ask us in those cases so we can make sure we fix it correctly. Otherwise sounds good!

Comment 5

[Jonathan French (:jfrench)]

Sounds good, I will check in, if I come across any where a timeout of 5000ms is being manipulated later in the script.

Comment 6

[Jonathan French (:jfrench)]

Starting to have a look at this bug now, while my patches from a variety of other bugs are pending review.

Comment 7

[Jonathan French (:jfrench)]

During related edits for timeout, also intend to adjust uses of waitForElement() where a default interval parameter of 100ms is used.

so something like this
const TIMEOUT = 5000;
controller.waitForElement(myElement, TIMEOUT, 100);

will become this
controller.waitForElement(myElement);

Comment 8

[Jonathan French (:jfrench)]

For clarity before any patches, here are all the files which contain TIMEOUT = 5000 in the Default branch that will be modified. I've done a first pass on updates locally. Two lib's I actually updated; they defined TIMEOUT but were never used.

All other lib's I left, since I assume they are what drive the harness defaults.

tests/functional/testAwesomeBar/testAccessLocationBar.js
tests/functional/testAwesomeBar/testPasteLocationBar.js
tests/functional/testBookmarks/testAddBookmarkToMenu.js
tests/functional/testFindInPage/testFindInPage.js
tests/functional/testFormManager/testClearFormHistory.js
tests/functional/testFormManager/testDisableFormManager.js
tests/functional/testFormManager/testSaveFormInformation.js
tests/functional/testLayout/testNavigateFTP.js
tests/functional/testPasswordManager/testPasswordNotificationBar.js
tests/functional/testPopups/testPopupsBlocked.js
tests/functional/testPreferences/testDefaultPhishingEnabled.js
tests/functional/testPreferences/testDefaultSecurityPrefs.js
tests/functional/testPreferences/testDisableCookies.js
tests/functional/testPreferences/testEnableCookies.js
tests/functional/testPreferences/testPasswordNotSaved.js
tests/functional/testPreferences/testPasswordSavedAndDeleted.js
tests/functional/testPreferences/testRemoveCookie.js
tests/functional/testPreferences/testRestoreHomepageToDefault.js
tests/functional/testSearch/testSearchSelection.js
tests/functional/testSecurity/testGreenLarry.js
tests/functional/testSecurity/testSafeBrowsingNotificationBar.js
tests/functional/testSecurity/testSafeBrowsingWarningPages.js
tests/functional/testSecurity/testSecurityInfoViaMoreInformation.js
tests/functional/testSecurity/testSSLDisabledErrorPage.js
tests/functional/testSecurity/testUnknownIssuer.js
tests/functional/testTabbedBrowsing/testOpenInBackground.js
tests/functional/testTechnicalTools/testAccessPageInfoDialog.js
tests/functional/testToolbar/testBackForwardButtons.js
(n/a) lib/addons.js
(n/a) lib/downloads.js
(n/a) lib/search.js
(n/a) lib/toolbars.js
lib/tabs.js **unused timeout, planning to remove
lib/tabview.js **unused timeout, planning to remove
lib/tests/testAddonsManager.js
lib/tests/testDownloads.js
lib/tests/testTabView.js
lib/tests/testToolbar.js
templates/testEmptyTest.js
templates/testModalDialog.js
templates/testPreferencesDialog.js
templates/testSharedModules.js

I will provide a preliminary patch shortly for Default. But even outside of any changes, I will likely need to do more resolves before landing, after patches for bug 848361 and bug 848649 are landed first.

Comment 9

[Jonathan French (:jfrench)]

Attachment: remove default timeouts (default) rev1.0 - wip

Patch "remove default timeouts (default) rev1.0" for the default branch. Thirty eight files modified.

This patch will likely need another resolve after patches are landed for bug 848361 and bug 848649, so not setting review flag at this time.

Tested with Default Nightly 22.0a1 20130325031100. All tests pass where expected.

testSecurity/testUnknownIssuer is a known skip, lib/tests/testDownloads and lib/tests/testTabView are known partial fails.

Both waitForElement() and waitForClick() were modified with these changes.

Two instances of controller.sleep() referencing a timeout were also removed from testFormManager/testDisableFormManager. This particular test was re-tested in succession about a dozen times, and it seems to be fine without them, at least on my windows platform.

Comment 10

[Andreea Matei [:AndreeaMatei]]

Jonathan, I think you forgot to add review request here :)

Comment 11

[Andreea Matei [:AndreeaMatei]]

Another note here, I don't see the removal for controller.waitThenClick() which also has default 5000ms. I looked because I'm reviewing bug 860382 (search.js lib) and I wasn't sure if your patch from here was landed yet or not.

Comment 12

[Jonathan French (:jfrench)]

(In reply to Andreea Matei [:AndreeaMatei] from comment #10)
> Jonathan, I think you forgot to add review request here :)

Ya, actually this was intentional, as per the bottom of Comment#7. We are trying to get bug 848649 landed first since it is double the size in affected files(~80), and I believe many of the files intersect this patch. So completing them in that order will greatly reduce the number of resolves.

Comment 13

[Jonathan French (:jfrench)]

(In reply to Andreea Matei [:AndreeaMatei] from comment #11)
> Another note here, I don't see the removal for controller.waitThenClick()
> which also has default 5000ms. I looked because I'm reviewing bug 860382
> (search.js lib) and I wasn't sure if your patch from here was landed yet or
> not.

For lib/search.js, I checked my local notes, and yes I consciously left them out of this first wip patch, listed as not applicable "(n/a)" in Comment#8 - "All other libs I left since I assume they are what drive the harness defaults". If that's not the case and they should be purged, let me know. :) There are 4 libs that would require removal, they are those listed in that comment, and excerpted below

(n/a) lib/addons.js
(n/a) lib/downloads.js
(n/a) lib/search.js
(n/a) lib/toolbars.js

Comment 14

[Jonathan French (:jfrench)]

(In reply to Jonathan French (:jfrench) from comment #12)
> So completing them in that order will greatly reduce the number of resolves.

I actually meant to write it will just make it a bit easier, since the patch for 848649 is so big, sync'd to default, and almost ready for landing. But whichever is fine, if we want to finish this bug first, and land that one second, we can do that also.

Comment 15

[Andreea Matei [:AndreeaMatei]]

Sorry I didn't read all the comment Jonathan. It's safe to remove those from libraries as well, it will not affect us, if are default.

I have no hurry in getting this landed first/after, was just wondering to know if I have to ask to get that removed in the other bug I was reviewing.

Comment 16

[Jonathan French (:jfrench)]

Cool, I will add in these 4 libs, and re-sync the patch against latest Default.

Comment 17

[Jonathan French (:jfrench)]

I had a look at the libs this evening. lib/downloads, lib/search seem to be clear enough case to remove timeout, they seem to have a very test-like usage of the const. 

However, lib/addons, lib/toolbars, seem a bit different. They use timeout in inline if statements, which seems to require some sort of fallback value. I'm unclear where these libs would get any timeout value, if it was removed and the object didn't pass in the parameter?

(lib/addons.js)
line 19: const TIMEOUT = 5000;

line 153: var timeout = (categoryID === "discover") ? TIMEOUT_REMOTE : TIMEOUT;
line 913: var timeout = (spec.timeout == undefined) ? TIMEOUT : spec.timeout;

* @param {object} aSpec
*        Object with parameters for customization
*        Elements: filter  - Filter element to wait for
*                  timeout - Duration to wait for the target state
*                            [optional - default: 5s]

Comment 18

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

(In reply to Jonathan French (:jfrench) from comment #17)
> (lib/addons.js)
> line 19: const TIMEOUT = 5000;
> 
> line 153: var timeout = (categoryID === "discover") ? TIMEOUT_REMOTE :
> TIMEOUT;

Instead of using TIMEOUT here we could leave the value as 'undefined' if we have a catergory different to discover. In that case waitFor picks the default value.

> line 913: var timeout = (spec.timeout == undefined) ? TIMEOUT : spec.timeout;

Same here. Just take `spec.timeout` without any further check. If it is undefined waitFor will handle it appropriately.

Comment 19

[Jonathan French (:jfrench)]

Thanks Henrik. I've made the changes locally and will do some test runs to ensure the before/after changes to these libs yield the same results, before uploading a patch.

Comment 20

[Jonathan French (:jfrench)]

I've done a variety of tests; running latest default tests as a baseline, vs. with the patch applied. About a half-dozen runs each with lib/tests. And similar for tests/functional. I am satisfied that the test results are consistent pre vs. post-patch, with these lib changes.

lib/tests/testSessionStore.js was behaving inconsistently run to run with each repo sometimes failing, sometimes passing; so that took some time to investigate and exclude as an issue. Unrelated but I will check bugzilla to see if that has been reported.

I will upload a patch shortly to remove timeout.

Comment 21

[Jonathan French (:jfrench)]

Attachment: remove default timeouts (default) rev1.1

Patch "remove default timeouts (default) rev1.1" for the default branch. Forty two files modified.

Major change in this patch rev, is the inclusion of these files for removal of timeout:

lib/addons.js
lib/downloads.js
lib/search.js
lib/toolbars.js

This patch has been synced to latest Default.

Tested with Default Nightly 23.0a1 20130422030937. All tests pass where expected.

Comment 22

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

Comment on attachment 740442 [details] [diff] [review]
remove default timeouts (default) rev1.1

Review of attachment 740442 [details] [diff] [review]:
-----------------------------------------------------------------

That's a fantastic patch. I love to see that we finally remove all that crap from former time. Hope that will make things easier to understand. Tested and works fine. 

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/e8a1fd262e65 (default)

Comment 23

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

We can let those changes ride the train. So no need for backports. Thanks again, Jonathan!