Closed Bug 1314474 Opened 3 years ago Closed 3 years ago

Enable no-shadow rule for eslint for browser/ mochitests

Categories

(Firefox :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 52
Tracking Status
firefox52 --- fixed

People

(Reporter: jaws, Assigned: jaws)

References

Details

Attachments

(1 file)

Enabling the no-shadow rule for all of m-c has thousands of errors, so this approach is to fix the no-shadow rule piecemeal.

The attached patch fixes the no-shadow rule for mochitests under /browser.
Comment on attachment 8806549 [details]
Bug 1314474 - Enable no-shadow rule for eslint for browser/ mochitests.

https://reviewboard.mozilla.org/r/89944/#review89444

Looks like I need to fix some more errors outside of /browser related to this rule.
Comment on attachment 8806549 [details]
Bug 1314474 - Enable no-shadow rule for eslint for browser/ mochitests.

https://reviewboard.mozilla.org/r/89944/#review89800

::: browser/base/content/test/general/browser_bug1297539.js:46
(Diff revision 2)
>    // Cut the current selection.
>    yield BrowserTestUtils.synthesizeKey("x", {accelKey: true}, browser);
>  
>    // The editor should be empty after cut.
>    yield ContentTask.spawn(browser, {elementQueryString, property},
> -    function* ({elementQueryString, property}) {
> +    function* ({contentElementQueryString, contentProperty}) {

This won't work, you need:

    function* ({elementQueryString: contentElementQueryString, property: contentProperty})
    
Which sure is ugly!

::: browser/base/content/test/general/contextmenu_common.js:262
(Diff revision 2)
>    }
>  
>    if (!options.skipFocusChange) {
>      yield ContentTask.spawn(gBrowser.selectedBrowser,
>                              {lastElementSelector, selector},
> -                            function*({lastElementSelector, selector}) {
> +                            function*({contentLastElementSelector, contentSelector}) {

Same issue as above

::: security/manager/ssl/tests/mochitest/stricttransportsecurity/test_sts_privatebrowsing_perwindowpb.html:129
(Diff revision 2)
>    function loadVerifyFrame(win, isPrivate, test, testName, aCallback) {
>      let id = 'ifr_' + testName;
>      let src = test.url + '?id=' + testName;
>      let browser = win.gBrowser.selectedBrowser;
>  
> -    ContentTask.spawn(browser, [id, src], function* ([id, src]) {
> +    ContentTask.spawn(browser, [id, src], function* ([contentId, contentSrc]) {

This is probably the better way to do it.

::: toolkit/components/alerts/test/test_multiple_alerts.html:19
(Diff revision 2)
>  const Ci = SpecialPowers.Ci;
>  
>  const chromeScript = SpecialPowers.loadChromeScript(_ => {
> -  const { classes: Cc, interfaces: Ci, utils: Cu } = Components;
> +  Components.utils.import("resource://gre/modules/Services.jsm");
> +  Components.utils.import("resource://gre/modules/Timer.jsm");
>  

:(

::: toolkit/components/passwordmgr/test/browser/browser_http_autofill.js:26
(Diff revision 2)
>  add_task(function* test_http_autofill() {
>    for (let scheme of ["http", "https"]) {
>      let tab = yield BrowserTestUtils
>        .openNewForegroundTab(gBrowser, `${scheme}${TEST_URL_PATH}form_basic.html`);
>  
>      let {username, password} = yield ContentTask.spawn(gBrowser.selectedBrowser, null, function* () {

Another problem here.

::: toolkit/components/passwordmgr/test/browser/browser_http_autofill.js:45
(Diff revision 2)
>  add_task(function* test_iframe_in_http_autofill() {
>    for (let scheme of ["http", "https"]) {
>      let tab = yield BrowserTestUtils
>        .openNewForegroundTab(gBrowser, `${scheme}${TEST_URL_PATH}form_basic_iframe.html`);
>  
>      let {username, password} = yield ContentTask.spawn(gBrowser.selectedBrowser, null, function* () {

And here

::: toolkit/components/passwordmgr/test/browser/browser_http_autofill.js:65
(Diff revision 2)
>  add_task(function* test_http_action_autofill() {
>    for (let type of ["insecure", "secure"]) {
>      let tab = yield BrowserTestUtils
>        .openNewForegroundTab(gBrowser, `https${TEST_URL_PATH}form_cross_origin_${type}_action.html`);
>  
>      let {username, password} = yield ContentTask.spawn(gBrowser.selectedBrowser, null, function* () {

And here

::: toolkit/components/passwordmgr/test/browser/browser_notifications.js:44
(Diff revision 2)
>        // case. This will cause the doorhanger notification to be displayed.
>        let promiseShown = BrowserTestUtils.waitForEvent(PopupNotifications.panel,
>                                                         "popupshown",
>                                                         (event) => event.target == PopupNotifications.panel);
>        yield ContentTask.spawn(browser, { username, password },
> -        function* ({ username, password }) {
> +        function* ({ contentUsername, contentPassword }) {

And here
Attachment #8806549 - Flags: review-
Comment on attachment 8806549 [details]
Bug 1314474 - Enable no-shadow rule for eslint for browser/ mochitests.

https://reviewboard.mozilla.org/r/89944/#review90428
Attachment #8806549 - Flags: review?(dtownsend) → review+
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2a1b034ed3fb
Enable no-shadow rule for eslint for browser/ mochitests. r=mossop
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b885dbd862e5
Enable no-shadow rule for eslint for browser/ mochitests. r=mossop
https://hg.mozilla.org/mozilla-central/rev/b885dbd862e5
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
You need to log in before you can comment on or make changes to this bug.