1519742 - fix Thunderbird's test_autoconfigUtils.js test

Status: RESOLVED FIXED

(MailNews Core :: Testing Infrastructure, defect)

Description

[:aceman]

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
@chrome://messenger/content/accountcreation/guessConfig.js:1057:1
xpcshell/comm/mailnews/base/test/unit/test_autoconfigUtils.js:28:3
load_file@/var/NVME/TB-hg/mozilla/testing/xpcshell/head.js:636:7
_load_files@/var/NVME/TB-hg/mozilla/testing/xpcshell/head.js:648:3
_execute_test@/var/NVME/TB-hg/mozilla/testing/xpcshell/head.js:502:3
(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
TEST_END: PASS
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.

Comment 1

[:aceman]

Attachment: 1519742.patch

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?

Comment 2

[:aceman]

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.

Comment 3

[Ben Bucksch (:BenB)]

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 https://github.com/mozilla/releases-comm-central/commit/28d1c8be479449eec1e6b012b53ce3975ca9a403 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.

Comment 4

[:aceman]

(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?

Comment 5

[Ben Bucksch (:BenB)]

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.

Comment 6

[:aceman]

Attachment: 1519742.patch

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}]
logException@resource:///modules/errUtils.js:35:3
readFromXML@chrome://messenger/content/accountcreation/readFromXML.js:205:19
test_readFromXML_config1@/var/NVME/TB-hg/mozilla/obj-x86_64-pc-linux-gnu/_tests/xpcshell/comm/mail/components/test/unit/test_autoconfigXML.js:163:16
run_test@/var/NVME/TB-hg/mozilla/obj-x86_64-pc-linux-gnu/_tests/xpcshell/comm/mail/components/test/unit/test_autoconfigXML.js:263:3

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

Comment 7

[Ben Bucksch (:BenB)]

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
'<socketType>GSSAPI2</socketType>'
In readFromXML.js:
sanitize.translate(oXauth,
{ // 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.

Comment 8

[:aceman]

Thanks.
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.

Comment 9

[Ben Bucksch (:BenB)]

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

Comment 10

[Jorg K (CEST = GMT+2)]

Comment on attachment 9036473 [details] [diff] [review]
1519742.patch

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.

Comment 11

[Ben Bucksch (:BenB)]

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.

Comment 12

[Jorg K (CEST = GMT+2)]

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.

Comment 13

[:aceman]

Attachment: 1519742.patch v1.1

Thanks, is it better now?

Comment 14

[Jorg K (CEST = GMT+2)]

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

Beautiful, I hope the linter likes it ;-)

Comment 15

[Jorg K (CEST = GMT+2)]

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

Comment 16

[:aceman]

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

Comment 17

[:aceman]

Attachment: 1519742-2.patch eslint

Comment 18

[Jorg K (CEST = GMT+2)]

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

Thanks, I checked the linting and ran the tests.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/4c0e476ae818
fix tests test_autoconfig*.js to run under Thunderbird. r=BenB
https://hg.mozilla.org/comm-central/rev/b6ff801164a8
fix eslint problems in test_autoconfig*.js test files. r=jorgk