1420695 - TEST-UNEXPECTED-FAIL | /builds/worker/workspace/build/tests/mozmill/account/test-mail-account-setup-wizard.js on TaskCluster

Status: RESOLVED FIXED

(Thunderbird :: Account Manager, defect)

Description

[Jorg K (CEST = GMT+2)]

TEST-UNEXPECTED-FAIL | /builds/worker/workspace/build/tests/mozmill/account/test-mail-account-setup-wizard.js | test-mail-account-setup-wizard.js::test_mail_account_setup
 
TEST-UNEXPECTED-FAIL | /builds/worker/workspace/build/tests/mozmill/account/test-mail-account-setup-wizard.js | test-mail-account-setup-wizard.js::test_bad_password_uses_old_settings

Only seen on TaskCluster runs.

Comment 1

[:aceman]

This one is harder, I do not see it locally right now.

The context is:
SUMMARY-UNEXPECTED-FAIL | test-mail-account-setup-wizard.js | test-mail-account-setup-wizard.js::test_mail_account_setup
   EXCEPTION: Timeout waiting for current config to become non-null
   at: utils.js line 409
      TimeoutError utils.js:409 13
      waitFor utils.js:447 11
      MozMillController.prototype.waitFor controller.js:687 3
      test_mail_account_setup/< test-mail-account-setup-wizard.js:93 5
      Runner.prototype.wrapper frame.js:584 7
      startTest test-window-helpers.js:326 11
      waitFor utils.js:440 5
      WindowWatcher_waitForModalDialog test-window-helpers.js:385 5
      wait_for_modal_dialog test-window-helpers.js:619 3
      open_mail_account_setup_wizard test-mail-account-setup-wizard.js:43 10
      test_mail_account_setup test-mail-account-setup-wizard.js:75 3
      Runner.prototype.wrapper frame.js:589 9
      Runner.prototype._runTestModule frame.js:659 9
      Runner.prototype.runTestModule frame.js:705 3
      Runner.prototype.runTestDirectory frame.js:529 7
      runTestDirectory frame.js:711 3
      Bridge.prototype._execFunction server.js:177 10
      Bridge.prototype.execFunction server.js:181 16
      Session.prototype.receive server.js:280 3
      AsyncRead.prototype.onDataAvailable server.js:88 3
SUMMARY-UNEXPECTED-FAIL | test-mail-account-setup-wizard.js | test-mail-account-setup-wizard.js::test_bad_password_uses_old_settings
   EXCEPTION: Timeout waiting for create button to be visible and active
   at: utils.js line 409
      TimeoutError utils.js:409 13
      waitFor utils.js:447 11
      MozMillController.prototype.waitFor controller.js:687 3
      test_bad_password_uses_old_settings/< test-mail-account-setup-wizard.js:181 7
      Runner.prototype.wrapper frame.js:584 7
      startTest test-window-helpers.js:326 11
      waitFor utils.js:440 5
      WindowWatcher_waitForModalDialog test-window-helpers.js:385 5
      wait_for_modal_dialog test-window-helpers.js:619 3
      open_mail_account_setup_wizard test-mail-account-setup-wizard.js:43 10
      test_bad_password_uses_old_settings test-mail-account-setup-wizard.js:163 3
      Runner.prototype.wrapper frame.js:589 9
      Runner.prototype._runTestModule frame.js:659 9
      Runner.prototype.runTestModule frame.js:705 3
      Runner.prototype.runTestDirectory frame.js:529 7
      runTestDirectory frame.js:711 3
      Bridge.prototype._execFunction server.js:177 10
      Bridge.prototype.execFunction server.js:181 16
      Session.prototype.receive server.js:280 3
      AsyncRead.prototype.onDataAvailable server.js:88 3

Comment 2

[:aceman]

Both of the lines failing (93 and 181) in the test file are waiting for some even. They already have an unusually high timeouts of 8 seconds so it could be it is already known they can take long.
We could at least try to increase the timeout to see if that is the problem. If it still doesn't work on TC it may the the even is never coming due to some other problem.

Comment 3

[Tom Prince [:tomprince]]

Attachment: Bug 1420695: Don't try to connect to fake ISPs in tests.

Review commit: https://reviewboard.mozilla.org/r/210750/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/210750/

Comment 4

[Ben Bucksch (:BenB)]

Comment on attachment 8940553 [details]
Bug 1420695: Don't try to connect to fake ISPs in tests.

r+

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/9a40dfc76b55
Don't try to connect to fake ISPs in tests; r=BenB

Comment 6

[Tom Prince [:tomprince]]

Apparently I was looking at the wrong tests results when checking this patch.

Comment 7

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/comm-central/rev/e51b1503b985
Backed out changeset 9a40dfc76b55 for not fixing the test. DONTBUILD

Comment 8

[:aceman]

