A Testing Lesson

As part of the big migration work I'm doing, a big chunk is validating and migrating the code to v11, adding and modifying tests, adding demo data that kind of thing.  I was adding tests to a reasonably large custom module and was 80% through when I hit a point of "How am I going to test this?'.  The module in question is reasonably straightforward, it handles the creation, management and application of customer and opportunity specific special pricing based off purchase requisitions when creating MTO style Purchase Orders.  The method itself was also reasonably innocuous, like hundreds you will see in Odoo core code and community modules every day.  A straight forward call of the parent function, and update its results with the best price applicable to that combination of customer, opportunity and vendor.


# Snippet is licensed AGPL3.0 / snipped for brevity

class ProcurementRule(models.Model):
_inherit = 'procurement.rule'

@api.multi
def _prepare_purchase_order_line(self, product_id, product_qty,
product_uom, values, po, supplier):
line_vals = super(ProcurementRule, self)._prepare_purchase_order_line(
product_id, product_qty, product_uom, values, po, supplier)
matching_lines = self.env['purchase.requisition.line']
if po.requisition_id:
matching_lines |= po.requisition_id.line_ids.filtered(
lambda l: l.product_id.product_tmpl_id.id ==
product_id.product_tmpl_id.id
)

Requisition = self.env['purchase.requisition']
search_args = [<a long list of args including po.date_order and the supplier>]
if values.get('group_id') and values['group_id'].sale_id:
sale = values['group_id'].sale_id
search_args.extend([<additional args based on the customer>]

matching_lines |= Requisition.search(search_args).mapped(
'line_ids').filtered(
lambda l: l.product_id.product_tmpl_id.id ==
product_id.product_tmpl_id.id
)
line_vals['price_unit'] = min(
matching_lines.mapped('price_unit') + [line_vals['price_unit']])
return line_vals

A quick look reveals a few things that would be required to test this function.  First we would need to fill all the parameters to the function.  So as a minimum we need to setup a purchase order, a sales order, a product, a supplier info record and then a values dict which we know at least requires a key "group_id" which has an attribute sale_id, let alone what the super call might need.  In a test that is a lot of code and we'll need more than just 1 variety.  At this point its looking more like an integration test than a unit test.  I need to worry that the super call returns correctly or else I need to mock that out.  In fact it is looking easier to just do an integration test, where the function is called as part of that.

The things is, all that is really important here is that the right price is returned based on the inputs.  An analysis of the logic reveals that we don't need the po, just a requisition_id and a date.  Same for for the supplierinfo record, we only need a partner.  The value dict, only its associated sales order, and from the parent call only its price.  In fact we don't even care about the sales order, just the customer (and later its opportunity).  With just that information we can determine the price to apply. 

So why go to the effort of setting all the rest up just to call the parent function.  We create a new function called _get_special_pricing that takes those parameters and amend our function to call it with those params.


# Snippet is licensed AGPL3.0

class ProcurementRule(models.Model):
_inherit = "procurement.rule"

@api.multi
def _prepare_purchase_order_line(
self, product_id, product_qty, product_uom, values, po, supplier
):
line_vals = super()._prepare_purchase_order_line(
product_id, product_qty, product_uom, values, po, supplier
)
sale = values.get("group_id") and values["group_id"].sale_id
partner, opportunity = False, False
if sale:
partner = sale.partner_id
opportunity = sale.opportunity_id
price = line_vals["price_unit"]
line_vals["price_unit"] = self._get_special_pricing(
product_id,
po.date_order,
supplier.name,
price,
requisition=po.requisition_id,
partner=partner,
opportunity=opportunity,
)
return line_vals

def _get_special_pricing(
self,
product,
date_order,
supplier,
price,
requisition=None,
partner=None,
opportunity=None,
):

The logic largely remains the same as before.  A few variables are simplified and I extended to take opportunities into account, but now we can test the part we care about easily and quickly without being concerned about things we don't.  Every single parameter is easily passed without needing to worry about creating objects, ensuring their states.  Which makes them especially conducive to utilising demo data.  Compared to all the setup needed before we get 2 line tests like

def test_get_special_pricing3(self):
"""
Customer specific price is lowest
:return:
"""
price = self.env["procurement.rule"]._get_special_pricing(
self.product1,
self.today,
self.vendor,
18.99,
partner=self.r("example.customer_1"),
)
self.assertEquals(price, 14.0)

The end result, a comprehensive test suite, with easy to tweak parameters that runs in under 0.5s and a big lesson that the prevailing idiom of set some vals, call parent, set some more vals, return can be harmful especially when that parent requires a lot of parameters. A far better way in most circumstances is to completely separate the logic of what you are trying to do into a distinctly testable unit and leave the sequencing of operations in the parent function.


Complex Migration