Closed Bug 1165320 Opened 10 years ago Closed 10 years ago

TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/notification/test-notification.js

Categories

(Thunderbird :: General, defect)

Product:
Thunderbird
Component:
General
Platform:
All
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(thunderbird38 fixed, thunderbird39 fixed, thunderbird40 fixed, thunderbird41 fixed)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 41.0
Tracking Flags:
Tracking Status
thunderbird38 --- fixed
thunderbird39 --- fixed
thunderbird40 --- fixed
thunderbird41 --- fixed

People

(Reporter: mkmelin, Assigned: rsx11m.pub)

References

Details

(Keywords: intermittent-failure, regression)

Attachments

(2 files, 1 obsolete file)

Possible workaround
1.34 KB, patch
mkmelin
: review+
mkmelin
: approval-comm-aurora+
mkmelin
: approval-comm-beta+
Details | Diff | Splinter Review
Follow-up patch (v2)
1.42 KB, patch
mkmelin
: review+
rkent
: approval-comm-aurora+
rkent
: approval-comm-beta+
Details | Diff | Splinter Review
Seems these are all broken on linux, from something checked in during the build bustage? Bug 1144693? TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/notification/test-notification.js | test-notification.js::test_new_mail_received_causes_notification TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/notification/test-notification.js | test-notification.js::test_show_oldest_new_unread_since_last_notification TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/notification/test-notification.js | test-notification.js::test_notification_works_across_accounts TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/notification/test-notification.js | test-notification.js::test_notifications_independent_across_accounts TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/notification/test-notification.js | test-notification.js::test_show_subject TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/notification/test-notification.js | test-notification.js::test_hide_subject TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/notification/test-notification.js | test-notification.js::test_show_only_subject TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/notification/test-notification.js | test-notification.js::test_show_sender TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/notification/test-notification.js | test-notification.js::test_hide_sender TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/notification/test-notification.js | test-notification.js::test_show_only_sender TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/notification/test-notification.js | test-notification.js::test_show_preview TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/notification/test-notification.js | test-notification.js::test_hide_preview TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/notification/test-notification.js | test-notification.js::test_show_only_preview TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/notification/test-notification.js | test-notification.js::test_still_notify_with_unchanged_biff TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/notification/test-notification.js | test-notification.js::test_no_notification_for_uninteresting_folders
(In reply to Magnus Melin from comment #0) > Seems these are all broken on linux, from something checked in during the > build bustage? Bug 1144693? This is weird, that patch works around the libnotify failures by switching back to the built-in (="new") notifications. If the test explicitly expects a response from libnotify rather than that notification it would fail, but persistently rather than intermittently. Do you see failures prior to comm-central changeset d39bdd7c8e4c, pushed May 07 21:51:53 UTC? > TEST-UNEXPECTED-FAIL | > /builds/slave/test/build/tests/mozmill/notification/test-notification.js | > test-notification.js::test_show_sender > TEST-UNEXPECTED-FAIL | > /builds/slave/test/build/tests/mozmill/notification/test-notification.js | > test-notification.js::test_show_only_sender I would expect those and the respective subject tests to fail with libnotify due to bug 1152773 prior to switching the default; if they proceed(ed), something seems to be wrong with those tests. I don't have Mozmill set up on my machine, thus can't investigate further.
Flags: needinfo?(mkmelin+mozilla)
The comm-central tree was busted when this checked in, thus hard to tell if it happened exactly with that push. On the other hand, I can't see it in any Mozmill log before the bustage, but in all Linux logs after the bustage. Since there aren't many other candidates, bug 1144693 indeed seems a likely candidate. Given that it worked with libnotify enabled by default (notwithstanding that the regression documented in bug 1152773 wasn't caught by the current tests), I'd suggest to simply add some statements at the beginning of the tests to enable it setting mail.biff.use_system_alert to "true" and thus going back to the pre-1144693 state for now as far as the tests are concerned.
OS: Unspecified → Linux
Hardware: Unspecified → All
(In reply to rsx11m from comment #14) > The comm-central tree was busted when this checked in, thus hard to tell if > it happened exactly with that push. Checked comm-aurora and comm-beta, and it's less ambiguous there. It was pushed alongside bug 1138154, but that's plugins and not related to notifications. Mozmill test failures occur with and after that push.
Untested patch that explicitly sets mail.biff.use_system_alert=true in setupModule(), thus will hopefully be picked up by all subsequent subtests. The underlying problem may be that the tests use a "Mock Alerts Service" defined just before the line I've added, whereas the behavior for mail.biff.use_system_alert=false skips the step of trying the alert service with alertsService->ShowAlertNotification() and goes straight to ShowNewAlertNotification(). mail.biff.use_system_alert=true will try ShowAlertNotification() first and fall back to ShowNewAlertNotification() only if the alert service reports an exception.
Flags: needinfo?(mkmelin+mozilla)
Attachment #8606364 - Flags: review?(mkmelin+mozilla)
(In reply to rsx11m from comment #13) > I don't have Mozmill set up on my machine, thus can't investigate further. If you have a build, you have mozmill (unless you disabled test explicitly). Then in the obj-dir run make SOLO_TEST=notification/test-notification.js mozmill-one
Comment on attachment 8606364 [details] [diff] [review] Possible workaround Review of attachment 8606364 [details] [diff] [review]: ----------------------------------------------------------------- Yeah the other option would be to add linux to the tests EXCLUDED_PLATFORMS, but then the test wouldn't be used much so I think this is fine.
Attachment #8606364 - Flags: review?(mkmelin+mozilla) → review+
Assignee: nobody → rsx11m.pub
I'll push this in a bit
https://hg.mozilla.org/comm-central/rev/ccba868cf370 -> FIXED
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 41.0
Comment on attachment 8606364 [details] [diff] [review] Possible workaround [Triage Comment] Fix test bustages
Attachment #8606364 - Flags: approval-comm-beta+
Attachment #8606364 - Flags: approval-comm-aurora+
Sorry, this needs a follow-up patch. While Linux tests are fixed now, I see failures on Mac OSX and Windows. The reason being that mail.biff.use_system_alert is only defined on Linux, not on the other platforms, thus remember_and_set_bool_pref() throws an exception. > TEST-UNEXPECTED-FAIL | C:\slave\test\build\tests\mozmill\notification\test-notification.js | test-notification.js::setupModule > TEST-UNEXPECTED-FAIL | (runtestlist.py) | Exited with code 1 during directory run The obvious solution is to restrict this statement to Linux platforms. I'll post a patch later.
Status: RESOLVED → REOPENED
status-thunderbird38: fixed → affected
status-thunderbird39: fixed → affected
status-thunderbird40: fixed → affected
status-thunderbird41: fixed → affected
Resolution: FIXED → ---
I can create the patch relative to attachment 8606364 [details] [diff] [review], thus no need to back it out.
Attached patch Follow-up patch (obsolete) — — Splinter Review
This attempts to switch mail.biff.use_system_alert only if the platform is identified as Linux and works for me running the test per comment #17 (thanks!). Alternatively, I could have tested if a default value exists and only then perform the assignment, which would be more generic; but on the other hand, testing that it only happens on Linux as a platform should be part of the test, given that the pref is #ifdefed for XP_UNIX in bug 1144719 to start with.
Attachment #8606568 - Flags: review?(mkmelin+mozilla)
On a side note, bug 1144719 landed on comm-esr38 already whereas bug 1144693 for switching the default only landed on the default comm-beta branch but not on THUNDERBIRD_38_VERBRANCH. Can you (rkent) have a look at this when pushing the follow-up patch to the branches?
Bug 1144693 landed in TB 38 at rev 22077 while THUNDERBIRD_38_VERBRANCH begins at rev 22082. See http://hg.mozilla.org/releases/comm-beta/shortlog/22082 for the branch point and revs before.
Comment on attachment 8606568 [details] [diff] [review] Follow-up patch Review of attachment 8606568 [details] [diff] [review]: ----------------------------------------------------------------- I'm not sure how this plays out in other unixes... You may want to use let isLinux = ("@mozilla.org/gio-service;1" in Components.classes); ::: mail/test/mozmill/notification/test-notification.js @@ +104,5 @@ > remember_and_set_bool_pref("mail.biff.show_alert", true); > > // Ensure that system notifications are used (relevant for Linux only) > + if (Services.appinfo.OS === "Linux") > + remember_and_set_bool_pref("mail.biff.use_system_alert", true); nit: usually just using == in mozilla code
Attachment #8606568 - Flags: review?(mkmelin+mozilla) → review+
> I'm not sure how this plays out in other unixes... Let's test all variants then. The rationale for going with Services.appinfo.OS was that it's the easiest identification for Linux (and since this code is running only on test machines which /are/ Linux it shouldn't be a problem at this time), but it makes sense to be more comprehensive in case testing platforms are expanded in the future.
Attachment #8606568 - Attachment is obsolete: true
Attachment #8606644 - Flags: review?(mkmelin+mozilla)
(In reply to Kent James (:rkent) from comment #29) > Bug 1144693 landed in TB 38 at rev 22077 while THUNDERBIRD_38_VERBRANCH begins at rev 22082. Right, thanks - guess I didn't follow all those lines in the graph view correctly. ;-)
Attachment #8606644 - Flags: review?(mkmelin+mozilla) → review+
Thanks, push for comm-central please (and hopefully fixed now for all platforms).
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/1abdcb3e3035
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Hmm, I planned to wait with branch-approval requests until the comm-central tests verifiably pass for all platforms, but now it's busted due to bug 1165737. Guess it can't hurt to wait a little longer...
status-thunderbird41: affected → fixed
Comment on attachment 8606644 [details] [diff] [review] Follow-up patch (v2) Test bustage fix, including THUNDERBIRD_38_VERBRANCH. [Approval Request Comment] Regression caused by (bug #): bug 1144693 User impact if declined: none, test bustage on Windows and Mac OSX only Testing completed (on c-c, etc.): trunk tinderbox test passes on all platforms now Risk to taking this patch (and alternatives if risky): none
Attachment #8606644 - Flags: approval-comm-beta?
Attachment #8606644 - Flags: approval-comm-aurora?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: