848653 - Clean up non capitalized constants, purge gTimeout,gDelay

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Jonathan French (:jfrench)]

During other work I noticed a few un-capitalized or otherwise unconventional constant names, in files in the repo. I searched with a little regex to find all the files in the repo that aren't at a minimum all capitalized, or are interCapped.

They are:

tests/functional/testAwesomeBar/testLocationBarSearches.js
tests/functional/testInstallation/testBreakpadInstalled.js
tests/functional/testLayout/testNavigateFTP.js
tests/functional/testPopups/testPopupsAllowed.js
tests/functional/testPopups/testPopupsBlocked.js
tests/functional/testPreferences/testDefaultPhishingEnabled.js
tests/functional/testPreferences/testDefaultSecurityPrefs.js
tests/functional/testPreferences/testSwitchPanes.js
tests/functional/testSearch/testAddMozSearchProvider.js
tests/functional/testSearch/testRemoveSearchEngine.js
tests/functional/testSearch/testReorderSearchEngines.js
tests/functional/testSecurity/testSafeBrowsingNotificationBar.js
tests/functional/testSecurity/testSafeBrowsingWarningPages.js
tests/functional/testSecurity/testSSLDisabledErrorPage.js
tests/functional/testSecurity/testUnknownIssuer.js
tests/functional/testTabbedBrowsing/testBackgroundTabScrolling.js
tests/functional/testTabbedBrowsing/testCloseTab.js
tests/functional/testTabbedBrowsing/testOpenInForeground.js
lib/downloads.js
lib/tests/testDownloads.js
lib/tests/testSearch.js
lib/utils.js
lib/widgets.js

There may be some that are ALLCAPS, but aren't seprarated by an underscore where they should be ALL_CAPS. But I couldn't think of a clever way to find those.

If you'd like me to fix these known ones, feel free to assign me and I'll be happy to do it.

Comment 1

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

Absolutely! Would be great to see the same style for constant names. Thanks Jonathan! I will assign it to you.

Comment 2

[Jonathan French (:jfrench)]

Sure, no problems. I will look at this after or in-between work on 2-3 other bugs I'm currently looking at.

Comment 3

[Jonathan French (:jfrench)]

Attachment: clean up non-cap constants (default)

Patch "clean up non-cap constants (default)" for the default branch. Twenty three files affected.

There were instances of constants gTimeout,gDelay which were not used in some tests. They have been capitalized, but potential removal from the tests altogether can be addressed in bug 848200.

There were some cases where a path was being assembled again in controller.open(). Those were refactored into the standard LOCAL_TEST_LOCATION, LOCAL_TEST_PAGE structure near the top of the test, similar to the work done in bug 848361. Related to this - it might be an idea to review and land that patch for that bug first, before landing this one. A few of the files intersect both patches and this patch adds a few new lines, in places. But I could tweak that other patch if needed if this is landed first.

Particular attention in the review should be paid to download.js in this bug's review. 'downloadState' appeared to be getting used syntactically as both a constant and a property. With this change, the constant is updated to ALL_CAPS, and the property remains unchanged. As you can see below, the key pair constant got used for a property of the same name, for the window object when the constructor is called. It is also used again at the bottom of the module to provide the same array values for the test lib/tests/testDownloads.js, when it is run and the downloadState is recorded. I made a point to confirm this by intentionally breaking the module, by altering the property value and running the test.

downloads.js (post change)

(line30)
const DOWNLOAD_STATE = {
  notStarted      : -1,
  downloading     : 0,
  finished        : 1,
  ......etc

/**
 * Constructor
 */
function downloadManager() {
  this._controller = null;
  this.downloadState = DOWNLOAD_STATE;
}

(line 462)
// Export of variables
exports.downloadState = DOWNLOAD_STATE;


Tested with Default Nightly 22.0a1 20130309030841. Tests pass where expected. lib/tests/testDownloads contains one known fail, and tests/functionality/testSecurity/testUnknownIssuer in its entirety, is a known skip.

Let me know if there's anything you'd like adjusted.

Comment 4

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 723208 [details] [diff] [review]
clean up non-cap constants (default)

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

Awesome work Jonathan! So glad to see such consistency in our tests, thanks!

There were some cases where we had timeout and delay consts that were not being used at all in the test so we can safely remove them.
After removing those we'll be good to land!

::: lib/downloads.js
@@ +453,1 @@
>                                 .QueryInterface(Ci.nsIFileURL);

Can you please move this to the right so the dots are aligned?

::: lib/tests/testSearch.js
@@ +8,5 @@
>  var search = require("../search");
>  var utils = require("../utils");
>  
> +const G_DELAY = 0;
> +const G_TIMEOUT = 5000;

I don't see these being used in the code, don't know why were here. So we can remove G_TIMEOUT, use G_DELAY = 500 and replace 500 in the sleep calls from the test.

::: lib/utils.js
@@ +15,5 @@
>  // Include required modules
>  var { assert } = require("assertions");
>  var prefs = require("prefs");
>  
> +const G_TIMEOUT = 5000;

It's not being used here either.

::: lib/widgets.js
@@ +9,5 @@
>  
>  var EventUtils = {};
>  Cu.import('resource://mozmill/stdlib/EventUtils.js', EventUtils);
>  
> +const G_TIMEOUT = 5000;

Same here.

::: tests/functional/testAwesomeBar/testLocationBarSearches.js
@@ +6,5 @@
>  var { assert } = require("../../../lib/assertions");
>  var prefs = require("../../../lib/prefs");
>  var toolbars = require("../../../lib/toolbars");
>  
> +const G_TIMEOUT = 5000;

We can remove this now.

::: tests/functional/testInstallation/testBreakpadInstalled.js
@@ +12,4 @@
>                     "darwin" : "crashreporter.app",
>                     "win32"  : "crashreporter.exe",
>                     "linux"  : "crashreporter"
>                    };

This needs to be aligned with the opening one please and the elements moved a space to the right.

::: tests/functional/testPopups/testPopupsAllowed.js
@@ +13,5 @@
>  
>  const PREF_POPUP_BLOCK = "dom.disable_open_during_load";
>  
> +const G_DELAY = 0;
> +const G_TIMEOUT = 5000;

These two are not being used here, we can remove them.

::: tests/functional/testPopups/testPopupsBlocked.js
@@ +13,4 @@
>  
>  const PREF_POPUP_BLOCK = "dom.disable_open_during_load";
>  
> +const G_DELAY = 0;

We can remove Delay const is not used in the test.

::: tests/functional/testPreferences/testDefaultPhishingEnabled.js
@@ +5,5 @@
>  // Include necessary modules
>  var { expect } = require("../../../lib/assertions");
>  var prefs = require("../../../lib/prefs");
>  
> +const G_DELAY = 0;

Same here.

::: tests/functional/testPreferences/testDefaultSecurityPrefs.js
@@ +5,5 @@
>  // Include necessary modules
>  var { expect } = require("../../../lib/assertions");
>  var prefs = require("../../../lib/prefs");
>  
> +const G_DELAY = 0;

And here.

::: tests/functional/testSearch/testAddMozSearchProvider.js
@@ +12,5 @@
>  
>  const TIMEOUT_INSTALL_DIALOG = 30000;
>  
> +const SEARCH_ENGINE = {name: "mozqa.com",
> +                      url : LOCAL_TEST_FOLDER + "search/mozsearch.html"};

Please move this one space to the right so it's aligned with name.

::: tests/functional/testSearch/testRemoveSearchEngine.js
@@ +6,5 @@
>  var { assert, expect } = require("../../../lib/assertions");
>  var search = require("../../../lib/search");
>  
> +const G_DELAY = 0;
> +const G_TIMEOUT = 5000;

We can remove them as they are not being used

::: tests/functional/testSearch/testReorderSearchEngines.js
@@ +6,5 @@
>  var { expect } = require("../../../lib/assertions");
>  var search = require("../../../lib/search");
>  
> +const G_DELAY   = 0;
> +const G_TIMEOUT = 5000;

G_TIMEOUT is not used.

::: tests/functional/testSecurity/testSSLDisabledErrorPage.js
@@ +10,5 @@
>  const PREF_KEEP_ALIVE = "network.http.keep-alive";
>  const PREF_SSL_3 = "security.enable_ssl3";
>  const PREF_TLS = "security.enable_tls";
>  
> +const G_DELAY = 0;

G_DELAY is not being used

::: tests/functional/testSecurity/testSafeBrowsingNotificationBar.js
@@ +6,5 @@
>  var { assert, expect } = require("../../../lib/assertions");
>  var tabs = require("../../../lib/tabs");
>  var utils = require("../../../lib/utils");
>  
> +const G_DELAY = 0;

This can be removed.

::: tests/functional/testSecurity/testSafeBrowsingWarningPages.js
@@ +8,5 @@
>  var tabs = require("../../../lib/tabs");
>  var utils = require("../../../lib/utils");
>  
>  
> +const G_DELAY = 0;

Here as well.

::: tests/functional/testSecurity/testUnknownIssuer.js
@@ +4,5 @@
>  
>  // Include necessary modules
>  var { assert, expect } = require("../../../lib/assertions");
>  
> +const G_DELAY = 0;

