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

[Albert Astals Cid]

> On March 18, 2016, 5: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
> 
> Martin Klapetek wrote:
>     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.

ok, a bug to fix/discuss later in time :)


- Albert


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


On March 16, 2016, 8: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, 8: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/fb14a003/attachment-0001.html>