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

Note that many tests started failing recently. This bug here, but also bug 1499240 and bug 1499286.

Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/a15d82e95a29 Temporarily switch off failing test-mail-account-setup-wizard.js::test_bad_password_uses_old_settings on Linux. rs=bustage-fix DONTBUILD

In some other bugs test failures were fixed by changing oncommand to oninput. But here we're out of luck since oninput is already used: https://searchfox.org/comm-central/search?q=%3Ctextbox&case=false&regexp=false&path=emailWizard.xul

The other failure in this test is 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 This is the code that fails: awc.waitFor(() => awc.window.gEmailConfigWizard._currentConfig != null, "Timeout waiting for current config to become non-null", 8000, 600); since the log says: EXCEPTION: Timeout waiting for current config to become non-null

I'm fairly certain this has something to do with bug 1437064. Removing lines 17 and 34 of fetchConfig.js fixes the test. This whole component needs modernisation. :( https://hg.mozilla.org/comm-central/file/tip/mail/components/accountcreation/content/fetchConfig.js#l17

Which test? Or both sub-tests?

Both. At least, it does on my machine.

What is |return new TimeoutAbortable(runAsync(function() {| meant to do? Could you send a patch to try with this change that re-enables the switched-off test, please. If it works, I'll r+ it. It was your idea.

As far as I can see, it doesn't actually do anything useful. runAsync is shorthand for setTimeout with 0 wait. TimeoutAbortable lets you cancel the timeout, which has already started because it has no wait time. You'd have to cancel it before the end of the event loop that created it, to actually cancel it. That said, I don't think we should fix it this way. There's something wrong with bug 1437064 that needs to be fixed. Backing it out also fixes bug 1499240, and (I assume) would've fixed bug 1499286: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=df518c915dfa19353928e927ec66f6af78c6c48e

Hmm, so my regression range in bug 1499240 comment #0 was right after all. You want to start a discussion with the M-C authors over there? Now I'm confused what we should to. Why not remove the un-useful code and fix the tests that way?

Sorry, I see you started that discussion in bug 1501138.

Let's see how far we get with this.

Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/8c530b9c0f4b Tweak fetchConfig.js to fix test failures in MozMill account/test-mail-account-setup-wizard.js. rs=bustage-fix https://hg.mozilla.org/comm-central/rev/d303f46daae4 Backed out changeset a15d82e95a29 to re-enable test. a=backout

Beta (TB 64): https://hg.mozilla.org/releases/comm-beta/rev/53867f882e8eb69b45c1553cc96921ba0b9908ef https://hg.mozilla.org/releases/comm-beta/rev/a7e374bef2b8405098b322c2da12ca3be8ec6c46

Comment on attachment 9020002 [details] [diff] [review] 1499316-temporary-tweak.patch Hey Jörg, Can you please make sure that such hacks get proper review and are fixed properly at least after the fact? This change is an API violation, because the function is supposed to return an Abortable object, and now it doesn't anymore. Luckily, the code happens to work, but that's only due to a co-incidence, and this hack is still very broken. I've found this only by coincidence, because my build for bug 1500105 was broken due to this. This code is now actually worse than before. Before, it was a test failure, but the code was correct. Now the test passes, but the code is wrong. :-( I'd just like to ensure that such things are not done quickly and then forgotten, because they can wreak real havok later on.

Comment on attachment 9020002 [details] [diff] [review] 1499316-temporary-tweak.patch r-

Sorry, I mis-addressed the last comment. The idea was actually from Geoff. > There's something wrong with bug 1437064 that needs to be fixed. Geoff, could you please pass this info upstream in that bug? Regarding the patch here, this needs to be re-visited. The old code was correct. Either the test is wrong, or the Gecko change in bug 1437064 is broken.

This patch is still a workaround and avoids the timeout, but at least adheres to the function API. The real fix would need to find out why the test fails. I have no idea. But I'm fairly sure that the application code was fine.

Comment on attachment 9020002 [details] [diff] [review] 1499316-temporary-tweak.patch (In reply to Ben Bucksch (:BenB) from comment #16) > Comment on attachment 9020002 [details] [diff] [review] > 1499316-temporary-tweak.patch > r- That's not constructive. This was landed and even uplifted. I understand you don't like it, but your patch doesn't revert the change, so it must have been a step in the right direction.

Comment on attachment 9020002 [details] [diff] [review] 1499316-temporary-tweak.patch Jörg, I understand that this was a hot fix. We said that hot fixes that fix build breakge are acceptable without review, but that they must be reviewed after the fact. This is simply such a post-land review. My new patch just fixes the *new* bug introduced by the previous temp patch, but I think the problem is somewhere else entirely. The old app code was correct, and the cause of the test failure hasn't been found yet. I am just trying to get to a proper fix for the problem here.

Comment on attachment 9020002 [details] [diff] [review] 1499316-temporary-tweak.patch r- would mean that we need to back it out. You're not suggesting to back it out.

Comment on attachment 9022720 [details] [diff] [review] Fix, v3 - Better temporary fix that at least adheres to the API Sure, thanks.

Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/0a2c39228302 Follow-up: Make fetchConfigFromDisk() return Abortable() again. r=darktrojan

This was indeed a Gecko bug, which has been fixed in bug 1501138. I'll attach a patch to revert the changes done here. This makes our code correct again.

Comment on attachment 9023862 [details] [diff] [review] Revert changes, go back to correct original code [Approval Request Comment] Regression caused by (bug #): Bug 1437064 User impact if declined: None Testing completed (on c-c, etc.): None, this is the original code

comm-beta uplift landing needs to wait for mozilla-beta landing of bug 1501138.

Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/5f68c3830a75 Backed out changesets 8c530b9c0f4b and 0a2c39228302 to restore code to original state. a=backout

Comment on attachment 9023862 [details] [diff] [review] Revert changes, go back to correct original code Unless I'm missing something, you forgot to revert the change in the middle (which is surprising, since it was your own change): https://hg.mozilla.org/comm-central/rev/5f68c3830a75#l1.23

Yes, thanks.

Beta (TB 64 beta 3): https://hg.mozilla.org/releases/comm-beta/rev/74fdd2657b94