Same here.

::: tests/functional/testTabbedBrowsing/testCloseTab.js
@@ +6,5 @@
>  var { assert } = require("../../../lib/assertions");
>  var tabs = require("../../../lib/tabs");
>  
> +const G_DELAY = 0;
> +const G_TIMEOUT = 5000;

We can remove these 2 here.

::: tests/functional/testTabbedBrowsing/testOpenInForeground.js
@@ +13,5 @@
>  
>  const PREF_TAB_LOAD_IN_BACKGROUND = "browser.tabs.loadInBackground";
>  
> +const G_DELAY = 0;
> +const G_TIMEOUT = 5000;

And here as well.

Comment 5

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

I just want to add that we can even remove those timeouts which are 5s and still in use for waitForElement. 5s is the default used internally so we can strip those off too.

Comment 6

[Andreea Matei [:AndreeaMatei]]

Thanks Henrik. 
For waitThenClick() as well, I've updated the mdn page with this timeout:
https://developer.mozilla.org/en-US/docs/Mozmill/Mozmill_Element_Object

Comment 7

[Jonathan French (:jfrench)]

(In reply to Andreea Matei [:AndreeaMatei] from comment #4)
> Comment on attachment 723208 [details] [diff] [review]
> clean up non-cap constants (default)
> 
> Review of attachment 723208 [details] [diff] [review]:
> -----------------------------------------------------------------

Sure, no problems Andreea. I wasn't sure how aggressive we wanted to be with purging the unused gdelay,gTimeout during the same work, I had originally thought we might do that as part of the purge of unused 5000ms timeouts in bug 848200. Sort of capitalize everything first, then purge. But no problems, I will do that with this change. I will make all the other tweaks as described.

If it helps you speeding up later reviews, you can always say 'remove unused x,y or z' in this type of scenario if it saves you time itemizing each, I will find them all no problems. Thanks for the other subtlety ones though, I will address those as described.

I'm inclined to leave the used 5000ms timeouts (default waitForElement), and address their purge in bug 848200? If that's ok? But if you guys would prefer I do it here in this bug I will, no problems.

Comment 8

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

We might want to do it the other way around. I don't see a reason why to modify constants which will be removed right after. So I would say get bug 848200 fixed first. :)

Comment 9

[Jonathan French (:jfrench)]

Cool, no probs :) I've already started on the updates to this patch including purging unused gDelay,gTimeout, so I'll upload a new patch for this bug shortly, and then do the other bug also. Whichever patch gets landed first, I'll do a new pull, and do any resolves if needed and freshen up the 'other' patch for you. 

That other bug initial intent, was to purge unused occurrences of 'regular' timeout, rather than gTimeout. Though there could be some other variants I've yet to encounter, and may find during that work :)

Comment 10

[Jonathan French (:jfrench)]

Attachment: clean up non-cap constants (default) rev 1.1

Patch "clean up non-cap constants (default) rev1.1" for the default branch. Twenty three files affected.

Instances of former constants gTimeout,gDelay were removed where unused, as per Comment #4 review. testPreferences/testDefaultSecurityPrefs actually references G_DELAY in the test, though its initial value is set to 0, so it was left in as part of the fix for now. Whitespace adjusted as specified. I should have caught those. For some reason I mapped an equivalent number of characters in my head, but in some cases they were altering the line length.

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

This patch supersedes the original for default, so marking that one as obsolete.

Comment 11

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 724048 [details] [diff] [review]
clean up non-cap constants (default) rev 1.1

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

Great, it's fine by me since you already started to work on this. Looks good. 
Looking forward for the fix in the other bug as well :)

Landed on default:
http://hg.mozilla.org/qa/mozmill-tests/rev/28b60116c6fb

Comment 12

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

Comment on attachment 724048 [details] [diff] [review]
clean up non-cap constants (default) rev 1.1

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

::: lib/downloads.js
@@ +17,5 @@
>  var domUtils = require("dom-utils");
>  var prefs = require("prefs");
>  var utils = require("utils");
>  
> +const G_TIMEOUT = 5000;

While this is fine the 'G_' prefix would not have been necessary here. We started to use the hungarian notation at the beginning but then decided otherwise. Can we do a follow-up for those tests which are affected before we backport?

Comment 13

[Jonathan French (:jfrench)]

Absolutely. I will replace all occurrences of gTimeout(or G_TIMEOUT) with TIMEOUT in default for this bug, with another patch. Then when backporting I gather I will combine both patches, into a single patch for aurora, beta, release, esr17.

