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

RESOLVED FIXED in Thunderbird 61.0

Status

defect
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: jorgk, Assigned: aceman)

Tracking

Thunderbird 61.0

Thunderbird Tracking Flags

(thunderbird60 fixed, thunderbird61 fixed)

Details

(Whiteboard: [Thunderbird-testfailure: Z all TC])

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

2 years ago
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.
Reporter

Updated

2 years ago
Whiteboard: [Thunderbird-testfailure: Z all TC]
Assignee

Comment 1

2 years ago
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
Assignee

Comment 2

2 years ago
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 hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8940553 - Flags: feedback?(ben.bucksch)
Comment on attachment 8940553 [details]
Bug 1420695: Don't try to connect to fake ISPs in tests.

r+
Attachment #8940553 - Flags: review+

Updated

2 years ago
Attachment #8940553 - Flags: feedback?(ben.bucksch) → feedback+

Comment 5

2 years ago
Pushed by mozilla@hocat.ca:
https://hg.mozilla.org/comm-central/rev/9a40dfc76b55
Don't try to connect to fake ISPs in tests; r=BenB
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Apparently I was looking at the wrong tests results when checking this patch.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 7

2 years ago
Backout by mozilla@hocat.ca:
https://hg.mozilla.org/comm-central/rev/e51b1503b985
Backed out changeset 9a40dfc76b55 for not fixing the test. DONTBUILD
Assignee

Comment 8

Last year
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
Assignee: nobody → acelists
Attachment #8940553 - Attachment is obsolete: true
Status: REOPENED → ASSIGNED
Attachment #8971425 - Flags: review?(jorgk)
Attachment #8971425 - Flags: review?(ben.bucksch)
Reporter

Comment 9

Last year
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?
Attachment #8971425 - Flags: review?(jorgk) → review+
Assignee

Comment 10

Last year
(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

Last year
Pushed by mozilla@jorgk.com:
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
Status: ASSIGNED → RESOLVED
Closed: 2 years agoLast year
Resolution: --- → FIXED
Reporter

Comment 12

Last year
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.
Target Milestone: --- → Thunderbird 61.0
Reporter

Comment 13

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

[Triage Comment]
Attachment #8971425 - Flags: approval-comm-beta+
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 on attachment 8971425 [details] [diff] [review]
1420695.patch: don't use nsIUserInfo

r=BenB for the non-test code
Attachment #8971425 - Flags: review?(ben.bucksch) → review+
Assignee

Comment 16

Last year
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).
You need to log in before you can comment on or make changes to this bug.