Merge lp:~anybox/ocb-server/7.0-test-report into lp:ocb-server

[Georges Racinet]

The core of OpenERP already has a system to report test failures (data loading, YML tests, true unit tests).

This non too-intrusive change enhances it to record more detail, allowing the launcher process to end with a useful summary.

There is a matching branch of openerp-command/7.0 that simply prints the data at the end of the test of all modules. It is I believe already enough to help maintainers quickly get an idea of the errors and dispatch them accordingly.

Here's a sample output for the current broken tests in OCB:

FAIL : 2 failure(s) or error(s) have been recorded
Module account_payment, in test file u'test/payment_order_process.yml': AssertionError in Python code : Due date is not correct.
Module purchase_requisition: Exception during load of legacy data-based tests (yml...)

See http://buildbot.anybox.fr/builders/ocb-7.0-postgresql-9.3/builds/273/steps/testing/logs/stdio for the full log.

[Stefan Rijnhart (Opener) (stefan-opener)]

Hi Georges,

thanks, this is a great help in identifying failed tests! I'd love to have this available. However, the OCB-policy dictates that this patch would also be proposed to upstream OpenERP. Do you have such a proposal ready? To be able to quickly identify the status of this change in all projects, you should make a wishlist bug report that you link to both branches.

[Georges Racinet (gracinet)]

Hi Stefan,

thanks for kindly explaining policy ! I started indeed developing the change in OCB context (where it is useful now).

[Georges Racinet (gracinet)]

but I believe even in runbot, that could prove useful (don't know what's in current trunk, though)

[Laurent Mignon (Acsone) (lmi)]

Hi, Georges,

A useful proposal that we already use in all our buildbot slaves with success here at Acsone (thanks to buildout merge-in). I've also successfully applied the proposal on the official Openerp's trunk thanks to the merge-in functionality of buildout with:
merges = bzr lp:~anybox/ocb-server/7.0-test-report parts/openerp 5252..5253
Such a enhancement is now a requirement in our continuous integration methodology.

Thanks for the contrib.
lmi

[Leonardo Pistone (lepistone)]

Thanks for your work Georges.

