Code review comment for lp:~vauxoo/openobject-addons/trunk-bug_887376-account_move_compute_with_btree-moylop260

On 01/17/2012 03:49 PM, Nhomar Hernandez (Vauxoo) wrote:
> Can somebody tell me what original YAML i can use to test this feature.

There is probably not much to reuse in the existing YAML tests, as none
are specifically designed to verify the most complex cases of
consolidated charts.

Because this patch touches very core accounting features
(credit/debit/balance computation!), it is very sensitive, and proving
that it does not affect the results is the first step towards getting it
merged.

It also bypasses the aggregation logic used by the rest of the system
(get_children_and_..), so its correctness needs to be assessed even more.

> LEt me explain :
>
> How we test this feature.
>
> I have 1 year of real data:
> We have in the Account CHart ALL internal type of accounts (consolidation was the more difficult to test and solve issues)
> We KNOW results for financial reports.
> We compare MAnually 5 or 6 key accounts (download account move lines and calculate manually on SpreadSheet)
> We see results with OLD and with NEW code.
>
> What shoul be the correct YAML to test it!

Ideally OpenERP should already have such test scenarios, but
unfortunately this is not yet the case, so it makes it harder to review
and validate your patch.

A typical scenario for testing your patch would be to inject some sample
data in a separate demo chart of accounts, making sure to cover as much
complicated cases as possible (different consolidation options, multiple
levels of accounts, etc.) and compute the various credit, debit and
balance results for all of them, comparing them against the correct values.

This does not necessarily need to contain huge volumes of data, because
we're not concerned about performance (that is secondary and not the
goal of the scenario), but the correctness of the calculation.

You should probably create a separate chart of account for the purpose
of this test to make sure your results are not impacted by other changes
to demo data.
Actually, it would even be acceptable to create a new demo XML or CSV
file for the definition of that separate chart of accounts and the
related journal entries, which could then be used as the basis for your
tests. In that case your tests would be very simple: just assert the
result of the functions are correct based on the demo data. The tests
only need to pass after a fresh installation of a new DB with demo data.

This may sound like a lot of work but if you have existing test data you
can perhaps grab a small sample of it, anonymize it and use it as demo
data for the tests? You don't need a lot of data, just enough data to be
representative of the most complicated cases of aggregation (again, we
don't care about the volume).

The good thing about this is that these tests would also become core
accounting tests and used to better validate the core OpenERP Accounting
in the future, in addition to confirming the correctness of the new
algorithm.
Of course the tests should first pass without your patch, and then
continue passing after your patch is applied, confirming that it did not
affect the result, while still improving a lot the performance!

This is critical because if your algorithm works it should certainly be
applied in other places, and we will need more similar regression tests
to give enough confidence to perform these refactorings.

Thanks a lot for your contributions, your patience and your eagerness to
improve OpenERP!