fix Thunderbird's test_autoconfigUtils.js test

RESOLVED FIXED in Thunderbird 66.0


6 months ago
6 months ago


(Reporter: aceman, Assigned: aceman)


(Blocks 1 bug)

Thunderbird 66.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)



(2 attachments, 2 obsolete attachments)

When running the xpcshell test mailnews/base/test/unit/test_autoconfigUtils.js for account creation under Thunderbird, it seems to be silently dead for possibly a long time already.

Here is the output:
TEST_START: comm/mailnews/base/test/unit/test_autoconfigUtils.js
loading accountcreation JS files failed: ReferenceError: UserCancelledException is not defined
(xpcshell/head.js) | test MAIN run_test pending (1)
test_autoconfigUtils.js not running, because this is SeaMonkey.
(xpcshell/head.js) | test MAIN run_test finished (1)
exiting test
INFO | Result summary:
INFO | Passed: 1

It fails due to an undefined object based on which it incorrectly assumes it is running under Seamonkey and gracefully exits with success, thus the error remains unnoticed (xpcshell does not show test output if test does not fail).
I only found this when carefully grepping all tests' output (--verbose argument to xpcshell run) for bug 1519712.

I have a patch for this.

Posted patch 1519742.patch (obsolete) — Splinter Review

So this works for me.
It needed to import fetchhttp.js to get the UserCancelledException.

I added a check so that if the load of the modules fails we do not blindly trust we as in Seamonkey, but if we are actually running under Thunderbird abort the test loudly.

After fixing the imports it actually came to light the test does not pass due to logical errors. The list of protocol+security combinations that are checked do not match those produced in mail/components/accountcreation/content/guessConfig.js (e.g. getIncomingTryOrder()). You can see the getHostEntry(protocol, SSL, ...) lines are commented out there.

So I commented those combinations in the test too and now it passes.

But this is a question for Ben whether the test needs to adapt (as I did) or why those SSL combinations are commented out in guessConfig.js . Maybe the test needs to add some more checks where SSL is requested explicitly?

Attachment #9036243 - Flags: review?(ben.bucksch)

Or we could simplify/remove the Seamonkey checking logic by moving the test_autoconfig*.js files under /mail as whole accountcreation was also moved under /mail as Seamonkey does not intend to use it.

aceman, thanks for finding and fixing this.

Regarding SSL options, could you make that a separate patch? It seems like it's a separate issue. I don't know why the normal SSL options are disabled and only the STARTTLS options are tried. I traced it back to from bug 721369, which is my own patch from 2012, and which fixed a nasty SSL cert override bug. But it shows no hint of why the normal SSL options were disabled, and I don't remember, either. All I could find was bug 721369 comment 51.

(In reply to Ben Bucksch (:BenB) from comment #3)

Regarding SSL options, could you make that a separate patch? It seems like it's a separate issue.

What do you mean here? Adding ssl=SSL branch in the test and checking the combinations returned by guessConfig?

And what about moving the tests to /mail and dropping the Seamonkey checks?

What do you mean here?

I meant to separate it in to 2 patch files, 2 hg commits. But actually: nevermind. It wasn't important. Please ignore that.

And what about moving the tests to /mail and dropping the Seamonkey checks?

If it's easy to do, I think it would be a good idea, yes.
Up to you to decide.

Attachment #9036243 - Flags: review?(ben.bucksch) → review+
Posted patch 1519742.patch (obsolete) — Splinter Review

This works for me now.

Is it correct that the autoconfigXML.js test throws this the following?
Supplied value not in allowed list
-- Exception object --
_message (string) 'Supplied value not in allowed list'
stack (string) 672 chars
constructor (function) 8 lines
message (string) 'Supplied value not in allowed list'
toString (function) 3 lines
"CONSOLE_MESSAGE: (error) [JavaScript Error: "Supplied value not in allowed list" {file: "resource:///modules/errUtils.js" line: 35}]

The test passes even with this exception. Is the test feeding the parser invalid data? Shouldn't the exception be caught in some way?

Attachment #9036243 - Attachment is obsolete: true
Attachment #9036473 - Flags: review?(ben.bucksch)
Attachment #9036473 - Flags: review?(ben.bucksch) → review+

aceman, I don't know whether that's normal, I didn't write the test.
Sticking around blindly a bit, I think this is the reason:
In the test:
// outgoing server with invalid auth method
In readFromXML.js:
{ // open relay
"none": Ci.nsMsgAuthMethod.none,
// inside ISP or corp network
"client-IP-address": Ci.nsMsgAuthMethod.none,
// hope for the best
"smtp-after-pop": Ci.nsMsgAuthMethod.none,
"password-cleartext": Ci.nsMsgAuthMethod.passwordCleartext,
// @deprecated TODO remove
"plain": Ci.nsMsgAuthMethod.passwordCleartext,
"password-encrypted": Ci.nsMsgAuthMethod.passwordEncrypted,
// @deprecated TODO remove
"secure": Ci.nsMsgAuthMethod.passwordEncrypted,
"GSSAPI": Ci.nsMsgAuthMethod.GSSAPI,
"NTLM": Ci.nsMsgAuthMethod.NTLM,
"OAuth2": Ci.nsMsgAuthMethod.OAuth2,

In other words, the test deliberately tries to use an invalid authentication method. So, the code must throw. So, the error message you see is deliberately provoked and means the test is working as expected.

An intentional exception should somehow be caught by the test and checked for and not appear as a "JavaScript Error:". But I have seen also other exceptions show as "JavaScript Error: ... uncaught exception: ..." even though they were clearly inside a try {} catch {} block. I'll investigate these in a new bug.

Keywords: checkin-needed

It's the logException(e) in the app code that shows this. This is intentional, for debugging when users run into this.

Comment on attachment 9036473 [details] [diff] [review]

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

::: mail/components/test/unit/test_autoconfigUtils.js
@@ +83,5 @@
>    let port = UNKNOWN;
>    let tryOrder = getIncomingTryOrder(host, protocol, ssl, port);
>    assert_equal_try_orders(tryOrder,
>                            [[POP, TLS, 110],
> +//                           [POP, SSL, 995],

Sorry, why are we commenting out those lines? And also destroying the alignment here? Are those tests moved below? Can we remove those lines then. Or comment why the are still there. This looks like WIP.
Keywords: checkin-needed

They are commented out, because they are also commented out in the app code. This makes the test match.

As to why, that is unknown. They should be enabled, but were disabled for an unknown reason. I baffled as well. See comment 3. This is a separate bug.

OK, but somehow they weren't commented out before. Maybe the entire test didn't run. Anyway, let's not make it look like WIP, fix the alignment and put a comment why they are commented out with a bug reference. We either remove dead code or tell the reader why it's still there.

Blocks: 1520283

Thanks, is it better now?

Attachment #9036473 - Attachment is obsolete: true
Attachment #9036697 - Flags: review+
Attachment #9036697 - Flags: feedback?(jorgk)

Comment on attachment 9036697 [details] [diff] [review]
1519742.patch v1.1

Beautiful, I hope the linter likes it ;-)

Attachment #9036697 - Flags: feedback?(jorgk) → feedback+

Wow, 318 linter errors. You can't just move unlinted mailnews code to mail :-(

OK, I can make the linter fixes in a new patch on top.

Attachment #9036713 - Flags: review?(jorgk)

Comment on attachment 9036713 [details] [diff] [review]
1519742-2.patch eslint

Thanks, I checked the linting and ran the tests.

Attachment #9036713 - Flags: review?(jorgk) → review+

Pushed by
fix tests test_autoconfig*.js to run under Thunderbird. r=BenB
fix eslint problems in test_autoconfig*.js test files. r=jorgk

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