On a technical side, it looks good! a bit hacky the way you find the path opening the __init__.py, but I guess you have to do that (and you're using the openerp tools). Perhaps it would be a bit cleaner to import the module you want and then take module.__file__, but that maybe doesn't even work.

On a functional side, I agree that is an improvement! I have something else to mumble that has nothing to do with your work: it doesn't feel very good to work to improve a custom bit of openerp that does a generic thing (in that case, a test runner).

In any case, lgtm!

review: Approve (code review)

[Raphaël Valyi - http://www.akretion.com (rvalyi)]

Hello Georges,

this is an improvement rather than a fix. This is usually avoided on OCB (I encourage to explore ways of diminishing the cost of tracking branches and cherry picking). That being said, it's still a minor change with little side effects to expect. I would approve if you replay a MP to the official server as requested by Stefan and the OCB policy. Thanks for jumping into OCB, I think people should clearly understand the value of OCB.

[Holger Brunn (Therp) (hbrunn)]

#134 should be os.path.sep

review: Needs Fixing

[Georges Racinet (gracinet)]

Hi Holger, thanks for the review.

Yes you're right: I thought that tools.file_open did some normalization on '/' (similar to an URL) to do its magic lookup in the wanted addons directory, but after re-reading its code, I see it does not, and indeed expects the incoming ``name`` argument to be built with os.sep, as in:

    if name.replace(os.sep, '/').startswith('addons/'):
        subdir = 'addons'
        name2 = name[7:]

That's probably because it also accepts absolute paths.

btw, I don't know of any platform where os.sep is more than 1 char, but the above extract would be wrong on it.

[lp:~anybox/ocb-server/7.0-test-report]

5254. By Georges Racinet on 2014-03-04

[FIX] platform independency by using openerp.module.module API

[Georges Racinet (gracinet)]

Just pushed a new revision that addresses Holger's concern and Leonardo's remark by requesting the module's base path from openerp.module.module API.

misc.file_open does not proceed itself this way, but in case of discrepancy, I guess OpenERP would be in far more trouble than getting a yml test file path wrong in a test report... I checked quickly that it's consistent, though.

[Georges Racinet (gracinet)]

For the record, the branch against mainline openobject-addons has been updated, too

[Georges Racinet (gracinet)]

Leonardo: actually what --tests-enable (or oe initialize --tests) do is not so generic, since it does interleaving of modules initialisation and tests, so that each module's test suite is run with exactly the same demo data set no matter what dependent modules could later add.
Of course that's a consequence of the whole design of these tests. Other frameworks I used to work with solved the same kind of problems in other ways.

Rvalyi: thanks for the welcome ! You must have missed it, but right after Stefan's comment, I complied and created a bug, then linked the two branches to it.

[Raphaël Valyi - http://www.akretion.com (rvalyi)]

Thanks!

review: Approve

[Leonardo Pistone (lepistone)]

Thanks for your explanation Georges.

[Holger Brunn (Therp) (hbrunn)]

review: Approve (code review)

=== modified file 'openerp/modules/loading.py'
--- openerp/modules/loading.py	2013-11-25 10:38:42 +0000
+++ openerp/modules/loading.py	2014-03-04 14:07:44 +0000
@@ -197,13 +197,21 @@
                 # 'data' section, but should probably not alter the data,
                 # as there is no rollback.
                 if tools.config.options['test_enable']:
-                    report.record_result(load_test(module_name, idref, mode))
-
+                    report.record_result(load_test(module_name, idref, mode),
+                                         details=(dict(module=module_name,
+                                                       msg="Exception during load of legacy "
+                                                       "data-based tests (yml...)")))
                     # Run the `fast_suite` and `checks` tests given by the module.
                     if module_name == 'base':
                         # Also run the core tests after the database is created.
-                        report.record_result(openerp.modules.module.run_unit_tests('openerp'))
-                    report.record_result(openerp.modules.module.run_unit_tests(module_name))
+                        report.record_result(openerp.modules.module.run_unit_tests('openerp'),
+                                             details=dict(module='openerp',
+                                                          msg="Failure or error in server core "
+                                                          "unit tests"))
+                    report.record_result(openerp.modules.module.run_unit_tests(module_name),
+                                         details=dict(module=module_name,
+                                                      msg="Failure or error in unit tests, "
+                                                      "check logs for more details"))
 
             processed_modules.append(package.name)
 

=== modified file 'openerp/tools/assertion_report.py'
--- openerp/tools/assertion_report.py	2012-03-02 11:02:27 +0000
+++ openerp/tools/assertion_report.py	2014-03-04 14:07:44 +0000
@@ -8,20 +8,28 @@
     def __init__(self):
         self.successes = 0
         self.failures = 0
+        self.failures_details = []
 
     def record_success(self):
         self.successes += 1
 
-    def record_failure(self):
+    def record_failure(self, details=None):
         self.failures += 1
-
-    def record_result(self, result):
+        if details is not None:
+            self.failures_details.append(details)
+
+    def record_result(self, result, details=None):
+        """Record either success or failure, with the provided details in the latter case.
+
+        :param result: a boolean
+        :param details: a dict with keys ``'module'``, ``'testfile'``, ``'msg'``, ``'msg_args'``
+        """
         if result is None:
             pass
         elif result is True:
             self.record_success()
         elif result is False:
-            self.record_failure()
+            self.record_failure(details=details)
 
     def __str__(self):
         res = 'Assertions report: %s successes, %s failures' % (self.successes, self.failures)

=== modified file 'openerp/tools/convert.py'
--- openerp/tools/convert.py	2013-12-03 09:24:33 +0000
+++ openerp/tools/convert.py	2014-03-04 14:07:44 +0000
@@ -692,13 +692,15 @@
             if rec_src_count:
                 count = int(rec_src_count)
                 if len(ids) != count:
-                    self.assertion_report.record_failure()
                     msg = 'assertion "%s" failed!\n'    \
                           ' Incorrect search count:\n'  \
                           ' expected count: %d\n'       \
-                          ' obtained count: %d\n'       \
-                          % (rec_string, count, len(ids))
-                    _logger.error(msg)
+                          ' obtained count: %d\n'
+                    msg_args = (rec_string, count, len(ids))
+                    _logger.error(msg, msg_args)
+                    self.assertion_report.record_failure(details=dict(module=self.module,
+                                                                      msg=msg,
+                                                                      msg_args=msg_args))
                     return
 
         assert ids is not None,\
@@ -720,13 +722,15 @@
                 expected_value = _eval_xml(self, test, self.pool, cr, uid, self.idref, context=context) or True
                 expression_value = unsafe_eval(f_expr, globals_dict)
                 if expression_value != expected_value: # assertion failed
-                    self.assertion_report.record_failure()
                     msg = 'assertion "%s" failed!\n'    \
                           ' xmltag: %s\n'               \
                           ' expected value: %r\n'       \
-                          ' obtained value: %r\n'       \
-                          % (rec_string, etree.tostring(test), expected_value, expression_value)
-                    _logger.error(msg)
+                          ' obtained value: %r\n'
+                    msg_args = (rec_string, etree.tostring(test), expected_value, expression_value)
+                    self.assertion_report.record_failure(details=dict(module=self.module,
+                                                                      msg=msg,
+                                                                      msg_args=msg_args))
+                    _logger.error(msg, msg_args)
                     return
         else: # all tests were successful for this assertion tag (no break)
             self.assertion_report.record_success()

=== modified file 'openerp/tools/yaml_import.py'
--- openerp/tools/yaml_import.py	2012-12-01 11:35:24 +0000
+++ openerp/tools/yaml_import.py	2014-03-04 14:07:44 +0000
@@ -1,10 +1,12 @@
 # -*- coding: utf-8 -*-
+import os
 import threading
 import types
 import time # used to eval time.strftime expressions
 from datetime import datetime, timedelta
 import logging
 
+from copy import deepcopy
 import openerp.pooler as pooler
 import openerp.sql_db as sql_db
 import misc
@@ -193,7 +195,13 @@
         return node
 
     def _log_assert_failure(self, msg, *args):
-        self.assertion_report.record_failure()
+        from openerp.modules import module  # cannot be made before (loop)
+        basepath = module.get_module_path(self.module)
+        self.assertion_report.record_failure(
+            details=dict(module=self.module,
+                         testfile=os.path.relpath(self.filename, basepath),
+                         msg=msg,
+                         msg_args=deepcopy(args)))
         _logger.error(msg, *args)
 
     def _get_assertion_id(self, assertion):