fix some Javascript errors in Thunderbird's xpcshell tests

RESOLVED FIXED in Thunderbird 66.0

Status

defect
--
trivial
RESOLVED FIXED
4 months ago
4 months ago

People

(Reporter: aceman, Assigned: aceman)

Tracking

Trunk
Thunderbird 66.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Assignee

Description

4 months ago

Running Thunderbird's xpcshell test suite (/mail and /mailnews) produces a few Javascript error messages, mostly about undeclared variables or access past string/array end.

E.g.:
JavaScript strict warning: resource:///modules/jsmime/jsmime.js, line 951: ReferenceError: reference to undefined property 0
JavaScript strict warning: comm/mailnews/mime/test/unit/test_mimeStreaming.js, line 73: ReferenceError: assignment to undeclared variable gStreamListener
JavaScript strict warning: resource:///modules/jsmime/jsmime.js, line 1099: ReferenceError: reference to undefined property 0
JavaScript strict warning: comm/mailnews/mime/jsmime/test/test_header.js, line 701: ReferenceError: reference to undefined property 2
JavaScript strict warning: comm/mailnews/extensions/bayesian-spam-filter/test/unit/test_traits.js, line 161: ReferenceError: reference to undefined property "currentIndex"
JavaScript Warning: "ReferenceError: assignment to undeclared variable messageContent"
JavaScript Warning: "ReferenceError: assignment to undeclared variable customValue"
JavaScript Warning: "ReferenceError: assignment to undeclared variable acctMgr"
JavaScript Error: "kDebug is not defined" {file: "chrome://messenger/content/accountcreation/sanitizeDatatypes.js" line: 200}]

Assignee

Comment 1

4 months ago
Posted patch 1519712.patch (obsolete) — Splinter Review
Attachment #9036203 - Flags: review?(jorgk)
Attachment #9036203 - Flags: review?(Pidgeot18)
Assignee

Comment 2

4 months ago
Comment on attachment 9036203 [details] [diff] [review]
1519712.patch

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

I'd like jcranmer to see the changes in jsmime.

::: mailnews/mime/jsmime/jsmime.js
@@ +947,5 @@
>        }
>  
>        // We need space for the next token if we aren't some kind of comment or
>        // . delimiter.
> +      needsSpace = (token.toString().length > 0) && (token.toString()[0] != '.');

Caused by some empty string in test_header.js. I don't know if in that case we 'need space' or not.

@@ +1095,5 @@
>        if (number == '0')
>          entry.hasCharset = lastStar;
>  
>        // Is the continuation number illegal?
> +      else if (number.length == 0 || (number[0] == '0' && number != '0') ||

This one is triggered by this string in test_header.js:
     ["attachment; filename=basic; filename**=UTF-8''0;",
        ["attachment", {filename: "basic"}]],

I don't know if we should get entry.valid = false in this case, but the test works.

Comment 4

4 months ago
Comment on attachment 9036203 [details] [diff] [review]
1519712.patch

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

You're waiting for Joshua? ;-)

::: mail/components/accountcreation/content/sanitizeDatatypes.js
@@ +187,5 @@
>          return mapping[inputValue];
>      }
>      // value is bad
> +    if (typeof(defaultValue) == "undefined")
> +      throw new MalformedException("allowed_value.error", unchecked);

I don't see a change here.

@@ +196,5 @@
>  function MalformedException(msgID, uncheckedBadValue) {
>    var stringBundle = getStringBundle(
>        "chrome://messenger/locale/accountCreationUtil.properties");
>    var msg = stringBundle.GetStringFromName(msgID);
> +  if ((typeof(kDebug) != "undefined") && kDebug)

Does typeof need parenthesis? I don't think so. Would !!kDebug work?

::: mailnews/extensions/bayesian-spam-filter/test/unit/test_traits.js
@@ +158,5 @@
>        delete gTest.percents[aTokenString[i]];
>      }
>      Assert.equal(Object.keys(gTest.percents).length, 0);
> +    if (gTest.command == kClass)
> +      gTest.currentIndex++;

