Closed Bug 1541136 Opened 6 years ago Closed 6 years ago

TEST-UNEXPECTED-FAIL | [snip]mozmill/testBasicFunctionality.js | testBasicFunctionality.js::testSmokeTest and TEST-UNEXPECTED-FAIL | [snip]/mozmill/testLocalICS.js | testLocalICS.js::testLocalICS

Categories

(Calendar :: General, defect)

Product:
Calendar
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: jorgk-bmo, Assigned: darktrojan)

References

Details

(Whiteboard: [Thunderbird-testfailure: Z all][Thunderbird-disabled-test])

Attachments

(3 files, 1 obsolete file)

1541136-calendar-wizardpage-1.diff
(deleted), patch
Fallen
: review+
Details | Diff | Splinter Review
1541136-mail-wizardpage-2.diff
(deleted), patch
aceman
: review+
Details | Diff | Splinter Review
1541136-follow-up.patch
(deleted), patch
aceman
: review+
Details | Diff | Splinter Review

Fresh bustage, just in :-(

TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1554226816/build/tests/mozmill/testBasicFunctionality.js | testBasicFunctionality.js::testSmokeTest
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1554226816/build/tests/mozmill/testLocalICS.js | testLocalICS.js::testLocalICS

Log says:
17:50:58 INFO - SUMMARY-UNEXPECTED-FAIL | testBasicFunctionality.js | testBasicFunctionality.js::testSmokeTest
17:50:58 INFO - EXCEPTION: menuitem is null
17:50:58 INFO - at: test-calendar-utils.js line 627
17:50:58 INFO - menulistSelect test-calendar-utils.js:627 5
17:50:58 INFO - handleNewCalendarWizard test-calendar-utils.js:577 5
17:50:58 INFO - testSmokeTest/< testBasicFunctionality.js:85 9

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ea09774456976ada64ed753a00724ef6bc&tochange=92d1c344e7c53af9b8ad8a4192d93478f7

Looks like
Bug 1498569, Replace wizard.xml attributes with event listeners, r=Gijs

changed <wizardpage>

Flags: needinfo?(geoff)
Whiteboard: [Thunderbird-testfailure: Z all]
Keywords: leave-open
Whiteboard: [Thunderbird-testfailure: Z all] → [Thunderbird-testfailure: Z all][Thunderbird-disabled-test]
Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/46eb5a67c089 temporarily disable testBasicFunctionality.js and testLocalICS.js. rs=bustage-fix

You're right, the <wizardpage> change broke things. This patch only fixes the instances in calendar, there are more. I will look at them shortly.

Assignee: nobody → geoff
Status: NEW → ASSIGNED
Attachment #9055405 - Flags: review?(philipp)
Attached patch 1541136-mail-wizardpage-1.diff (obsolete) (deleted) — — Splinter Review

The non-calendar code that needed the same changes, but clearly doesn't break any tests without the changes.

Flags: needinfo?(geoff)
Attachment #9055408 - Flags: review?(acelists)
Pushed by geoff@darktrojan.net: https://hg.mozilla.org/comm-central/rev/70c4a25e3eaf Backed out changeset 46eb5a67c089 to re-enable tests https://hg.mozilla.org/comm-central/rev/cb6bda3df23a Replace on* attributes of <wizardpage> with addEventListener, calendar only; rs=bustage-fix https://hg.mozilla.org/comm-central/rev/ae892d0e67b2 Replace on* attributes of <wizardpage> with addEventListener, non-calendar; rs=bustage-fix DONTBUILD
Comment on attachment 9055408 [details] [diff] [review] 1541136-mail-wizardpage-1.diff Review of attachment 9055408 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, but you haven't tested this much. Any of the wizard dialogs is broken. Please also remove wizard.xml from security.uris_using_eval_with_system_principal as m-c has done, if we do not use any other evals in it. ::: mail/components/im/content/imAccountWizard.js @@ +9,5 @@ > var {MailServices} = ChromeUtils.import("resource:///modules/MailServices.jsm"); > > var PREF_EXTENSIONS_GETMOREPROTOCOLSURL = "extensions.getMoreProtocolsURL"; > > +document.getElementById("accountprotocol").addEventListener("pageadvanced", accountWizard.selectProtocol); TypeError: document.getElementById(...) is null imAccountWizard.js:13:10 TypeError: accountWizard is undefined imAccountWizard.xul:1:1 ::: mailnews/base/prefs/content/AccountWizard.js @@ +73,5 @@ > + document.documentElement.canAdvance = true; > + }); > + accounttypePage.addEventListener("pageadvanced", acctTypePageUnload); > + let identityPage = document.getElementById("identitypage"); > + identityPage.addeEventListener("pageshow", identityPageInit); typo: addeEventListener ::: mailnews/extensions/newsblog/content/feedAccountWizard.js @@ +6,5 @@ > var {FeedUtils} = ChromeUtils.import("resource:///modules/FeedUtils.jsm"); > > +document.getElementById("accountsetuppage").addEventListener( > + "pageshow", FeedAccountWizard.accountSetupPageValidate > +); TypeError: document.getElementById(...) is null in feedAccountWizard.js:8:10
Attachment #9055408 - Flags: review?(acelists)
Attached patch 1541136-mail-wizardpage-2.diff (deleted) — — Splinter Review

Let's try that again.

Attachment #9055408 - Attachment is obsolete: true
Attachment #9055649 - Flags: review?(acelists)

Thanks Geoff! New patch seems to work fine for me on the Feeds and Newsgroups wizard at least.

Keywords: leave-open
Attached patch 1541136-follow-up.patch (deleted) — — Splinter Review

This is the difference between version 1 and version 2 of the non-calendar part.

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/007c7ec19799
Replace on* attributes of <wizardpage> with addEventListener, non-calendar, follow-up. rs=bustage-fix,jorgk DONTBUILD

Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Pushed by geoff@darktrojan.net: https://hg.mozilla.org/comm-central/rev/6ae5ec3f014b Follow-up: remove one remaining onwizardfinish attribute; rs=me DONTBUILD
Comment on attachment 9055725 [details] [diff] [review] 1541136-follow-up.patch Review of attachment 9055725 [details] [diff] [review]: ----------------------------------------------------------------- Thanks. ::: mailnews/extensions/newsblog/content/feedAccountWizard.js @@ +9,5 @@ > var FeedAccountWizard = { > accountName: "", > > + onLoad() { > + document.documentElement.addEventListener("wizardfinish", this.onFinish.bind(this)); What about the 'wizardcancel' event? Where did that go?
Attachment #9055725 - Flags: review+

You were meant to review the other patch, version 2, not the difference between v1 and v2 which I landed :-(

The feed wizard cancel was a no-op returning true to allow the panel to close, which is no longer needed, at least that's my understanding.

Comment on attachment 9055649 [details] [diff] [review] 1541136-mail-wizardpage-2.diff Review of attachment 9055649 [details] [diff] [review]: ----------------------------------------------------------------- OK, this looks better now. But creating some accounts still shows errors, I'll file that separately as I am not sure if that is caused by these changes.
Attachment #9055649 - Flags: review?(acelists) → review+

(In reply to Jorg K (GMT+2) from comment #14)

The feed wizard cancel was a no-op returning true to allow the panel to close, which is no longer needed, at least that's my understanding.

Exactly this.

Target Milestone: --- → 7.0
Attachment #9055405 - Flags: review?(philipp) → review+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: