From 9cb3e0d58bd61ee8bed8e80f264a03b7189f0c0e Mon Sep 17 00:00:00 2001 From: Xavier-Do Date: Wed, 29 Apr 2026 12:38:57 +0200 Subject: [PATCH 1/2] [IMP] runbot: improve test-tags display and edition On an error, tests tags are joined by a comma and displayed in a single line. This makes it hard to read and edit when there are many tags, especially if they have parameters. This commit changes the separator to a newline, making it easier to read and edit the tags. In the future, it may also make the parsing more robust as we can easily split on \n, but the validation is still a good idea. This commit also adds a check on the format of the test tags to prevent saving invalid tag and hopefully complelty preventing saving tags that would break all builds. While on it, fixing the impacted error domain for parametric tags since right now they would match all errors ignoring the parameters making this feature pretty much unusable. --- runbot/common.py | 38 +++++++++++++++++--- runbot/models/build_config.py | 42 ++++++++++++++-------- runbot/models/build_error.py | 57 ++++++++++++++++++++---------- runbot/tests/test_build_error.py | 41 ++++++++++++++++++--- runbot/views/build_error_views.xml | 2 +- 5 files changed, 136 insertions(+), 44 deletions(-) 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..711a88667 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): @@ -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') 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 @@ - +
From 883add454af9334b036efc9da997dcace507c826 Mon Sep 17 00:00:00 2001 From: Xavier-Do Date: Mon, 4 May 2026 10:00:26 +0200 Subject: [PATCH 2/2] [FIX] runbot: better support newid --- runbot/models/build_error.py | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/runbot/models/build_error.py b/runbot/models/build_error.py index 711a88667..00bd5e9b7 100644 --- a/runbot/models/build_error.py +++ b/runbot/models/build_error.py @@ -429,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) @@ -443,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) @@ -457,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) @@ -471,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) @@ -1010,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), )