Attachment: 1420695.patch: don't use nsIUserInfo

This fixes the test on TC Linux.
The problem was that on that particular machine returned an empty user's real name via nsIUserInfo (understandable on a server) and the test used that to populate the real name field in the account wizard. With the field empty, the "Next" button stayed disabled and no config discovery was ever started. Of course the test collapsed from that.

I don't understand why we populated the real name from the system, when the test actually had a pre-defined name in the 'user' object.
This was added in bug 29593, maybe by mistake. So I change the test to input the pre-defined name.

BenB, am I missing anything?
Also, the file check in fetchConfig.js is to silence an exception when there is no file (common situation) and readURLasUTF8() was called on the non-existent file. Can you please review these 2 changes?

Jorg, you can please check the rest which is just cleanup.

Try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=fd25815aa3dc02150e4900f49730731b1601675d

Comment 9

[Jorg K (CEST = GMT+2)]

Comment on attachment 8971425 [details] [diff] [review]
1420695.patch: don't use nsIUserInfo

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

Looks OK, I'll check the try run tomorrow. BIG THANKS!

::: mail/test/mozmill/account/test-mail-account-setup-wizard.js
@@ -92,5 @@
>      // XXX: This should probably use a notification, once we fix bug 561143.
>      awc.waitFor(() => awc.window.gEmailConfigWizard._currentConfig != null,
>                  "Timeout waiting for current config to become non-null",
>                  8000, 600);
> -    config = awc.window.gEmailConfigWizard.getConcreteConfig();

So that can be removed since 'config' is never used, right?

Comment 10

[:aceman]

(In reply to Jorg K (GMT+1) from comment #9)
> Comment on attachment 8971425 [details] [diff] [review]
> 1420695.patch: don't use nsIUserInfo
> 
> Review of attachment 8971425 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks OK, I'll check the try run tomorrow. BIG THANKS!

The try run is already done. Sadly there seems to be a different failure so it isn't green :(

> ::: mail/test/mozmill/account/test-mail-account-setup-wizard.js
> > -    config = awc.window.gEmailConfigWizard.getConcreteConfig();
> 
> So that can be removed since 'config' is never used, right?

Looks like unused to me. Maybe Ben could comment on that one too. Hopefully gEmailConfigWizard.getConcreteConfig() does not have some sideeffects that we want to trigger. But then a comment at the call would be in order.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/367d7c0fa00e
Don't take possibly empty user real name from the OS in the account setup tests. r=jorgk

Comment 12

[Jorg K (CEST = GMT+2)]

Sorry, Ben's availability is rather uncertain, so I didn't want to delay this any further. If he has nits, we can follow up.

Comment 13

[Jorg K (CEST = GMT+2)]

Comment on attachment 8971425 [details] [diff] [review]
1420695.patch: don't use nsIUserInfo

[Triage Comment]

Comment 14

[Ben Bucksch (:BenB)]

As for the username, what we prepopulate from the nsIUserInfo is just an aid to help the user. It's fine for an automated test to populate this input field with a hardcoded value, overwriting nsIUserInfo. Was that your question?

I was not involved in the automated tests, so I can't comment on the test code.

> the file check in fetchConfig.js is to silence an exception when there is no file (common situation)
> and readURLasUTF8() was called on the non-existent file.

Yes, this is designed to throw, and the exception is the normal case. The exception is caught right after, and the error handler called. The code is designed to be concise, and the "file exists" check is made during open anyway, that's why I didn't have that explicit "file exists" check.

I don't object the change, just asking: Why did you add that explicit check? Are you concerned about exceptions being slow? Given that we catch it, it shouldn't spill into the error console or anything. Does the test framework have a problem with it? If so, I would think that's rather a problem in the framework. Just asking.

Comment 15

[Ben Bucksch (:BenB)]

Comment on attachment 8971425 [details] [diff] [review]
1420695.patch: don't use nsIUserInfo

r=BenB for the non-test code

Comment 16

[:aceman]

The exception spills into the test log, via https://dxr.mozilla.org/comm-central/rev/8d636ad5810b5586433294c16cb2376d826fcca8/mail/components/accountcreation/content/emailWizard.js#574 so I saw it there. I wanted to silence it because it is not really a problem if the file is not found. I don't like exceptions for returning common expectable return values (non-exceptional). Notice I do call the errorCallback function.

But if it is designed to only return errors via exceptions, I can put it back, it is not really a problem, just a distraction for test log readers. As there is a ton of such messages, it is tedious to remember which ones can be ignored (and hardly scriptable to detect new errors/exceptions).

Comment 17

[Jorg K (CEST = GMT+2)]

Beta (TB 60 beta 6):
https://hg.mozilla.org/releases/comm-beta/rev/37a9e7a7f097d4a2e3bb3c59eda483cf1790e829