524781 - xpcshell-tests: turn on strict & werror by default

Status: ASSIGNED

(Testing :: General, defect, minor)

Description

[Jae-Seong Lee-Russo]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20091027 Minefield/3.7a1pre (.NET CLR 3.5.30729)
Build Identifier: 

Run the xpcshell test with the Javascript options 'strict' & 'werror' on by default.

Right now the test runs with the 'strict' on, and some of the existing tests fail when the 'werror' option is on.

Reproducible: Always

Steps to Reproduce:
1. Run 'make xpcshell-tests' with the '-S' option

Actual Results:  
Failed

Expected Results:  
Passed

Comment 1

[Jae-Seong Lee-Russo]

Attachment: patch for tests that fails on js strict mode [Checkin: Comment 6]

Comment 2

[Jae-Seong Lee-Russo]

Attachment: xpcom/tests/unit/test_localfile.js [Checkin: Comment 7 & 16 & 20]

Comment 3

[Jae-Seong Lee-Russo]

Two tests fail on JS strict mode (I submitted patches for everything else for now):

TEST-UNEXPECTED-FAIL | c:/mozilla-build/mozilla-central-devel/obj-i686-pc-mingw32/_tests/xpcshell/test_extensionmanager/unit/test_bug470377_3.js | true == false
JS frame :: c:/mozilla-build/mozilla-central-devel/obj-i686-pc-mingw32/_tests/xpcshell/test_extensionmanager/unit/test_bug470377_3.js :: run_test :: line 94

TEST-UNEXPECTED-FAIL | c:/mozilla-build/mozilla-central-devel/obj-i686-pc-mingw32/_tests/xpcshell/test_extensionmanager/unit/test_bug470377_4.js | false == true
JS frame :: c:/mozilla-build/mozilla-central-devel/obj-i686-pc-mingw32/_tests/xpcshell/test_extensionmanager/unit/test_bug470377_4.js :: run_test :: line 96

Comment 4

[Jae-Seong Lee-Russo]

Attachment: Fix errors in JS strict mode [Checkin: Comment 8]

Comment 5

[Jae-Seong Lee-Russo]

Attachment: json2.js

Comment 6

[Serge Gautherie (:sgautherie)]

Comment on attachment 409240 [details] [diff] [review]
patch for tests that fails on js strict mode
[Checkin: Comment 6]


http://hg.mozilla.org/mozilla-central/rev/4346a668d379

Comment 7

[Serge Gautherie (:sgautherie)]

Comment on attachment 409242 [details] [diff] [review]
xpcom/tests/unit/test_localfile.js
[Checkin: Comment 7 & 16 & 20]


http://hg.mozilla.org/mozilla-central/rev/a27c6a3bcd6d

Comment 8

[Serge Gautherie (:sgautherie)]

Comment on attachment 409600 [details] [diff] [review]
Fix errors in JS strict mode
[Checkin: Comment 8]


http://hg.mozilla.org/mozilla-central/rev/89fbfcf812e2

Comment 9

[Serge Gautherie (:sgautherie)]

Comment on attachment 409240 [details] [diff] [review]
patch for tests that fails on js strict mode
[Checkin: Comment 6]


"approval1.9.2=?":
for nsExtensionManager.js.in.
"No" risk, 'var' addition.

Comment 10

[Serge Gautherie (:sgautherie)]

Comment on attachment 409600 [details] [diff] [review]
Fix errors in JS strict mode
[Checkin: Comment 8]


"approval1.9.2=?":
"No" risk, 'var' addition.

Comment 11

[Serge Gautherie (:sgautherie)]

Jae-Seong, please, in future, file separate bugs for non-test files.

Comment 12

[Jeff Walden [:Waldo]]

Um, this doesn't need to be fixed for 1.9.2.  Or 1.9.1.  Or anywhere but mozilla-central.  There's no urgent need for thinko-catching behavior, it's just nice (perhaps very nice, sometimes) to have, and we can live a little longer if we have to without it on branches.

Jae-Seong, did you mean to request review from a particular person on the json2.js bit?

Comment 13

[Jae-Seong Lee-Russo]

Attachment: xpcshell-tests: turn on strict & werror by default [don't check-in yet]

Jeff, I don't know whom to ask to review json2.js.  I was just hoping somebody picks up and reviews the file.

I think I fixed all the existing tests.  json2.js & bug 520743 are pending.

Comment 14

[Robert Sayre]

Comment on attachment 410739 [details] [diff] [review]
json2.js

have the default case throw

Comment 15

[Serge Gautherie (:sgautherie)]

Comment on attachment 413054 [details] [diff] [review]
xpcshell-tests: turn on strict & werror by default [don't check-in yet]


Jae-Seong, did you check that the (3) comm-central apps too are ready for this?

Comment 16

[Serge Gautherie (:sgautherie)]

Comment on attachment 409242 [details] [diff] [review]
xpcom/tests/unit/test_localfile.js
[Checkin: Comment 7 & 16 & 20]


http://hg.mozilla.org/releases/mozilla-1.9.2/rev/9f90859d2a9c

Comment 17

[Jae-Seong Lee-Russo]

(In reply to comment #15)
> (From update of attachment 413054 [details] [diff] [review])
> 
> Jae-Seong, did you check that the (3) comm-central apps too are ready for this?

Ooops, I didn't.  Thank you for saying that.  I will look at comm-central, too.

Comment 18

[Jae-Seong Lee-Russo]

Attachment: default thorws Error

Comment 19

[Serge Gautherie (:sgautherie)]

(In reply to comment #17)
> I will look at comm-central, too.

Thanks for filing blocking bugs :-)
Note that tests are considered part of the application(s) components: the Testing component is mainly for the harness itself...

Comment 20

[Serge Gautherie (:sgautherie)]

Comment on attachment 409242 [details] [diff] [review]
xpcom/tests/unit/test_localfile.js
[Checkin: Comment 7 & 16 & 20]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/cd24e78c2f2c

Comment 21

[Jae-Seong Lee-Russo]

Bug 514559 forbids the use of octal literals on the strict mode, and I can't think of a strict-legal expression as simple as the old one.

e.g., dir.createUnique(Ci.nsIFile.DIRECTORY_TYPE, 0744);

Can you suggest any?

Comment 22

[Jeff Walden [:Waldo]]

I was thinking maybe (7 << 6) | (4 << 3) | 4 at one point, but maybe something like this is better:

function octal()
{
  var r = 0;
  for (var i = 0; i < arguments.length; i++)
    r = (r << 3) + arguments[i];
  return r;
}

  dir.createUnique(Ci.nsIFile.DIRECTORY_TYPE, octal(7, 4, 4));

Comment 23

[Jae-Seong Lee-Russo]

Comment on attachment 413054 [details] [diff] [review]
xpcshell-tests: turn on strict & werror by default [don't check-in yet]

There are still many things to do; calendar does not build on m-c.  Octal literals are deprecated.

Comment 24

[Serge Gautherie (:sgautherie)]

Comment on attachment 409600 [details] [diff] [review]
Fix errors in JS strict mode
[Checkin: Comment 8]


(Giving up on branches.)

Comment 25

[Serge Gautherie (:sgautherie)]

Comment on attachment 409240 [details] [diff] [review]
patch for tests that fails on js strict mode
[Checkin: Comment 6]


(Giving up on branches.)

Comment 26

[Jeff Walden [:Waldo]]

Comment on attachment 413554 [details] [diff] [review]
default thorws Error

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

This file got removed when I moved JSON testing into js/src/tests, and the file itself didn't get transferred.  So I don't think this patch is needed any more.