[Kde-pim] Review Request 109914: [IMAP] Use itemsRetrievedIncremental() when fetching missing bodies

[Andras Mantia]

I take the liberty to reply on the list only, as this is not related to the 
review itself imo.

On Wednesday, April 10, 2013 09:43:24 PM Kevin Ottens wrote:
> > On April 9, 2013, 6:24 a.m., Kevin Ottens wrote:
> > > Any chance to get that logic unit tested? The original submitter for
> > > this behavior didn't unit test and it shows now.> 
> > Dan Vrátil wrote:
> >     The code for retrieving missing bodies uses Akonadi::ItemFetchJob,
> >     which means the test would have to start a fake Akonadi server. I
> >     know there's an implementation in kdepimlibs, but I don't know how to
> >     reach it there from kdepim-runtime.> 
> > Dan Vrátil wrote:
> >     It also means that the RetrieveItemsTask test does not work at all at
> >     the moment.> 
> > Andras Mantia wrote:
> >     I didn't write a unit test as the whole tests with a fake server are
> >     unreliable for me (and cause configuration issues according to
> >     David). Neverthless we should fix that and have working and reliable
> >     unit tests. The patch itself looks good for me.
> 
> You know that's no good excuse right?

Not a good one, but it is an excuse. I have a better one though if you want: 
the original request was not rejected because of missing unit test. :)  That 
is, blocking this because of that makes no sense. Blaming me for not writing 
unit test for the original feature does.
That is why I said to commit this fix: without the fix, the original feature has 
bad side-effects for master users. We could either remove (disable) that or get 
this patch in. I find the patch in a better solution.

> The resource is designed to not talk
> to the akonadi server at all in test mode. I'm pretty sure it could be unit
> tested today. I'm really unimpressed at the current layering violation
> introduced. No Akonadi jobs should be used in the resource tasks, this way
> it's properly modular and testable. It's not the case anymore for
> AddCollectionTask and RetrieveItemsTask.


Well, this is news for me and again, was not mentioned for the original patch. 
The missing body checker can be moved out from RetrieveItemsTask (in a 
separate class or code that is in the main imapresource class), although the 
best place is there imo. If you want that, we can discuss.

Andras
_______________________________________________
KDE PIM mailing list kde-pim at kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/