1490448 - make Thunderbird's mozmill tests run in JS strict mode

Status: RESOLVED FIXED

(Thunderbird :: Testing Infrastructure, enhancement)

Description

[:aceman]

It was found in bug 1474219 that Thunderbird's mozmill tests do not run in Javascript strict mode so there is less code checking by the JS engine.

Let's see if it is possible to enable strict mode to catch more problems in future, like changed interfaces and calls to no longer existing objects.

Comment 1

[:aceman]

Attachment: 1490448-strict.patch

This adds the "use strict" directive into all test files so that they run in JS strict mode. Probably not much to see here.

Comment 2

[Jorg K (CEST = GMT+2)]

Comment on attachment 9009719 [details] [diff] [review]
1490448-strict.patch

Indeed, not terribly good reading.

Comment 3

[:aceman]

Attachment: 1490448-fixes.patch

This fixes problems found by the strict mode, without which the tests would be aborted due to JS TypeErrors and similar.

Mostly just undeclared variables.
The "var global = this" playes are discussed with arai.
It is interesting that non-strict mode didn't warn about the assignment to a read-only crashreporter.enabled. This is probably the only real bug that the strict mode has found.

Both patches must be landed together, the "fixes" on top of "strict".

Successful try run here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=56d76e12648d1cd1e4e2d04d3152b6d4467c5005

Comment 4

[:aceman]

Next step would be to run eslint on the mozmill tests :)

Comment 5

[Jorg K (CEST = GMT+2)]

Comment on attachment 9009722 [details] [diff] [review]
1490448-fixes.patch

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

Not so exiting and quite a bit of boy-scout clean-up ;-)

The change you discussed with Tooru (:arai) isn't immediately obvious, what were the errors?

::: mail/test/mozmill/folder-display/test-right-click-middle-click-folders.js
@@ -237,2 @@
>  function _generate_background_foreground_tests(aTests) {
> -  let self = this;

That's the normal way to do these things, no? Were there strict errors that mandated the change? I don't think it's much more legible now. What did the helperFunc.apply() do?

Comment 6

[:aceman]

(In reply to Jorg K (GMT+2) from comment #5)
> Comment on attachment 9009722 [details] [diff] [review]
> 1490448-fixes.patch
> 
> Review of attachment 9009722 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Not so exiting and quite a bit of boy-scout clean-up ;-)

Not that much, only at places which already had an error. E.g. sometimes instead of declaring a variable I chose to remove it (or rename it), which could cause some churn.

> ::: mail/test/mozmill/folder-display/test-right-click-middle-click-folders.js
> @@ -237,2 @@
> >  function _generate_background_foreground_tests(aTests) {
> > -  let self = this;
> 
> That's the normal way to do these things, no? Were there strict errors that
> mandated the change? I don't think it's much more legible now. What did the
> helperFunc.apply() do?

Yes, error "this is undefined; can't access its "_middle_click_folder_with_nothing_selected_helper" property  at: nonesuch line 240", seen e.g. at https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&selectedJob=199581192&revision=d8f9c93e5dedc6678ede26c8955df4dc03f122c2

From arai:
arai: `this` inside a function is the value that is used when calling the function
arai: `this` at the toplevel is the global object

We weren't passing anything into the function so 'this' was undefined. Another way to fix this would be to call the generating function with .apply(this); (the top level 'this').
.apply() calls the function by specifying this and arguments (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/apply). I removed that as it seems we do not need 'this' inside the generated functions, so it could be simplified.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2977f7b7b8d6
force strict JS mode in MozMill tests. r=jorgk
https://hg.mozilla.org/comm-central/rev/4769902c580e
fix problems found by JS strict mode in MozMill tests. r=jorgk CLOSED TREE