Bug 71693 – [Qt] http/tests/notifications tests make fast/notifications/notifications-click-event.html fail

• Component: Tools / Tests
• Status: RESOLVED WONTFIX

[Csaba Osztrogonác]

You can easily reproduce this fail:
$Tools/Scripts/old-run-webkit-tests http/tests/notifications/icon-does-not-exist.html fast/notifications/notifications-click-event.html

fail:

[Csaba Osztrogonác]

fail:
--- /tmp/layout-test-results/fast/notifications/notifications-click-event-expected.txt	2011-11-07 16:35:36.762379664 +0000
+++ /tmp/layout-test-results/fast/notifications/notifications-click-event-actual.txt	2011-11-07 16:35:36.762379664 +0000
@@ -1,8 +1,6 @@
 DESKTOP NOTIFICATION: icon , title New E-mail, text Meet me tonight at 8!
-DESKTOP NOTIFICATION CLOSED: New E-mail
 Showing notifications.
 
 To exercise manually, grant notification permissions and load this page, then click on the notification. You should see a "PASS" message.
 
-PASS: click event fired.

[Csaba Osztrogonác]

One more culprit test which makes the other fail:

$ Tools/Scripts/old-run-webkit-tests --wait-for-httpd http/tests/notifications/icon-exists-show-alert-during-load.html fast/notifications/notifications-click-event.html

[Kristóf Kosztyó]

Created attachment 121094 [details]
proposed fix

This patch clear the m_notifications in the resetToConsistentStateBeforeTesting() so the notification tests don't cause side effect in the next notification test

[Simon Hausmann]

Your patch makes sense, but I'm curious about one bit: Why do the other ports not need this? Where do _they_ reset the state of their notification presenter?

AFAICS disconnectFrame() only resets permissions, but does that imply cancellation of any visible notifications, too?

[Kristóf Kosztyó]

(In reply to comment #4)
> Your patch makes sense, but I'm curious about one bit: Why do the other ports not need this? Where do _they_ reset the state of their notification presenter?

I have to look after it how it works on the other platforms.
This problem is caused by the m_notifications which is still containing the notifications from the previous tests when the flaky one is running so the "click" sometimes goes to the wrong notification.

> 
> AFAICS disconnectFrame() only resets permissions, but does that imply cancellation of any visible notifications, too?

I don't know, I will check this too.

[Kristóf Kosztyó]

(In reply to comment #5)
> (In reply to comment #4)
> > Your patch makes sense, but I'm curious about one bit: Why do the other ports not need this? Where do _they_ reset the state of their notification presenter?
> 
> I have to look after it how it works on the other platforms.
> This problem is caused by the m_notifications which is still containing the notifications from the previous tests when the flaky one is running so the "click" sometimes goes to the wrong notification.
> 
> > 
> > AFAICS disconnectFrame() only resets permissions, but does that imply cancellation of any visible notifications, too?
> 
> I don't know, I will check this too.

I looked after it in the chromium, and I found this in the changelog:
+        Adds necessary hooks to chromium's DRT so that clicks on desktop notifications
+        can be simulated during a layout test.  Requires storing a list of active
+        notifications so that they can be referred to later for clicking.
+        https://bugs.webkit.org/show_bug.cgi?id=44800

That means it is normal to the previous test's notification is still reachable btw there isn't any tests what work this way and this can cause sideeffects like this one.

[Yael]

(In reply to comment #4)
> Your patch makes sense, but I'm curious about one bit: Why do the other ports not need this? Where do _they_ reset the state of their notification presenter?
> 
Other ports do not run http/notifications tests, so they don't have this problem.

[Yael]

Comment on attachment 121094 [details]
proposed fix

This patch makes sense to me.
We identify notifications by their title, but all of the test notifications use the same title. That's what causes the side effect.
IMO it is ok to clear the notifications between tests, even if in the real world we would not cancel notifications when navigating away from the page that created them.

[WebKit Review Bot]

Comment on attachment 121094 [details]
proposed fix

Rejecting attachment 121094 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
ines).
patching file Source/WebKit/qt/WebCoreSupport/NotificationPresenterClientQt.cpp
patching file Source/WebKit/qt/WebCoreSupport/NotificationPresenterClientQt.h
patching file Tools/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp
Hunk #1 succeeded at 555 (offset -4 lines).

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Simon Haus..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/11856198

[Csaba Osztrogonác]

Arghhhh ... why wasn't it committed? Kristóf?

Otherwise http/tests/notifications tests were removed from trunk - https://trac.webkit.org/changeset/115153

Do we still need this fix?

[Csaba Osztrogonác]

ping?

[Csaba Osztrogonác]

notifications and Qt isn't in WebKit trunk long time ago.