Comment 14

[Jonathan French (:jfrench)]

Attachment: clean up non-cap constants (default) supplementary - rev 2.0

Patch "clean up non-cap constants (default) supplementary - rev 2.0" for the default branch. Ten files modified. 

Converted all occurrences of G_TIMEOUT to TIMEOUT. Once all the many changes for various bugs have settled down across all repos (particularly bug 848649), I will later remove instances of TIMEOUT like these, where the wait is the default 5000ms, in bug 848200.

Tested with Default Nightly 22.0a1 20130313031041. Tests pass where expected.

Tests testSecurity/testUnknownIssuer is a known skip, lib/tests/testDownloads is a known partial fail (2pass,1fail).

This patch is supplementary to the earlier patch rev1.1 for default, not a replacement, it should be landable as it is resolved against a new pull.

For backports to aurora,beta,release,esr17 if applicable, a unified patch will be generated.

Comment 15

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 724439 [details] [diff] [review]
clean up non-cap constants (default) supplementary - rev 2.0

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

While the work for TIMEOUT is complete, I would like to see the same for G_DELAY before landing this.

::: lib/tests/testDownloads.js
@@ +4,5 @@
>  
>  // Include required modules
>  var downloads = require("../downloads");
>  
>  const G_DELAY = 0;

Would you mind changing this constant as well with DELAY?
Applies to all occurrences in the repository.

Comment 16

[Jonathan French (:jfrench)]

Sure, no problems. I will convert all occurrences of G_DELAY to DELAY. Will have a patch shortly. 

I'll want to do another resolve though before uploading, after a supplementary patch for bug 848361 is landed first. Just to be sure this patch will land cleanly on top. Some files intersect both bugs.

Comment 17

[Jonathan French (:jfrench)]

Attachment: clean up non-cap constants (default) supplementary - rev 2.1

Patch "clean up non-cap constants (default) supplementary - rev 2.1" for the default branch. Thirteen files modified. 

Converted all occurrences of G_TIMEOUT,G_DELAY, and TIMEOUT,DELAY. 

Removed G_DELAY completely from lib/tests/testDownloads, as it was unused.

Once all the many changes for various bugs have settled down across all repos (particularly bug 848649), I will later remove instances of TIMEOUT like these, where the wait is the default 5000ms, in bug 848200.

Tested with Default Nightly 22.0a1 20130314030914. Tests pass where expected.

Tests testSecurity/testUnknownIssuer is a known skip, lib/tests/testDownloads is a known partial fail (2pass,1fail).

This patch is supplementary to the earlier patch rev1.1 for default, not a replacement. Perhaps wait to try landing this patch, after bug 848361's patch has been landed on default, some files may intersect. If there's any issues I will resolve either of them as needed.

For backports to aurora,beta,release,esr17 if applicable, a unified patch will be generated.

Comment 18

[Jonathan French (:jfrench)]

(In reply to Jonathan French (:jfrench) from comment #17)
> 
> Converted all occurrences of G_TIMEOUT,G_DELAY, and TIMEOUT,DELAY. 

Meant to write "to TIMEOUT,DELAY", here.

Comment 19

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 724955 [details] [diff] [review]
clean up non-cap constants (default) supplementary - rev 2.1

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

Awesome, we're good for default now.
Just that the patch is no longer applying, probably because of landing from bug 848361. 
Would you mind saving the patch with .patch extension?

applying patchBug848653_Default_rev2_1.txt
patching file tests/functional/testSecurity/testSSLDisabledErrorPage.js
Hunk #1 FAILED at 5
Hunk #2 FAILED at 47
2 out of 2 hunks FAILED -- saving rejects to file tests/functional/testSecurity/testSSLDisabledErrorPage.js.rej
patching file tests/functional/testSecurity/testUnknownIssuer.js
Hunk #1 FAILED at 0
1 out of 1 hunks FAILED -- saving rejects to file tests/functional/testSecurity/testUnknownIssuer.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh patchBug848653_Default_rev2_1.txt

Comment 20

[Jonathan French (:jfrench)]

Sure, makes sense with that other new patch just landed. Will also use .patch no problems.

Comment 21

[Jonathan French (:jfrench)]

Attachment: clean up non-cap constants (default) supplementary - rev 2.2

Patch "clean up non-cap constants (default) supplementary - rev 2.2" for the default branch. Thirteen files modified.

Converted all occurrences of G_TIMEOUT,G_DELAY, to TIMEOUT,DELAY.

Additional resolves were anticipated, now completed. 

Tested with Default Nightly 22.0a1 20130315030943. Tests pass where expected.

This patch is supplementary to the earlier patch rev1.1 for default, not a replacement. For backports to aurora,beta,release,esr17 if applicable, a unified patch will be generated.

This patch supersedes the earlier rev2.1 for default, so marking that one as obsolete.

Comment 22

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 725398 [details] [diff] [review]
clean up non-cap constants (default) supplementary - rev 2.2

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

Great! Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/36eab0ecd471 (default)

Looking forward to backporting :)

Thanks!

Comment 23

[Jonathan French (:jfrench)]

Cool, will start that early this week.

Comment 24

[Jonathan French (:jfrench)]

Attachment: clean up non-cap constants (aurora) rev 1.0

Patch "clean up non-cap constants (aurora) - rev 1.0" for the Aurora branch. Twenty five files modified.

This patch represents the union of the original two patches (rev1.1,supplementary2.2) landed earlier on default. However two other files, not in the initial 'scope' for capitalization fixes, were also lurking in the repo with unwanted gTimeout,gDelay. They have been corrected and included with this Aurora patch. 

Those two files which are listed below, will require an additional supplementary patch for Default.

lib\software-update.js
tests\functional\testSecurity\testIdentityPopupOpenClose.js

Tested with Aurora 21.0a2 20130317042012. All tests pass where expected. lib/tests/testDownloads is a known partial-fail, testSecurity/testUnknownIssuer is a known skip.

I will provide that additional supplementary patch (supplementary rev3.0) for Default, shortly.

Comment 25

[Jonathan French (:jfrench)]

Attachment: clean up non-cap constants (default) supplementary - rev3.0

Patch "clean up non-cap constants (default) supplementary - rev 3.0" for the Default branch. Two files modified.

Two files not in the initial 'scope' for capitalization fixes, were lurking in the repo with unwanted gTimeout,gDelay. They have been corrected and included in this supplementary patch for Default only.

lib\software-update.js
tests\functional\testSecurity\testIdentityPopupOpenClose.js

Tested with Default 22.0a1 20130315030943. Test passes as expected.

Comment 26

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 726384 [details] [diff] [review]
clean up non-cap constants (default) supplementary - rev3.0

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

Pushed to default:
http://hg.mozilla.org/qa/mozmill-tests/rev/3ebf88afaff3

Comment 27

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 726369 [details] [diff] [review]
clean up non-cap constants (aurora) rev 1.0

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

Great to have a full patch :)
http://hg.mozilla.org/qa/mozmill-tests/rev/93be043ec5c7 (aurora)

Comment 28

[Jonathan French (:jfrench)]

Attachment: clean up non-cap constants (beta) rev 1.0

Patch "clean up non-cap constants (beta) - rev 1.0" for the Beta branch. Twenty five files modified.

This patch represents the union of the original patches landed earlier on default, and then unified in Aurora.

There were additional resolves required for Beta. No additional occurrences of gDelay,gTimeout or non-capitalized constants, unique to Beta, existed.

Tested with Beta 20.0 20130313170052. Tests pass where expected.

Comment 29

[Jonathan French (:jfrench)]

Attachment: clean up non-cap constants (release) rev 1.0

Patch "clean up non-cap constants (release) - rev 1.0" for the Release branch. Twenty nine files modified.

This patch represents the original patches landed earlier on default, then unified in Aurora,Beta.

There were additional resolves required for Release. Also, four files unique to Release required updates. They are:

tests/crash/testRemoveCookieAfterPrivateBrowsing.js
tests/functional/testPrivateBrowsing/testDisabledElements.js
tests/functional/testPrivateBrowsing/testGeolocation.js
lib/private-browsing.js

Tested with Release 19.0.2 20130307023931. Tests pass where expected.

Comment 30

[Jonathan French (:jfrench)]

Attachment: clean up non-cap constants (esr17) rev 1.0

Patch "clean up non-cap constants (esr17) - rev 1.0" for the Esr17 branch. Thirty files modified.

This patch represents the backport of the Release patch(which itself is a rollup of Default,Beta,Aurora patches), to Esr17.

No resolves were required, however one file unique to Esr17 required update, to purge gDelay,gTimeout. It is:

testSecurity/testEncryptedPageWarning

Tested with Esr17 Nightly 17.0.4esrpre 20130317034503. Tests pass where expected.

Comment 31

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 726911 [details] [diff] [review]
clean up non-cap constants (esr17) rev 1.0

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

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/de3ca37ad9ce (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/f3241487f946 (release)
http://hg.mozilla.org/qa/mozmill-tests/rev/ecdf2a62f0fd (esr17)

Thanks Jonathan, great work! We're done here :)