Closed Bug 876503 Opened 8 years ago Closed 4 years ago

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

Categories

(Thunderbird :: Testing Infrastructure, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 31.0

People

(Reporter: aceman, Assigned: aceman)

References

Details

Attachments

(3 files, 3 obsolete files)

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.)
Attached patch patch for shared-modules (obsolete) — Splinter Review
So let's start with the shared-modules files to see how it works.
Attachment #754560 - Flags: review?(mconley)
Status: NEW → ASSIGNED
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
(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.
Attached patch patch for shared-modules v2 (obsolete) — Splinter Review
Attachment #754560 - Attachment is obsolete: true
Attachment #754560 - Flags: review?(mconley)
Attachment #763282 - Flags: review?(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.
Attachment #763282 - Flags: review?(mconley) → review+
Thanks, done.
Attachment #763282 - Attachment is obsolete: true
Attachment #776584 - Flags: review+
Aryx, please create a try run with this patch. All systems, mozmill test only.
Flags: needinfo?(archaeopteryx)
Whiteboard: [please leave open after checkin]
Thanks, I see no new failures in the log so it can land.
Keywords: checkin-needed
Attachment #776584 - Attachment description: patch for shared-modules v3 → patch for shared-modules v3 [checkin: comment 10]
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).
Attachment #8379221 - Flags: review?(mkmelin+mozilla)
Attachment #8379221 - Flags: review?(mconley)
Attached patch small tweaks to tests (obsolete) — Splinter Review
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.
Attachment #8379223 - Flags: review?(mkmelin+mozilla)
Attachment #8379223 - Flags: review?(mconley)
Aryx, could you push each of these 2 patches to TB-try separately? mozmill tests are enough, thanks.
Flags: needinfo?(archaeopteryx)
Thanks, both runs are green.
Attachment #8379221 - Flags: review?(mkmelin+mozilla)
Attachment #8379221 - Flags: review?(mconley)
Attachment #8379221 - Flags: review+
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 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
Attachment #8379223 - Flags: review?(mkmelin+mozilla)
(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.
Attachment #8379223 - Attachment is obsolete: true
Attachment #8379223 - Flags: review?(mconley)
Attachment #8391879 - Flags: review?(mkmelin+mozilla)
Attachment #8391879 - Flags: review?(mkmelin+mozilla) → review+
Keywords: checkin-needed
Let's close this as is and make a new bug for the rest.
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Whiteboard: [please leave open after checkin]
Target Milestone: --- → Thunderbird 31.0
Blocks: 1308784
You need to log in before you can comment on or make changes to this bug.