diff --git a/runbot/common.py b/runbot/common.py index 23938c84e..fe34c2d44 100644 --- a/runbot/common.py +++ b/runbot/common.py @@ -329,9 +329,35 @@ class TestTagsParser: (?:\[(.*)\])? # parameters $''', re.VERBOSE) # [-][tag][/module][:class][.method][[params]] - def __init__(self, test_tags): - parts = re.split(r',(?![^\[]*\])', test_tags) # split on all comma not inside [] (not followed by ]) + def __init__(self, test_tags, keep_escape=True): + parts = [''] + bracket_level = 0 + escape_next = False + for char in test_tags: + if char == ',' and bracket_level == 0: + parts.append('') + continue + + if char == '\\': + if not escape_next: + escape_next = True + if keep_escape: + parts[-1] += '\\' # not as the TagsSelector, we keep the escape character + continue + elif char == '[': + if not escape_next: + bracket_level += 1 + elif char == ']': + if not escape_next: + bracket_level -= 1 + elif not keep_escape and escape_next: # the previous \ was not escaping anything, put it back + parts[-1] += '\\' + + escape_next = False + parts[-1] += char + filter_specs = [t.strip() for t in parts if t.strip()] + self.filter_specs = filter_specs self.exclude = set() self.include = set() self.parameters = OrderedSet() @@ -339,8 +365,7 @@ def __init__(self, test_tags): for filter_spec in filter_specs: match = self.filter_spec_re.match(filter_spec) if not match: - _logger.error('Invalid tag %s', filter_spec) - continue + raise ValueError('Invalid tag %s' % filter_spec) sign, tag, file_path, module, klass, method, parameters = match.groups() is_include = sign != '-' @@ -369,6 +394,7 @@ def __init__(self, test_tags): def test_tags_to_search_domain(self, exclude_error_id=None): search_domains = [] + params_by_spec = dict(self.parameters) for include in self.include: _, test_module, test_class, test_method, file_path = include module_path = file_path or ((test_module or '') + '%') @@ -376,6 +402,10 @@ def test_tags_to_search_domain(self, exclude_error_id=None): test_method = test_method or '%' search_pattern = f'{module_path}:{test_class}.{test_method}' tag_domain = [('canonical_tags', 'like', f'{search_pattern}')] + params = params_by_spec.get(include) + if params: + _sign, parameters = params + tag_domain.append(('canonical_tags', 'like', f'%[{parameters}%]%')) if exclude_error_id: tag_domain.append(('id', '!=', exclude_error_id)) search_domains.append(tag_domain) diff --git a/runbot/models/build_config.py b/runbot/models/build_config.py index 9c3a5ccee..ac34d1ae0 100644 --- a/runbot/models/build_config.py +++ b/runbot/models/build_config.py @@ -1,35 +1,46 @@ import base64 +import fnmatch import glob import json import logging -import fnmatch -import psutil import re import shlex import time -from unidiff import PatchSet -from ..common import now, grep, time2str, rfind, s2human, os, RunbotException, ReProxy, markdown_escape -from ..container import docker_get_gateway_ip, Command -from odoo import models, fields, api, tools -from odoo.exceptions import UserError, ValidationError -from odoo.tools.misc import file_open -from odoo.tools.safe_eval import safe_eval, test_python_expr, _SAFE_OPCODES, to_opcodes -# adding some additionnal optcode to safe_eval. This is not 100% needed and won't be done in standard but will help -# to simplify some python step by wraping the content in a function to allow return statement and get closer to other -# steps +import psutil +from unidiff import VERSION, PatchSet, patch +from odoo import api, fields, models, tools +from odoo.exceptions import UserError, ValidationError +from odoo.tools.misc import file_open +from odoo.tools.safe_eval import _SAFE_OPCODES, safe_eval, test_python_expr, to_opcodes + +from ..common import ( + ReProxy, + RunbotException, + TestTagsParser, + grep, + markdown_escape, + now, + os, + rfind, + s2human, + time2str, +) +from ..container import Command, docker_get_gateway_ip # There is an issue in unidiff 0.7.3 fixed in 0.7.4 # https://github.com/matiasb/python-unidiff/commit/a3faffc54e5aacaee3ded4565c534482d5cc3465 # Since the unidiff packaged version in noble is 0.7.3 # patching it looks like the easiest solution -from unidiff import patch, VERSION if VERSION == '0.7.3': patch.RE_DIFF_GIT_DELETED_FILE = re.compile(r'^deleted file mode \d+$') patch.RE_DIFF_GIT_NEW_FILE = re.compile(r'^new file mode \d+$') +# adding some additionnal optcode to safe_eval. This is not 100% needed and won't be done in standard but will help +# to simplify some python step by wraping the content in a function to allow return statement and get closer to other +# steps _SAFE_OPCODES |= set(to_opcodes(['LOAD_DEREF', 'STORE_DEREF', 'LOAD_CLOSURE', 'MAKE_CELL', 'COPY_FREE_VARS'])) @@ -426,7 +437,7 @@ class ConfigStep(models.Model): paths_to_omit = fields.Char('Paths to omit from coverage', tracking=True) flamegraph = fields.Boolean('Allow Flamegraph', default=False, tracking=True) test_enable = fields.Boolean('Test enable', default=True, tracking=True) - test_tags = fields.Char('Test tags', help="comma separated list of test tags", tracking=True) + test_tags = fields.Char('Test tags', help="new line (or comma) separated list of test tags", tracking=True) enable_auto_tags = fields.Boolean('Allow auto tag', default=True, tracking=True) sub_command = fields.Char('Subcommand', tracking=True) extra_params = fields.Char('Extra cmd args', tracking=True) @@ -637,6 +648,7 @@ def _make_python_ctx(self, build): 'json_loads': json.loads, 'PatchSet': PatchSet, 'markdown_escape': markdown_escape, + 'TestTagsParser': TestTagsParser, } def _run_python(self, build, force=False): @@ -774,7 +786,7 @@ def _run_install_odoo(self, build, config_data=None): test_tags_in_extra = '--test-tags' in extra_params if (test_enable or test_tags) and "--test-tags" in available_options and not test_tags_in_extra: - test_tags = [t.strip() for t in (test_tags or '').split(',')] + test_tags = [t.strip() for t in TestTagsParser(test_tags or '').filter_specs] if enable_auto_tags and not config_data.get('disable_auto_tags', False): if grep(config_path, "[/module][:class]"): auto_tags = self.env['runbot.build.error']._disabling_tags(build) diff --git a/runbot/models/build_error.py b/runbot/models/build_error.py index 87197ccfc..00bd5e9b7 100644 --- a/runbot/models/build_error.py +++ b/runbot/models/build_error.py @@ -9,16 +9,15 @@ from dateutil import rrule from dateutil.relativedelta import relativedelta from markupsafe import Markup - from werkzeug.urls import url_join from odoo import api, fields, models from odoo.exceptions import AccessError, UserError, ValidationError -from odoo.tools import SQL, lazy, ormcache from odoo.fields import Domain +from odoo.tools import SQL, lazy, ormcache +from ..common import TestTagsParser, transactioncache from ..fields import JsonDictField -from ..common import transactioncache, TestTagsParser _logger = logging.getLogger(__name__) @@ -242,8 +241,8 @@ class BuildError(models.Model): breaking_bundle_url = fields.Char('Breaking bundle url', related='breaking_bundle_id.frontend_url') breaking_pr_date = fields.Datetime('Breaking date', related="breaking_pr_id.close_date", help="Date of the merge of the first pr") - test_tags = fields.Char(string='Test tags', help="Comma separated list of test_tags to use to reproduce/remove this error", tracking=True) - canonical_tags = fields.Char('Canonical tag', compute='_compute_canonical_tags', store=True) + test_tags = fields.Text(string='Test tags', help="Comma separated list of test_tags to use to reproduce/remove this error", tracking=True) + canonical_tags = fields.Text('Canonical tag', compute='_compute_canonical_tags', store=True) tags_match_count = fields.Integer('Nb errors matching the test_tags', compute='_compute_tags_match_count') tags_min_version_excluded_id = fields.Many2one('runbot.version', 'Tag min version (excluded)') tags_min_version_id = fields.Many2one('runbot.version', 'Tags Min version', compute="_compute_tags_min_version_id", inverse="_inverse_tags_min_version_id", help="Minimal version where the test tags will be applied.", tracking=True) @@ -288,7 +287,7 @@ def _inverse_tags_min_version_id(self): def _compute_canonical_tags(self): for record in self: canonical_tags = sorted(set(record.error_content_ids.filtered('canonical_tag').mapped('canonical_tag'))) - record.canonical_tags = ','.join(canonical_tags) + record.canonical_tags = '\n'.join(canonical_tags) @api.depends('tags_min_version_id') def _compute_tags_min_version_id(self): @@ -430,10 +429,10 @@ def _compute_unique_qualifiers(self): @api.depends('common_qualifiers') def _compute_similar_ids(self): for record in self: - if record.common_qualifiers: + if record.common_qualifiers and (record.id or record.id.origin): query = SQL( r"""SELECT id FROM runbot_build_error WHERE id != %s AND common_qualifiers @> %s""", - record.id, + record.id or record.id.origin, json.dumps(record.common_qualifiers.dict), ) self.env.cr.execute(query) @@ -444,10 +443,10 @@ def _compute_similar_ids(self): @api.depends('common_qualifiers') def _compute_similar_content_ids(self): for record in self: - if record.common_qualifiers: + if record.common_qualifiers and (record.id or record.id.origin): query = SQL( r"""SELECT id FROM runbot_build_error_content WHERE error_id != %s AND qualifiers @> %s""", - record.id, + record.id or record.id.origin, json.dumps(record.common_qualifiers.dict), ) self.env.cr.execute(query) @@ -458,10 +457,10 @@ def _compute_similar_content_ids(self): @api.depends('common_qualifiers') def _compute_analogous_ids(self): for record in self: - if record.common_qualifiers: + if record.common_qualifiers and (record.id or record.id.origin): query = SQL( r"""SELECT id FROM runbot_build_error WHERE id != %s AND unique_qualifiers @> %s""", - record.id, + record.id or record.id.origin, json.dumps(record.unique_qualifiers.dict), ) self.env.cr.execute(query) @@ -472,10 +471,10 @@ def _compute_analogous_ids(self): @api.depends('common_qualifiers') def _compute_analogous_content_ids(self): for record in self: - if record.common_qualifiers: + if record.common_qualifiers and (record.id or record.id.origin): query = SQL( r"""SELECT id FROM runbot_build_error_content WHERE error_id != %s AND qualifiers @> %s""", - record.id, + record.id or record.id.origin, json.dumps(record.unique_qualifiers.dict), ) self.env.cr.execute(query) @@ -488,16 +487,20 @@ def _compute_tags_match_count(self): for record in self: record.tags_match_count = 0 if record.test_tags: - tags_parser = TestTagsParser(record.test_tags) - search_domain = tags_parser.test_tags_to_search_domain(exclude_error_id=record.id) - if search_domain: - record.tags_match_count = self.env['runbot.build.error'].with_context(active_test=True).search_count(search_domain) + try: + tags_parser = TestTagsParser(record.test_tags.replace('\n', ',')) + search_domain = tags_parser.test_tags_to_search_domain(exclude_error_id=record.id or record.id.origin) + if search_domain: + record.tags_match_count = self.env['runbot.build.error'].with_context(active_test=True).search_count(search_domain) + except Exception as e: # noqa: BLE001 + record.tags_match_count = -1 + _logger.warning("Error while computing tags_match_count for error %s with test_tags %s: %s", record.id, record.test_tags, e) def action_view_impacted_by_tag(self): self.ensure_one() if not self.test_tags: return - tags_parser = TestTagsParser(self.test_tags) + tags_parser = TestTagsParser(self.test_tags.replace('\n', ',')) return { 'type': 'ir.actions.act_window', 'views': [(False, 'list'), (False, 'form')], @@ -510,8 +513,15 @@ def action_view_impacted_by_tag(self): @api.constrains('test_tags') def _check_test_tags(self): for build_error in self: - if build_error.test_tags and '-' in build_error.test_tags: - raise ValidationError('Build error test_tags should not be negated') + if build_error.test_tags: + try: + test_tags = build_error.test_tags.replace('\n', ',') + tags_parser = TestTagsParser(test_tags) + tags_parser = TestTagsParser(test_tags, keep_escape=False) + except Exception as e: # noqa: BLE001 + raise ValidationError(f'Invalid test_tags format: {e}') + if tags_parser.exclude or any(params[0] == '-' for p, params in tags_parser.parameters): + raise ValidationError('Build error test_tags should not be negated') @api.onchange('test_tags') def _onchange_test_tags(self): @@ -586,12 +596,12 @@ def _merge(self, others): error.sudo().test_tags = previous_error.test_tags previous_error.sudo().test_tags = False elif self.env.su: - test_tags = error.test_tags.split(',') - previous_error - for tag in previous_error.test_tags.split(','): + test_tags = TestTagsParser(error.test_tags.replace('\n', ',')).filter_specs + previous_error_tags = TestTagsParser(previous_error.test_tags.replace('\n', ',')).filter_specs + for tag in previous_error_tags: if tag not in test_tags: test_tags.append(tag) - error.test_tags = ','.join(test_tags) + error.test_tags = '\n'.join(test_tags) previous_error.test_tags = False for field in fields_to_merge + fields_to_copy: if previous_error[field]: @@ -622,7 +632,16 @@ def filter_tags(e): return True test_tag_list = self.search([('test_tags', '!=', False)]).filtered(filter_tags).mapped('test_tags') - return [test_tag for error_tags in test_tag_list for test_tag in (error_tags).split(',')] + parsed_test_tags = [] + for error_tags in test_tag_list: + try: + # we cannot rely only on '\n' since old test-tags or user defined ones could be comma separated + error_tags = error_tags.replace('\n', ',') + tags_parser = TestTagsParser(error_tags) + parsed_test_tags.extend(tags_parser.filter_specs) + except Exception as e: # noqa: BLE001 + _logger.warning('Error while parsing test_tags for error with id %s: %s', self.id, e) + return parsed_test_tags @api.model def _disabling_tags(self, build_id=False): @@ -853,7 +872,7 @@ class BuildErrorContent(models.Model): breaking_pr_id = fields.Many2one(related='error_id.breaking_pr_id') fixing_pr_alive = fields.Boolean(related='error_id.fixing_pr_alive') fixing_pr_url = fields.Char(related='error_id.fixing_pr_url') - test_tags = fields.Char(related='error_id.test_tags') + test_tags = fields.Text(related='error_id.test_tags') tags_min_version_id = fields.Many2one(related='error_id.tags_min_version_id') tags_max_version_id = fields.Many2one(related='error_id.tags_max_version_id') @@ -991,10 +1010,10 @@ def _compute_error_display_id(self): def _compute_similar_ids(self): """error contents having the exactly the same qualifiers""" for record in self: - if record.qualifiers: + if record.qualifiers and (record.id or record.id.origin): query = SQL( r"""SELECT id FROM runbot_build_error_content WHERE id != %s AND qualifiers @> %s AND qualifiers <@ %s""", - record.id, + record.id or record.id.origin, json.dumps(record.qualifiers.dict), json.dumps(record.qualifiers.dict), ) diff --git a/runbot/tests/test_build_error.py b/runbot/tests/test_build_error.py index 24e9ab6a2..3525871cb 100644 --- a/runbot/tests/test_build_error.py +++ b/runbot/tests/test_build_error.py @@ -266,7 +266,7 @@ def test_relink_simple(self): build_b = self.create_test_build({'local_result': 'ko', 'local_state': 'done'}) error_content_b = self.BuildErrorContent.create({'content': 'foo bar'}) error_b = error_content_b.error_id - error_b.test_tags = 'footag' + error_b.test_tags = 'footag[@test, comma]\nfootag2[@test, comma],footag3[@test, comma]' self.BuildErrorLink.create({'build_id': build_b.id, 'error_content_id': error_content_b.id}) self.assertEqual(self.BuildErrorContent.search([('fingerprint', '=', error_content_a.fingerprint)]), error_content_a | error_content_b) @@ -275,7 +275,7 @@ def test_relink_simple(self): self.assertFalse(error_b.error_content_ids) self.assertTrue(error_a.active, 'The merged error without test tags should have been deactivated') - self.assertEqual(error_a.test_tags, 'footag', 'Tags should have been transfered from b to a') + self.assertEqual(error_a.test_tags, 'footag[@test, comma]\nfootag2[@test, comma],footag3[@test, comma]', 'Tags should have been transfered from b to a') self.assertFalse(error_b.active, 'The merged error with test tags should remain active') self.assertIn(build_a, error_content_a.build_ids) self.assertIn(build_b, error_content_a.build_ids) @@ -284,9 +284,9 @@ def test_relink_simple(self): tagged_error_content = self.BuildErrorContent.create({'content': 'foo bar'}) tagged_error = tagged_error_content.error_id - tagged_error.test_tags = 'bartag' + tagged_error.test_tags = 'bartag[@test, comma]\nbartag2[@test, comma],bartag3[@test, comma]' (error_content_a | tagged_error_content)._relink() - self.assertEqual(error_a.test_tags, 'footag,bartag') + self.assertEqual(error_a.test_tags, 'footag[@test, comma]\nfootag2[@test, comma]\nfootag3[@test, comma]\nbartag[@test, comma]\nbartag2[@test, comma]\nbartag3[@test, comma]') self.assertTrue(error_a.active) self.assertFalse(tagged_error.active) @@ -869,7 +869,38 @@ def test_error_content_multiple_canonical_tags(self): self.assertEqual(error_content_2.canonical_tag, '/web/tests/test_file.py:TestUi.TestUi') self.assertNotEqual(error_content_1, error_content_2) self.assertEqual(error_content_1.error_id, error_content_2.error_id) - self.assertEqual(error_content_1.error_id.canonical_tags, '/base/tests/test_file.py:TestUi.TestUi,/web/tests/test_file.py:TestUi.TestUi') + self.assertEqual(error_content_1.error_id.canonical_tags, '/base/tests/test_file.py:TestUi.TestUi\n/web/tests/test_file.py:TestUi.TestUi') + error_content_1.error_id.test_tags = error_content_1.error_id.canonical_tags + self.assertEqual(error_content_1.error_id._disabling_tags(), [ + '-/base/tests/test_file.py:TestUi.TestUi', + '-/web/tests/test_file.py:TestUi.TestUi', + ]) + error_content_3 = self.env['runbot.build.error.content'].create({ + 'content': 'Tour foobar_tour failed at step step_14 in mode mode', + 'metadata': {'test': {'canonical_tag': '/web/tests/test_file.py:TestJs.test_unit_desktop[@web/some , comma\\[\\] and brackets]'}}, + }) + self.assertEqual(error_content_3.canonical_tag, '/web/tests/test_file.py:TestJs.test_unit_desktop[@web/some , comma\\[\\] and brackets]') + self.assertNotEqual(error_content_1, error_content_2) + self.assertEqual(error_content_1.error_id, error_content_2.error_id) + self.assertEqual(error_content_1.error_id.canonical_tags, """/base/tests/test_file.py:TestUi.TestUi\n/web/tests/test_file.py:TestJs.test_unit_desktop[@web/some , comma\\[\\] and brackets]\n/web/tests/test_file.py:TestUi.TestUi""") + error_content_1.error_id.test_tags = error_content_1.error_id.canonical_tags + self.assertEqual(error_content_1.error_id._disabling_tags(), [ + '-/base/tests/test_file.py:TestUi.TestUi', + '-/web/tests/test_file.py:TestJs.test_unit_desktop[@web/some , comma\\[\\] and brackets]', + '-/web/tests/test_file.py:TestUi.TestUi', + ], "disabling tags must keep the escaping and correct format") + error_content_1.error_id.test_tags = error_content_1.error_id.test_tags.replace('\n', ',') + self.assertEqual(error_content_1.error_id._disabling_tags(), [ + '-/base/tests/test_file.py:TestUi.TestUi', + '-/web/tests/test_file.py:TestJs.test_unit_desktop[@web/some , comma\\[\\] and brackets]', + '-/web/tests/test_file.py:TestUi.TestUi', + ], "_disabling_tags shoudl also work fine with comma separted tags") + error_content_1.error_id.test_tags = error_content_1.error_id.test_tags.replace('\\', '') + self.assertEqual(error_content_1.error_id._disabling_tags(), [ + '-/base/tests/test_file.py:TestUi.TestUi', + '-/web/tests/test_file.py:TestJs.test_unit_desktop[@web/some , comma[] and brackets]', + '-/web/tests/test_file.py:TestUi.TestUi', + ]) class TestCodeOwner(RunbotCase): diff --git a/runbot/views/build_error_views.xml b/runbot/views/build_error_views.xml index cee4d2d8e..6eec4d12e 100644 --- a/runbot/views/build_error_views.xml +++ b/runbot/views/build_error_views.xml @@ -66,7 +66,7 @@ - +