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

RESOLVED FIXED in Thunderbird 38.0

Status

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: aceman, Assigned: aceman)

Tracking

({intermittent-failure})

Bug Flags:
in-testsuite +

Thunderbird Tracking Flags

(thunderbird37 fixed)

Details

Attachments

(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 (=)?]
../../../resources/logHelper.js:_errorConsoleTunnel.observe:95
mozilla/testing/xpcshell/head.js:_do_main:208
mozilla/testing/xpcshell/head.js:_execute_test:500
-e:null:1
null:null:0
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:

https://treeherder.mozilla.org/ui/#/jobs?repo=try-comm-central&revision=62af7dd8e24d
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:

http://mxr.mozilla.org/comm-central/source/calendar/providers/gdata/content/gdata-calendar-creation.js#175

loop:

http://mxr.mozilla.org/comm-central/source/mozilla/browser/components/loop/content/shared/libs/lodash-2.4.1.js#1173

devtools:

http://mxr.mozilla.org/comm-central/source/mozilla/browser/devtools/debugger/debugger-controller.js#1949

webapps:

http://mxr.mozilla.org/comm-central/source/mozilla/dom/apps/Webapps.jsm#4060

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+
Status: ASSIGNED → RESOLVED
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.