[Kmymoney-devel] Review Request: show another icon on homepage accounts tables if there are transactions after the last online transaction

[Łukasz Maszczyński]

> On Dec. 30, 2012, 9:19 a.m., Łukasz Maszczyński wrote:
> > kmymoney/mymoney/mymoneyfiletest.cpp, line 2080
> > <http://git.reviewboard.kde.org/r/107619/diff/2/?file=102889#file102889line2080>
> >
> >     Is it really required to call testAddAccounts() here? I imagine this is the least effort to setup the ground for your test, but I'd rather extract some part of testAddAccounts() (it tests more than a single scenario, so this should not be a problem) into a new test and use that one.
> 
> Marko Käning wrote:
>     I used the same approach for the test case as back then for MyMoneyFile::hasMatchingOnlineBalance()!
>     
>     Other tests also make use of testAddAccounts() which is why I thought it was advisable to do so. (I also thought it would be better to re-use code which is tested in itself.)

You're of course right that reusing code is good, and I'm not trying to convince you to create a new helper method _just_ for the purpose of the new test. I also understand why you chose to use testAddAccounts() - that's the fastest way to write the test.

But I don't think this is the best approach. Don't treat this personally - I also think that using testAddAccounts() should be revised in other tests as well, let me explain why:
testAddAccounts() tests several different behavious - around 5, from a brief look at it. Creating an account and verifying it's been created properly is one behaviour, the one you need in your test. But testAddAccounts() does far more than that - it tests 4 more behavious, and none of these are desirable from the perspective of your test. Yet, they are executed upon testAddAccounts() call.

My proposal is to extract the part of testAddAccounts() which creates the first account into a new test method (say testAddAccountSuccessfully(), or sth like that), and reuse that test in testAddAccounts() and your own. Sorry if I didn't make that point clear previously.


> On Dec. 30, 2012, 9:19 a.m., Łukasz Maszczyński wrote:
> > kmymoney/mymoney/mymoneyfiletest.cpp, lines 2105-2108
> > <http://git.reviewboard.kde.org/r/107619/diff/2/?file=102889#file102889line2105>
> >
> >     I'd split it into two test functions - you're testing two independent cases after all.
> 
> Marko Käning wrote:
>     I actually had 3 test cases encapsulated in one test in review 103264 which was said to be fine back then, which is why I followed the same strategy here.

In KMM you can see a lot of test methods testing several behaviors at once, that's true. However, in my opinion it's not the best approach possible.

A quote from developerforce.com explains really well why this should be avoided:
"Your unit tests should only test one aspect of your code at a time, so that it is easy to understand the purpose of each unit test. Properly written, and well-decomposed unit tests are an excellent source of documentation. This may mean that you will need to write multiple unit tests in order to fully exercise one method."


- Łukasz


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/107619/#review24223
-----------------------------------------------------------


On Dec. 29, 2012, 10:05 p.m., Marko Käning wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107619/
> -----------------------------------------------------------
> 
> (Updated Dec. 29, 2012, 10:05 p.m.)
> 
> 
> Review request for KMymoney.
> 
> 
> Description
> -------
> 
> The homepage accounts tables now show a different icon if there are transactions for an account after the last online transaction - highlighting for the user the necessity to download transactions in order to verify the account status.
> 
> 
> Diffs
> -----
> 
>   kmymoney/mymoney/mymoneyfile.h c43977ce7413eee2bc1e0a841fa548314c71e9df 
>   kmymoney/mymoney/mymoneyfile.cpp 6640356d5e07152f8eb4aecff23d62ee8d853dbd 
>   kmymoney/mymoney/mymoneyfiletest.h 5551fa94b6b34022c8e91c0301847d5479e6b1f2 
>   kmymoney/mymoney/mymoneyfiletest.cpp feb9d57ecd7156fc1eea189a905fa797aeae9183 
>   kmymoney/views/khomeview.cpp c79337176b7265cabe15cad4972bc719e797af7c 
> 
> Diff: http://git.reviewboard.kde.org/r/107619/diff/
> 
> 
> Testing
> -------
> 
> Built and ran application and test cases successfully.
> 
> 
> Thanks,
> 
> Marko Käning
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kmymoney-devel/attachments/20121230/a451c3f0/attachment-0001.html>