mailnews/imap/test/unit/test_imapSearch.js, line 260: SyntaxError: test for equality (==) mistyped

RESOLVED FIXED in Thunderbird 38.0


5 years ago
4 years ago


(Reporter: aceman, Assigned: aceman)



Bug Flags:
in-testsuite +

Thunderbird Tracking Flags

(thunderbird37 fixed)



(2 attachments, 1 obsolete attachment)

Seemingly new failure in test_imapSearch.js.
A JS warning suddenly became a SyntaxError? Maybe a change in the JS engine.

JavaScript strict warning: tbird-bin/_tests/xpcshell/mailnews/imap/test/unit/test_imapSearch.js, line 260: SyntaxError: test for equality (==) mistyped as
assignment (=)?
PROCESS | 13696 | set mailnews.customDBHeaders to x-spam-status oneliner twoliner threeliner nospace withspace
(xpcshell/head.js) | test MAIN run_test pending (1)
(xpcshell/head.js) | test pending (2)
(xpcshell/head.js) | test _async_driver pending (3)
(xpcshell/head.js) | test MAIN run_test finished (3)
running event loop
Error console says [stackFrame SyntaxError: test for equality (==) mistyped as assignment (=)?]
exiting test
"CONSOLE_MESSAGE: (warn) [JavaScript Warning: "SyntaxError: test for equality (==) mistyped as assignment (=)?" {file: "tbird-bin/_tests/xpcshell/mailnews/imap/test/unit/tes
t_imapSearch.js" line: 260 column: 35 source: "  while (test = searchTests.shift())
Posted patch patch (obsolete) — Splinter Review
Attachment #8547067 - Flags: review?(kent)
Posted patch patch v1.1Splinter Review
Attachment #8547067 - Attachment is obsolete: true
Attachment #8547067 - Flags: review?(kent)
Attachment #8547068 - Flags: review?(kent)
Duplicate of this bug: 1120112
Attachment #8547103 - Flags: review?(acelists)
Can't we just do the simpler fix, which is normal for these warnings? See try run:
I would not understand why there are double brackets and would remove them if I did cleanup in that file ;) I just handle basic JS methods :)
So the question here is whether if(( test = something())) is recognized as an idiom to avoid the warning. It is seen in calendar code:




So I guess I consider this to be the canonical way to allow the perfectly reasonable if (test = something())

Let's see what Magnus thinks.
Attachment #8547103 - Flags: review?(acelists) → review?(mkmelin+mozilla)
Comment on attachment 8547103 [details] [diff] [review]
juat fix the warning

Review of attachment 8547103 [details] [diff] [review]:

I'd argue that aceman's patch is preferable, though I think that this would be even better (so you don't need the shift either):
for (let test of searchTests) {

With the while loop the "test" is declared outside the smallest scope needed, and IMHO less readable (tough of course technically ok).
Attachment #8547103 - Flags: review?(mkmelin+mozilla)
Yes, I would like for..of too.
Yes, please let's clean up the code especially as aceman has already done the work :-)
Attachment #8547068 - Flags: review?(kent) → review+
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 38.0
Comment on attachment 8547068 [details] [diff] [review]
patch v1.1

[Triage Comment]
Please try and remember to see if patches need uplift, especially around merge times - marking this is a+ for aurora as its needed there.
Attachment #8547068 - Flags: approval-comm-aurora+
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.