876503 - cleanup headers of TB mozmill tests (mail/test/mozmill/)

Status: RESOLVED FIXED

(Thunderbird :: Testing Infrastructure, enhancement)

Description

[:aceman]

Some of the mozmill tests contain unneded declarations in their headers and the needed ones are in various orders.

I propose this order:
const MODULE_NAME = '...';

const RELATIVE_ROOT = '../shared-modules';
const MODULE_REQUIRES = [...];

<other module imports - Cu.import() >

<this module constants, variables, setupModule, etc.>

Many of the files declare Components.{classes, interfaces, utils} which are told to be unneeded. So I try to clean up something.

This does not change the logic of including modules in any way. For those see bug 876250 and bug 876089.

(Inspired by the investigations of Tessarakt.)

Comment 1

[:aceman]

Attachment: patch for shared-modules

So let's start with the shared-modules files to see how it works.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 754560 [details] [diff] [review]
patch for shared-modules

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

::: mail/test/mozmill/shared-modules/test-cloudfile-helpers.js
@@ +5,1 @@
>  const MODULE_NAME = 'cloudfile-helpers';

What about RELATIVE_ROOT and MODULE_REQUIRES? Why were those removed?

::: mail/test/mozmill/shared-modules/test-cloudfile-ubuntuone-helpers.js
@@ +5,2 @@
>  const MODULE_NAME = 'cloudfile-ubuntuone-helpers';
>  

Aren't RELATIVE_ROOT and MODULE_REQUIRES expected? Or are they optional?

