fix Thunderbird's test_autoconfigUtils.js test

RESOLVED FIXED in Thunderbird 66.0

Status

defect
RESOLVED FIXED
4 months ago
4 months ago

People

(Reporter: aceman, Assigned: aceman)

Tracking

(Blocks 1 bug)

Trunk
Thunderbird 66.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

4 months ago

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.

(Assignee)

Comment 1

4 months ago
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)
(Assignee)

Comment 2

4 months ago

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

(Assignee)

Comment 4

4 months ago

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

Updated

4 months ago
Attachment #9036243 - Flags: review?(ben.bucksch) → review+
(Assignee)

Comment 6

4 months ago
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}]
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?

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

Updated

4 months ago
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
'<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.

(Assignee)

Comment 8

4 months ago

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.

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 10

4 months ago
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.

Updated

4 months ago
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.

Comment 12

4 months ago

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.

(Assignee)

Updated

4 months ago
Blocks: 1520283
(Assignee)

Comment 13

4 months ago

Thanks, is it better now?

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

Comment 14

4 months ago

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

Beautiful, I hope the linter likes it ;-)

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

Comment 15

4 months ago

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

(Assignee)

Comment 16

4 months ago

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

(Assignee)

Comment 17

4 months ago
Attachment #9036713 - Flags: review?(jorgk)

Comment 18

4 months ago

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+

Comment 19

4 months ago

Pushed by mozilla@jorgk.com:
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

Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
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.