Closed Bug 1120093 Opened 11 years ago Closed 11 years ago

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

Categories

(MailNews Core :: Search, defect)

x86
Linux
defect
Not set
normal

Tracking

(thunderbird37 fixed)

RESOLVED FIXED
Thunderbird 38.0
Tracking Status
thunderbird37 --- fixed

People

(Reporter: aceman, Assigned: aceman)

References

Details

(Keywords: intermittent-failure)

Attachments

(2 files, 1 obsolete file)

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()) "}]"
Attached patch patch (obsolete) — — Splinter Review
Attachment #8547067 - Flags: review?(kent)
Attached patch patch v1.1 — — Splinter Review
Attachment #8547067 - Attachment is obsolete: true
Attachment #8547067 - Flags: review?(kent)
Attachment #8547068 - Flags: review?(kent)
Attached patch juat fix the warning — — Splinter Review
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: 11 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.

Attachment

General

Creator:
Created:
Updated:
Size: