Open Bug 524781 Opened 10 years ago Updated 8 years ago

xpcshell-tests: turn on strict & werror by default

Categories

(Testing :: General, defect, minor)

defect
Not set
minor

Tracking

(Not tracked)

ASSIGNED
mozilla1.9.3a1

People

(Reporter: lusian, Assigned: lusian)

References

Details

(Keywords: helpwanted)

Attachments

(5 files, 1 obsolete file)

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
Depends on: 520743, 521143, 521151, 521154, 521191
URL: 521189
URL: 521189
Depends on: 521189
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
Attachment #409242 - Flags: review?(doug.turner) → review+
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: checkin-needed
Attachment #409240 - Flags: review?(benjamin) → review+
Attachment #409600 - Flags: review? → review?(benjamin)
Attachment #409600 - Flags: review?(benjamin) → review+
Attached patch json2.js (obsolete) — Splinter Review
Attachment #410739 - Flags: review?
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
Attachment #409240 - Attachment description: patch for tests that fails on js strict mode → patch for tests that fails on js strict mode [Checkin: Comment 6]
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
Attachment #409242 - Attachment description: xpcom/tests/unit/test_localfile.js → xpcom/tests/unit/test_localfile.js [Checkin: Comment 7]
Comment on attachment 409600 [details] [diff] [review]
Fix errors in JS strict mode
[Checkin: Comment 8]


http://hg.mozilla.org/mozilla-central/rev/89fbfcf812e2
Attachment #409600 - Attachment description: Fix errors in JS strict mode → Fix errors in JS strict mode [Checkin: Comment 8]
Assignee: nobody → lusian
Severity: trivial → minor
Status: NEW → ASSIGNED
Flags: in-testsuite+
OS: Windows XP → All
Hardware: x86 → All
Whiteboard: [needs approval for c-n: m-1.9.2 and probably m-1.9.1]
Target Milestone: --- → mozilla1.9.3a1
Version: unspecified → Trunk
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.
Attachment #409240 - Flags: review?(beltzner)
Comment on attachment 409600 [details] [diff] [review]
Fix errors in JS strict mode
[Checkin: Comment 8]


"approval1.9.2=?":
"No" risk, 'var' addition.
Attachment #409600 - Flags: review?(beltzner)
Jae-Seong, please, in future, file separate bugs for non-test files.
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?
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.
Attachment #410739 - Flags: review? → review?(sayrer)
Comment on attachment 410739 [details] [diff] [review]
json2.js

have the default case throw
Attachment #410739 - Flags: review?(sayrer) → review-
Attachment #413054 - Flags: review?(jwalden+bmo)
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 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
Attachment #409242 - Attachment description: xpcom/tests/unit/test_localfile.js [Checkin: Comment 7] → xpcom/tests/unit/test_localfile.js [Checkin: Comment 7 & 16]
(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.
Attachment #410739 - Attachment is obsolete: true
Attachment #413554 - Flags: review?(sayrer)
Depends on: 530028
Depends on: 530032
Depends on: 530034
Depends on: 530035
(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 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
Attachment #409242 - Attachment description: xpcom/tests/unit/test_localfile.js [Checkin: Comment 7 & 16] → xpcom/tests/unit/test_localfile.js [Checkin: Comment 7 & 16 & 20]
Depends on: 530292
Depends on: 530376
Blocks: 532022
No longer blocks: 532022
Depends on: 532022
Depends on: 532035
Depends on: 532037
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?
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));
Attachment #413054 - Flags: review?(jwalden+bmo) → review+
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.
Attachment #413054 - Attachment description: xpcshell-tests: turn on strict & werror by default → xpcshell-tests: turn on strict & werror by default [don't check-in yet]
Comment on attachment 409600 [details] [diff] [review]
Fix errors in JS strict mode
[Checkin: Comment 8]


(Giving up on branches.)
Attachment #409600 - Flags: review?(beltzner)
Comment on attachment 409240 [details] [diff] [review]
patch for tests that fails on js strict mode
[Checkin: Comment 6]


(Giving up on branches.)
Attachment #409240 - Flags: review?(beltzner)
Whiteboard: [needs approval for c-n: m-1.9.2 and probably m-1.9.1]
Depends on: 551231
Depends on: 551521
Attachment #413554 - Flags: review?(sayrer) → review?(jwalden+bmo)
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.
Attachment #413554 - Flags: review?(jwalden+bmo)
You need to log in before you can comment on or make changes to this bug.