Review Request 127398: Add unit tests for KNotification and fix a whole bunch of issues uncovered thanks to them

[Martin Klapetek]

> On March 18, 2016, 6:20 a.m., Anthony Fieroni wrote:
> > src/knotification.cpp, line 63
> > <https://git.reviewboard.kde.org/r/127398/diff/1/?file=453383#file453383line63>
> >
> >     ref can't be UINT_MAX, but id can, no? Negative rage will brake all, it's this possible, maybe not. Counters it's not good idea to be signed.
> 
> Martin Klapetek wrote:
>     Yes...if your application will emit more than 32767 notifications in its lifetime. At which point the application is probalby broken.
> 
> Albert Astals Cid wrote:
>     Is it? I don't use KTP, but assuming KTP sends Knotifications and assuming you get like 1 chat notification per minute (not crazy imho) get say around 500 notifications per day, so in 66 days you'd go over the limit. Sure it's corner-case-y but i would not call it impossible nor the app being broken

This wouldn't actually break in the KTp case (in normal KNotification usage, the id is not really needed), however this code (going back to 2009) apparently chose real-life pragmatism over such corner cases.

To expand on that, the id uses values -1 and -2 to indicate "invalid" and "about to be deleted" states. I can either introduce some bool/enum making every "if (..)" twice as complicated, or start the counter from 2, reserving 0 and 1. Tbh given this wasn't a problem for the past 7 years, I don't think the code needs to be more complex making special assumptions for highly unlikely corner cases.


- Martin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127398/#review93676
-----------------------------------------------------------


On March 16, 2016, 9:23 p.m., Martin Klapetek wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127398/
> -----------------------------------------------------------
> 
> (Updated March 16, 2016, 9:23 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: knotifications
> 
> 
> Description
> -------
> 
> Adds basic set of unit tests including fake notifications server.
> 
> This helped uncover and fix these issues:
> 
> * Calling close() on KNotification that was not "sent" would not emit closed() and would not delete it
> * Closing a notification can delete the KNotification object prematurely
> * Invoking an action leads to unnecessary dbus roundtrip
> * Invoking an action would fail to properly close and delete the KNotification object
> 
> 
> Diffs
> -----
> 
>   CMakeLists.txt 6d09051 
>   autotests/CMakeLists.txt PRE-CREATION 
>   autotests/fake_notifications_server.h PRE-CREATION 
>   autotests/fake_notifications_server.cpp PRE-CREATION 
>   autotests/knotification_test.cpp PRE-CREATION 
>   autotests/knotifications5/qttest.notifyrc PRE-CREATION 
>   autotests/qtest_dbus.h PRE-CREATION 
>   src/knotification.cpp 17b0be2 
>   src/knotificationmanager.cpp e80d48c 
>   src/knotificationplugin.cpp acf964c 
>   src/notifybypopup.cpp b9489c1 
> 
> Diff: https://git.reviewboard.kde.org/r/127398/diff/
> 
> 
> Testing
> -------
> 
> Unit tests all pass.
> 
> 
> Thanks,
> 
> Martin Klapetek
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20160321/9f3a0d5a/attachment.html>