Closed Bug 1550674 Opened 4 months ago Closed 4 months ago

Turn on ESLint in mail/test/mozmill

Categories

(Thunderbird :: General, task)

task
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 68.0

People

(Reporter: darktrojan, Assigned: darktrojan)

References

Details

Attachments

(2 files, 1 obsolete file)

No description provided.
Attached patch 1550674-mozmill-prefs-1.diff (obsolete) — Splinter Review

This should be an easy read compared to some of the other stuff. Long, but it's mostly basic formatting.

I've tried to standardise the top of the test files as well, so it's

  • MPL licence
  • "use strict"
  • linting imports
  • MODULE_NAME
  • RELATIVE_ROOT
  • MODULE_REQUIRES

in all files. I got rid of a few function aliases as they're rarely used and it makes importing much easier. Also some testing of plugins code is obsolete now that we don't have plugins.

Attachment #9063992 - Flags: review?(acelists)

In this patch I've taught the test runner to make prefs files instead of having a lot of cut-and-paste boilerplate code in the directories that need prefs.

Attachment #9063993 - Flags: review?(acelists)

You attached the same patch twice, right?

Flags: needinfo?(geoff)

Apparently I did.

Flags: needinfo?(geoff)
Attachment #9063992 - Attachment is obsolete: true
Attachment #9063992 - Flags: review?(acelists)
Attachment #9064242 - Flags: review?(acelists)
Comment on attachment 9063993 [details] [diff] [review]
1550674-mozmill-prefs-1.diff

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

I like this, we already had to rewrite this at least once as m-c changed the way how to provide the prefs in a separate file.
Attachment #9063993 - Flags: review?(acelists) → review+
Comment on attachment 9064242 [details] [diff] [review]
1550674-eslint-mozmill-1.diff

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

Thanks!

::: mail/test/mozmill/attachment/test-attachment-in-plain-msg.js
@@ -1,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * 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/. */
>  
> -// make SOLO_TEST=attachment/test-attachment-in-plain-msg.js mozmill-one

I think Magnus adds these lines so you should ask him about the removal.

::: mail/test/mozmill/composition/test-forward-headers.js
@@ +28,5 @@
>  var cwc = null; // compose window controller
>  var folder;
>  var gDrafts;
>  
> +var setupModule = function(module) {

Can you please mass-replace these with 'function setupModule(module)' while touching them?

::: mail/test/mozmill/content-policy/test-plugins-policy.js
@@ +176,5 @@
>    assert_selected_and_displayed(0);
>  
>    // Now check that the content hasn't been loaded
> +  if (isPluginLoaded(mozmill.getMail3PaneController().window.content.document))
> +    throw new Error("Plugin has not been blocked in message as expected");

Nice :)

::: mail/test/mozmill/message-header/test-reply-identity.js
@@ +99,5 @@
>    account.addIdentity(identity);
>    account.addIdentity(identity2);
>  }
>  
> +var checkReply = function(replyWin, expectedFromEmail) {

Could be a normal function?

::: mail/test/mozmill/newmailaccount/test-newmailaccount.js
@@ -45,5 @@
>  var gProvisionerEnabled = Services.prefs.getBoolPref(kProvisionerEnabledPref);
> -
> -// Record what the original value of the mail.provider.enabled pref is so
> -// that we can put it back once the tests are done.
> -var gProvisionerEnabled = Services.prefs.getBoolPref(kProvisionerEnabledPref);

A pasto:)

::: mail/test/mozmill/shared-modules/test-nntp-helpers.js
@@ +141,5 @@
>  
>    return server;
>  }
>  
>  var URLCreator = MailServices.nntp.QueryInterface(Ci.nsIProtocolHandler);

Is this used now?

::: mail/test/mozmill/shared-modules/test-notificationbox-helpers.js
@@ +126,3 @@
>        return (box.getNotificationWithValue(aValue) != null) && !box._animating;
>      }
> +    return null;

Or return false?

@@ +129,2 @@
>    }
>    aController.waitFor(nbReady,

Interesting this works without '() =>'.

::: mail/test/mozmill/shared-modules/test-window-helpers.js
@@ +379,5 @@
> +      return (this.waitingForOpen == null && this.monitorizeClose());
> +    },
> +    "Timeout waiting for modal dialog to open.",
> +    aTimeout || WINDOW_OPEN_TIMEOUT_MS,
> +    WINDOW_OPEN_CHECK_INTERVAL_MS, this);

This indent looks wrong.

::: mail/test/mozmill/tabmail/test-tabmail-dragndrop.js
@@ +188,5 @@
>                        mc.tabmail.tabContainer);
>  
>    wait_for_message_display_completion(mc2);
>  
> +  assert_true(!!(mc.tabmail.tabContainer.childNodes.length == 1),

What do the !! do? It seems the expressions are already boolean.
Attachment #9064242 - Flags: review?(acelists) → review+

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/0ff27072766a
Turn on ESLint in mail/test/mozmill; r=aceman
https://hg.mozilla.org/comm-central/rev/b800b4a19b7f
Simplify setting of prefs for mozmill tests; r=aceman

Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED

Landed with requested changes.

Target Milestone: --- → Thunderbird 68.0
You need to log in before you can comment on or make changes to this bug.