What's the deal with gTest.command == kClass?
Attachment #9036203 - Flags: review?(jorgk) → review+
Assignee

Comment 5

4 months ago

(In reply to Jorg K (GMT+1) from comment #4)

::: mail/components/accountcreation/content/sanitizeDatatypes.js
@@ +187,5 @@

  • if (typeof(defaultValue) == "undefined")
  •  throw new MalformedException("allowed_value.error", unchecked);
    

I don't see a change here.

Sorry, no change there, remnant hunk.

@@ +196,5 @@

  • if ((typeof(kDebug) != "undefined") && kDebug)

Does typeof need parenthesis? I don't think so. Would !!kDebug work?

It does not need () but we use it both ways (with and without () randomly).
In my testing !!kDebug would still produce the warning about 'not defined' variable.

::: mailnews/extensions/bayesian-spam-filter/test/unit/test_traits.js
@@ +158,5 @@

  • if (gTest.command == kClass)
  •  gTest.currentIndex++;
    

What's the deal with gTest.command == kClass?

If you see the whole test file, it seems it only assigns and maintains gTest.currentIndex if gTest.command == kClass. Only this function was an exception thus the .currentIndex was not found.

Attachment #9036203 - Flags: review?(Pidgeot18) → review+
Assignee

Comment 6

4 months ago
Posted patch 1519712.patch v1.1 (obsolete) — Splinter Review

Thanks.

Attachment #9036203 - Attachment is obsolete: true
Attachment #9036513 - Flags: review+
Assignee

Updated

4 months ago
Keywords: checkin-needed

Comment 7

4 months ago
Comment on attachment 9036513 [details] [diff] [review]
1519712.patch v1.1

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

::: mailnews/local/test/unit/test_pop3ServerBrokenCRAMFail.js
@@ +66,5 @@
>    __proto__: POP3_RFC5034_handler.prototype, // inherit
>  
>    killConn() {
>      this._multiline = false;
> +    this._state = 1; // kStateAuthNeeded

Can you please explain why you're using 1 here? Would 0 also work, or null? Apparently this mus have been undefined, so the value wouldn't matter. I'm confused as to what the value should be:
https://searchfox.org/comm-central/search?q=kStateAuthNeeded&case=false&regexp=false&path=

Updated

4 months ago
Keywords: checkin-needed
Assignee

Comment 8

4 months ago

Yes, it may be possible the value does not matter.
I used 1 because this is a pop3 test and in pop3d.js the kStateAuthNeeded has a value of 1 :)
I the constant was added to both files in bug 525238 by BenB.

Flags: needinfo?(ben.bucksch)

No, undefined will not work:
USER: function (args) {
if (this._state != kStateAuthNeeded)
return "-ERR invalid state";

No, you can't just set the state to another value.

This seems very clearly defined what the state can be and should be. This is a state machine, and the states are clearly defined using constants. Looks all right to me.

I don't know why aceman changed this from the constant to 1, but I presume that the constant was not defined in this file. I would suggest to simply make sure that kStateAuthNeeded is defined in this JS file.

Flags: needinfo?(ben.bucksch)
Assignee

Comment 10

4 months ago

The constant is only defined in pop3d.js. It seemed awkward to me to import that file in the test.
Either I find a better one where to put the constant, or I open-coded it for now. If you want to make it a named constant 'kStateAuthNeeded' in this test file, I can do that (that's why I kept the name at least as a comment).

If you want to make it a named constant 'kStateAuthNeeded' in this test file, I can do that

Yes, just to prevent questions like comment 7.

Assignee

Comment 12

4 months ago
Attachment #9036661 - Flags: review+
Assignee

Updated

4 months ago
Keywords: checkin-needed
Assignee

Updated

4 months ago
Attachment #9036513 - Attachment is obsolete: true

Comment 13

4 months ago

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/ee93a8e6c632
fix some Javascript errors in xpcshell tests. r=jorgk,jcranmer

Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
Keywords: checkin-needed
Resolution: --- → FIXED

Updated

4 months ago
Target Milestone: --- → Thunderbird 66.0
You need to log in before you can comment on or make changes to this bug.