::: mail/test/mozmill/shared-modules/test-mock-object-helpers.js
@@ +134,5 @@
>  /**
>   * Swiped from mozilla/testing/mochitest/tests/SimpleTest/specialpowersAPI.js
>   */
>  function swapFactoryRegistration(cid, contractID, newFactory, oldFactory) {
> +  var componentRegistrar = Cm.QueryInterface(Ci.nsIComponentRegistrar);

Replace this var with let

Comment 3

[:aceman]

(In reply to Mike Conley (:mconley) from comment #2)
> Comment on attachment 754560 [details] [diff] [review]
> patch for shared-modules
> 
> Review of attachment 754560 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mail/test/mozmill/shared-modules/test-cloudfile-helpers.js
> @@ +5,1 @@
> >  const MODULE_NAME = 'cloudfile-helpers';
> 
> What about RELATIVE_ROOT and MODULE_REQUIRES? Why were those removed?
I'll check.

> ::: mail/test/mozmill/shared-modules/test-cloudfile-ubuntuone-helpers.js
> @@ +5,2 @@
> >  const MODULE_NAME = 'cloudfile-ubuntuone-helpers';
> >  
> Aren't RELATIVE_ROOT and MODULE_REQUIRES expected? Or are they optional?
It looks to me they are optional.

Comment 4

[:aceman]

Attachment: patch for shared-modules v2

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 763282 [details] [diff] [review]
patch for shared-modules v2

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

r=me with my nits fixed. Thanks aceman!

::: mail/test/mozmill/shared-modules/test-cloudfile-backend-helpers.js
@@ +30,5 @@
>    module.SimpleRequestObserver = SimpleRequestObserver;
>    module.assert_can_cancel_uploads = assert_can_cancel_uploads;
>  }
>  
>  function setupModule(module) {

Splinter doesn't let me drill around the rest of this file, but I'll assume you checked and ensured that fdh is never actually used, which is why you removed it.

::: mail/test/mozmill/shared-modules/test-content-tab-helpers.js
@@ +370,5 @@
>   * Returns the nsIPluginTag for the test plug-in, if it is available.
>   * Returns null otherwise.
>   */
>  function get_test_plugin() {
> +  var ph = Cc["@mozilla.org/plugin/host;1"].getService(Ci.nsIPluginHost);

let, not var

::: mail/test/mozmill/shared-modules/test-message-helpers.js
@@ +18,1 @@
>  function setupModule() {

We can remove this empty function too while we're at it.

@@ +19,5 @@
>    // do nothing
>  }
>  
>  function installInto(module) {
>    setupModule();

And this call to the empty function.

Comment 6

[:aceman]

Attachment: patch for shared-modules v3 [checkin: comment 10]

Thanks, done.

Comment 7

[:aceman]

Aryx, please create a try run with this patch. All systems, mozmill test only.

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Pushed to Thunderbird-Try as https://tbpl.mozilla.org/?tree=Thunderbird-Try&showall=1&rev=a19096919ef7

Comment 9

[:aceman]

Thanks, I see no new failures in the log so it can land.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/c88e23a09a07

Comment 11

[:aceman]

Attachment: remove unneeded includes in shared-modules

This removes some unneeded includes in the mozmill/shared-modules files. Notice this does not go against discussion in bug 876250. This does not yet remove imports of modules that are actually referenced in a file (even though those are unneeded too per bug 876250).

Comment 12

[:aceman]

Attachment: small tweaks to tests

While testing the previous patch I noticed some failures not caused by that patch but by the order in which tests are executed. Some tests were failing because a previous test didn't clean up after itself.

This patch also cleans up some module headers if they used unusual constructs to be in line with the commonly used style.

Comment 13

[:aceman]

Aryx, could you push each of these 2 patches to TB-try separately? mozmill tests are enough, thanks.

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Pushed to Thunderbird-Try:
unnecessary includes removed: https://tbpl.mozilla.org/?tree=Thunderbird-Try&showall=1&rev=07f2ad5a46af
small tweaks: https://tbpl.mozilla.org/?tree=Thunderbird-Try&showall=1&rev=10cf0213a3a5

Comment 15

[:aceman]

Thanks, both runs are green.

Comment 16

[Magnus Melin [:mkmelin]]

Comment on attachment 8379223 [details] [diff] [review]
small tweaks to tests

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

::: mail/test/mozmill/shared-modules/test-quick-filter-bar-helper.js
@@ +11,4 @@
>  
> +function setupModule() {
> +  fdh = collector.getModule('folder-display-helpers');
> +  mc = fdh.mc;

why this change? can't this use the "normal" way of not exposing fdh, and using the global mc?

@@ +31,5 @@
>    'assert_results_label_count',
>    'clear_constraints',
>  ];
>  
> +const backstage = this;

I don't think const should be used for non-constants.

::: mail/test/mozmill/shared-modules/test-subscribe-window-helper.js
@@ +76,5 @@
>  function check_newsgroup_displayed(swc, name) {
>    let tree = swc.eid("searchTree").getNode();
>    let treeview = tree.view;
>    let nameCol = tree.columns.getColumnFor(swc.eid("nameColumn2").getNode());
> +  for (let i = 0; i < treeview.rowCount; ++i ) {

make it i++ and remove the extra space also

Comment 17

[Magnus Melin [:mkmelin]]

Comment on attachment 8379223 [details] [diff] [review]
small tweaks to tests

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

Clearing r? until the above question is answered

Comment 18

[:aceman]

Attachment: small tweaks to tests v2

(In reply to Magnus Melin from comment #16)
> Review of attachment 8379223 [details] [diff] [review]:
> ::: mail/test/mozmill/shared-modules/test-quick-filter-bar-helper.js
> @@ +11,4 @@
> >  
> > +function setupModule() {
> > +  fdh = collector.getModule('folder-display-helpers');
> > +  mc = fdh.mc;
> why this change? can't this use the "normal" way of not exposing fdh, and
> using the global mc?

It appears to me this is the normal way in shared-modules. In this way (without .installInto(module)) functions from e.g. folder-display-helpers.js are actually not exported to the tests importing test-quick-filter-bar-helper.js. In normal tests, yes, all functions from the shared modules and mc are imported into the test itself and used as globals. But not in shared-modules files themselves.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/46881f948e10
https://hg.mozilla.org/comm-central/rev/4805e92bc2cb

Comment 20

[:aceman]

Let's close this as is and make a new bug for the rest.