From 610b1ffc6860a712646374fb8d9a8f8d596f057a Mon Sep 17 00:00:00 2001 From: Massimiliano Date: Wed, 11 Dec 2024 16:45:27 +0100 Subject: [PATCH] Report invalid units generated during test session (bugfix) (#1560) Before this commit, if a template generated a unit that was invalid, a message was logged (error for some, debug for some others) and the unit was skipped. The logged message was hard to find, and it was not immediately clear that some jobs had not been generated due to templating errors. This commit fixes this by introducing a new unit type (InvalidJob) and adding them to the test execution, so that errors in templating are surfaced in the test results as failures, and therefore easier to review and to fix. This behavior is the new default, but it is possible to use the legacy behavior (to just log the problems without creating "InvalidJobs") by defining setting the `strict_template_expansion` flag to `False` in the `features` section of a configuration file. * Implement late validation of template jobs * Added metabox test * Crash testplan * Better unit invalid exception * Full draft implementation of invalid units * Remove unused imports and lemmas * Make test positive * Don't reset the flags, but add and remove from set * Move SessionState_metadata resume before SessionState * Make InvalidJob valid enough to run the tests + use __index__ instead of UUID * Add Test for remote and local * New metabox tests provider * Check that the backward compatiblity works * Revert downgrade of warning * Fix tests for unit and ctrl * Move warning in use_feature_flags * Fully roll back downgrade to warning * Test new filter and wrap function in ctrl.py Minor: Also log warnings in wrapped units * Also test observe_results with new flag * New test for run_job * Add experimental feature tests * Verify that the infos are in the exception * Typo Co-authored-by: Pierre Equoy * Better description --------- Co-authored-by: Pierre Equoy --- checkbox-ng/plainbox/impl/config.py | 11 + checkbox-ng/plainbox/impl/ctrl.py | 150 +++++++----- checkbox-ng/plainbox/impl/execution.py | 38 ++- .../plainbox/impl/session/assistant.py | 6 +- checkbox-ng/plainbox/impl/session/resume.py | 6 +- checkbox-ng/plainbox/impl/session/state.py | 8 + .../plainbox/impl/session/test_state.py | 34 +++ checkbox-ng/plainbox/impl/test_ctrl.py | 148 ++++++++++-- checkbox-ng/plainbox/impl/test_execution.py | 42 ++++ checkbox-ng/plainbox/impl/unit/job.py | 113 +++++++-- checkbox-ng/plainbox/impl/unit/test_unit.py | 8 +- checkbox-ng/plainbox/impl/unit/unit.py | 22 +- .../metabox-provider/units/basic-jobs.pxu | 41 ++++ .../metabox-provider/units/basic-tps.pxu | 9 + .../metabox/metabox-provider/units/resume.pxu | 10 + .../scenarios/basic/template_expansion.py | 219 ++++++++++++++++++ 16 files changed, 749 insertions(+), 116 deletions(-) create mode 100644 checkbox-ng/plainbox/impl/test_execution.py create mode 100644 metabox/metabox/scenarios/basic/template_expansion.py diff --git a/checkbox-ng/plainbox/impl/config.py b/checkbox-ng/plainbox/impl/config.py index 3cd3cf1063..08a7e756d5 100644 --- a/checkbox-ng/plainbox/impl/config.py +++ b/checkbox-ng/plainbox/impl/config.py @@ -389,6 +389,17 @@ class DynamicSection(dict): ), }, ), + ( + "features", + { + "strict_template_expansion": VarSpec( + bool, + True, + "Invalid units generated by templates will be used and" + " reported as failures", + ) + }, + ), ( "ui", { diff --git a/checkbox-ng/plainbox/impl/ctrl.py b/checkbox-ng/plainbox/impl/ctrl.py index b02dc7e7d9..414de582ad 100644 --- a/checkbox-ng/plainbox/impl/ctrl.py +++ b/checkbox-ng/plainbox/impl/ctrl.py @@ -32,10 +32,6 @@ class to select the best method to execute a command of a particular job. This circumstances. """ -import abc -import contextlib -import errno - try: import grp except ImportError: @@ -44,36 +40,25 @@ class to select the best method to execute a command of a particular job. This import json import logging import os -import tempfile -import subprocess -import sys -import threading from functools import partial -from subprocess import check_output, CalledProcessError, STDOUT -from plainbox.abc import IJobResult -from plainbox.abc import ISessionStateController +from plainbox.abc import IJobResult, ISessionStateController from plainbox.i18n import gettext as _ -from plainbox.impl import get_plainbox_dir -from plainbox.impl.depmgr import DependencyDuplicateError from plainbox.impl.depmgr import DependencyMissingError -from plainbox.impl.resource import ExpressionCannotEvaluateError -from plainbox.impl.resource import ExpressionFailedError -from plainbox.impl.resource import ResourceProgramError -from plainbox.impl.resource import Resource +from plainbox.impl.resource import ( + ExpressionCannotEvaluateError, + ExpressionFailedError, + ResourceProgramError, + Resource, +) from plainbox.impl.secure.origin import JobOutputTextSource -from plainbox.impl.secure.providers.v1 import Provider1 -from plainbox.impl.secure.rfc822 import RFC822SyntaxError -from plainbox.impl.secure.rfc822 import gen_rfc822_records -from plainbox.impl.session.jobs import InhibitionCause -from plainbox.impl.session.jobs import JobReadinessInhibitor -from plainbox.impl.unit.job import JobDefinition +from plainbox.impl.secure.rfc822 import RFC822SyntaxError, gen_rfc822_records +from plainbox.impl.session.jobs import InhibitionCause, JobReadinessInhibitor from plainbox.impl.unit.template import TemplateUnit from plainbox.impl.unit.unit import MissingParam from plainbox.impl.validation import Severity from plainbox.suspend_consts import Suspend -from plainbox.vendor import morris -from plainbox.vendor import extcmd +from plainbox.impl.unit.job import InvalidJob __all__ = [ "CheckBoxSessionStateController", @@ -421,6 +406,55 @@ def _parse_and_store_resource(self, session_state, job, result): # Replace any old resources with the new resource list session_state.set_resource_list(job.id, new_resource_list) + @staticmethod + def _filter_invalid_log(unit): + """ + Used to filter units that are generated but are invalid, will return + True if the unit has to be kept, False if it has to be discarded + """ + try: + check_result = unit.check() + except MissingParam as m: + logger.critical( + _("Ignoring %s with missing template parameter %s"), + unit._raw_data.get("id"), + m.parameter, + ) + return False + + errors = [c for c in check_result if c.severity == Severity.error] + warnings = (c for c in check_result if c.severity == Severity.warning) + for warning in warnings: + logger.warning(str(warning)) + + if not errors: + return True + + for error in errors: + logger.error(str(error)) + logger.critical("Ignoring invalid generated job %s", unit.id) + return False + + @staticmethod + def _wrap_invalid_units(unit): + """ + Used to wrap invalid units generated by the template + """ + + try: + check_result = unit.check() + errors = [c for c in check_result if c.severity == Severity.error] + warnings = ( + c for c in check_result if c.severity == Severity.warning + ) + for warning in warnings: + logger.warning(str(warning)) + except MissingParam as m: + errors = [m] + if not errors: + return unit + return InvalidJob.from_unit(unit, errors=errors) + def _instantiate_templates( self, session_state, job, result, fake_resources=False ): @@ -432,35 +466,43 @@ def _instantiate_templates( # before it was suspended, so don't if result.outcome is IJobResult.OUTCOME_NONE: return - for unit in session_state.unit_list: - if isinstance(unit, TemplateUnit) and unit.resource_id == job.id: - logger.info(_("Instantiating unit: %s"), unit) - for new_unit in unit.instantiate_all( - session_state.resource_map[job.id], fake_resources - ): - try: - check_result = new_unit.check() - except MissingParam as m: - logger.debug( - _( - "Ignoring %s with missing " - "template parameter %s" - ), - new_unit._raw_data.get("id"), - m.parameter, - ) - continue - # Only ignore jobs for which check() returns an error - if [ - c for c in check_result if c.severity == Severity.error - ]: - logger.error( - _("Ignoring invalid generated job %s"), new_unit.id - ) - else: - session_state.add_unit( - new_unit, via=job, recompute=False - ) + # get all templates that use this (resource) job as template_resource + template_units = filter( + lambda unit: isinstance(unit, TemplateUnit) + and unit.resource_id == job.id, + session_state.unit_list, + ) + # get the parsed resource (list of dict created from the resource + # stdout) + parsed_resource = session_state.resource_map[job.id] + # get a list of all new units generated from each template + # this is an array of arrays units as follows: + # [[unit_from_template1, ...], [unit_from_template2, ...]] + new_units_lists = ( + template_unit.instantiate_all(parsed_resource, fake_resources) + for template_unit in template_units + ) + # flattening list to make it easier to work with + new_units = ( + new_unit + for new_unit_list in new_units_lists + for new_unit in new_unit_list + ) + + if ( + session_state.metadata.FLAG_FEATURE_STRICT_TEMPLATE_EXPANSION + in session_state.metadata.flags + ): + + new_units = map(self._wrap_invalid_units, new_units) + else: + new_units = filter(self._filter_invalid_log, new_units) + + for new_unit in new_units: + # here they are added unconditionally as they will be checked + # before running to make error reporting possible or were already + # filtered by non-strict template expansion + session_state.add_unit(new_unit, via=job, recompute=False) session_state._recompute_job_readiness() diff --git a/checkbox-ng/plainbox/impl/execution.py b/checkbox-ng/plainbox/impl/execution.py index 1d65ed224c..38a779bce9 100644 --- a/checkbox-ng/plainbox/impl/execution.py +++ b/checkbox-ng/plainbox/impl/execution.py @@ -38,16 +38,22 @@ from plainbox.impl.color import Colorizer from plainbox.impl.unit.job import supported_plugins from plainbox.impl.unit.unit import on_ubuntucore -from plainbox.impl.result import IOLogRecordWriter -from plainbox.impl.result import JobResultBuilder -from plainbox.impl.runner import CommandOutputWriter -from plainbox.impl.runner import IOLogRecordGenerator -from plainbox.impl.runner import JobRunnerUIDelegate -from plainbox.impl.runner import slugify +from plainbox.impl.result import ( + IOLogRecordWriter, + JobResultBuilder, + IOLogRecord, +) +from plainbox.impl.runner import ( + CommandOutputWriter, + IOLogRecordGenerator, + JobRunnerUIDelegate, + slugify, +) from plainbox.impl.jobcache import ResourceJobCache from plainbox.impl.secure.sudo_broker import sudo_password_provider from plainbox.impl.session.storage import WellKnownDirsHelper from plainbox.vendor import extcmd +from plainbox.impl.unit.job import InvalidJob logger = logging.getLogger("plainbox.unified") @@ -91,6 +97,25 @@ def __init__( def run_job(self, job, job_state, environ=None, ui=None): logger.info(_("Running %r"), job) + self._job_runner_ui_delegate.ui = ui + + if isinstance(job, InvalidJob): + self._job_runner_ui_delegate.on_begin("", dict()) + for error_line in job.error_lines: + self._job_runner_ui_delegate.on_chunk( + "stderr", (error_line + "\n").encode("utf8") + ) + self._job_runner_ui_delegate.on_end(1) + return JobResultBuilder( + outcome=IJobResult.OUTCOME_FAIL, + return_code=1, + io_log=[ + IOLogRecord( + 0, "stderr", "\n".join(job.error_lines).encode("utf8") + ) + ], + ).get_result() + if job.plugin not in supported_plugins: print( Colorizer().RED( @@ -108,7 +133,6 @@ def run_job(self, job, job_state, environ=None, ui=None): outcome=IJobResult.OUTCOME_SKIP, comments=_("Job skipped in dry-run mode"), ).get_result() - self._job_runner_ui_delegate.ui = ui # for cached resource jobs we get the result using cache # if it's not in the cache, ordinary "_run_command" will be run diff --git a/checkbox-ng/plainbox/impl/session/assistant.py b/checkbox-ng/plainbox/impl/session/assistant.py index 6d91fa1dc4..3ed8ecfd46 100644 --- a/checkbox-ng/plainbox/impl/session/assistant.py +++ b/checkbox-ng/plainbox/impl/session/assistant.py @@ -513,6 +513,7 @@ def start_new_session( self._metadata.app_id = self._app_id self._metadata.title = title self._metadata.flags = {SessionMetaData.FLAG_BOOTSTRAPPING} + self._metadata.update_feature_flags(self._config) self._manager.checkpoint() self._command_io_delegate = JobRunnerUIDelegate(_SilentUI()) self._init_runner(runner_cls, runner_kwargs) @@ -848,7 +849,7 @@ def bootstrap(self): UsageExpectation.of(self).allowed_calls = ( self._get_allowed_calls_in_normal_state() ) - self._metadata.flags = {SessionMetaData.FLAG_INCOMPLETE} + self._metadata.flags.add(SessionMetaData.FLAG_INCOMPLETE) self._manager.checkpoint() @raises(UnexpectedMethodCall) @@ -980,7 +981,8 @@ def finish_bootstrap(self): UsageExpectation.of(self).allowed_calls = ( self._get_allowed_calls_in_normal_state() ) - self._metadata.flags = {SessionMetaData.FLAG_INCOMPLETE} + self._metadata.flags.remove(SessionMetaData.FLAG_BOOTSTRAPPING) + self._metadata.flags.add(SessionMetaData.FLAG_INCOMPLETE) self._manager.checkpoint() # No bootstrap is done update the cache of jobs that were run # during bootstrap phase diff --git a/checkbox-ng/plainbox/impl/session/resume.py b/checkbox-ng/plainbox/impl/session/resume.py index bd202e0cee..db0db7ab81 100644 --- a/checkbox-ng/plainbox/impl/session/resume.py +++ b/checkbox-ng/plainbox/impl/session/resume.py @@ -1239,13 +1239,13 @@ def _build_SessionState(self, session_repr, early_cb=None): ) session = new_session # Restore bits and pieces of state + logger.debug(_("Starting to restore metadata...")) + self._restore_SessionState_metadata(session.metadata, session_repr) + logger.debug(_("restored metadata %r"), session.metadata) logger.debug( _("Starting to restore jobs and results to %r..."), session ) self._restore_SessionState_jobs_and_results(session, session_repr) - logger.debug(_("Starting to restore metadata...")) - self._restore_SessionState_metadata(session.metadata, session_repr) - logger.debug(_("restored metadata %r"), session.metadata) logger.debug(_("Starting to restore mandatory job list...")) self._restore_SessionState_mandatory_job_list(session, session_repr) logger.debug(_("Starting to restore desired job list...")) diff --git a/checkbox-ng/plainbox/impl/session/state.py b/checkbox-ng/plainbox/impl/session/state.py index b4cbf6bb7a..d766cc198a 100644 --- a/checkbox-ng/plainbox/impl/session/state.py +++ b/checkbox-ng/plainbox/impl/session/state.py @@ -77,6 +77,8 @@ class SessionMetaData: # and is not following any test plan FLAG_TESTPLANLESS = "testplanless" + FLAG_FEATURE_STRICT_TEMPLATE_EXPANSION = "strict_template_expansion" + def __init__( self, title=None, @@ -124,6 +126,12 @@ def title(self, title): """set the session title to the given value.""" self._title = title + def update_feature_flags(self, config): + if config.get_value("features", "strict_template_expansion"): + self._flags.add(self.FLAG_FEATURE_STRICT_TEMPLATE_EXPANSION) + else: + logger.warning("Using legacy non-strict template expansion") + @property def flags(self): """ diff --git a/checkbox-ng/plainbox/impl/session/test_state.py b/checkbox-ng/plainbox/impl/session/test_state.py index ada1518722..4674fe38d3 100644 --- a/checkbox-ng/plainbox/impl/session/test_state.py +++ b/checkbox-ng/plainbox/impl/session/test_state.py @@ -1005,6 +1005,40 @@ def test_app_id_kwarg_to_init(self): "com.canonical.certification.plainbox", ) + def test_update_feature_flags_strict_template_expansion(self): + self_mock = MagicMock() + self_mock._flags = set() + self_mock.FLAG_FEATURE_STRICT_TEMPLATE_EXPANSION = ( + SessionMetaData.FLAG_FEATURE_STRICT_TEMPLATE_EXPANSION + ) + + config_mock = MagicMock() + config_mock.get_value.return_value = True + SessionMetaData.update_feature_flags(self_mock, config_mock) + + self.assertIn( + SessionMetaData.FLAG_FEATURE_STRICT_TEMPLATE_EXPANSION, + self_mock._flags, + ) + + @patch("plainbox.impl.session.state.logger") + def test_update_feature_flags_strict_template_expansion_warn(self, logger): + self_mock = MagicMock() + self_mock._flags = set() + self_mock.FLAG_FEATURE_STRICT_TEMPLATE_EXPANSION = ( + SessionMetaData.FLAG_FEATURE_STRICT_TEMPLATE_EXPANSION + ) + + config_mock = MagicMock() + config_mock.get_value.return_value = False + SessionMetaData.update_feature_flags(self_mock, config_mock) + + self.assertNotIn( + SessionMetaData.FLAG_FEATURE_STRICT_TEMPLATE_EXPANSION, + self_mock._flags, + ) + self.assertTrue(logger.warning.called) + class SessionDeviceContextTests(SignalTestCase): def setUp(self): diff --git a/checkbox-ng/plainbox/impl/test_ctrl.py b/checkbox-ng/plainbox/impl/test_ctrl.py index f35d0d6daf..35a380c1af 100644 --- a/checkbox-ng/plainbox/impl/test_ctrl.py +++ b/checkbox-ng/plainbox/impl/test_ctrl.py @@ -23,35 +23,33 @@ Test definitions for plainbox.impl.ctrl module """ -from subprocess import CalledProcessError from unittest import TestCase import json -import os -import shutil from plainbox.abc import IJobResult -from plainbox.abc import IProvider1 -from plainbox.abc import IProviderBackend1 from plainbox.suspend_consts import Suspend -from plainbox.impl.ctrl import CheckBoxSessionStateController -from plainbox.impl.ctrl import SymLinkNest -from plainbox.impl.ctrl import gen_rfc822_records_from_io_log +from plainbox.impl.ctrl import ( + CheckBoxSessionStateController, + SymLinkNest, + gen_rfc822_records_from_io_log, +) from plainbox.impl.job import JobDefinition -from plainbox.impl.resource import Resource -from plainbox.impl.resource import ResourceExpression -from plainbox.impl.result import MemoryJobResult -from plainbox.impl.secure.origin import JobOutputTextSource -from plainbox.impl.secure.origin import Origin +from plainbox.impl.unit.job import InvalidJob +from plainbox.impl.resource import Resource, ResourceExpression +from plainbox.impl.secure.origin import JobOutputTextSource, Origin from plainbox.impl.secure.providers.v1 import Provider1 -from plainbox.impl.secure.rfc822 import RFC822Record -from plainbox.impl.secure.rfc822 import RFC822SyntaxError -from plainbox.impl.session import InhibitionCause -from plainbox.impl.session import JobReadinessInhibitor -from plainbox.impl.session import JobState -from plainbox.impl.session import SessionState +from plainbox.impl.secure.rfc822 import RFC822Record, RFC822SyntaxError +from plainbox.impl.session import ( + InhibitionCause, + JobReadinessInhibitor, + JobState, + SessionState, +) + from plainbox.impl.testing_utils import make_job from plainbox.impl.unit.template import TemplateUnit -from plainbox.vendor import extcmd +from plainbox.impl.validation import Severity +from plainbox.impl.unit.unit import MissingParam from plainbox.vendor import mock @@ -436,12 +434,120 @@ def test_observe_result__missing_resource_key(self, mock_logger): session_state = SessionState([template, job]) self.ctrl.observe_result(session_state, job, result) # Ensure that a warning was logged - mock_logger.debug.assert_called_with( + mock_logger.critical.assert_called_with( "Ignoring %s with missing template parameter %s", "foo-{missing}", "missing", ) + @mock.patch("plainbox.impl.ctrl.logger") + def test_observe_result__missing_resource_key_invalid_units( + self, mock_logger + ): + job = make_job("R", plugin="resource") + template = TemplateUnit( + { + "template-resource": job.id, + "id": "foo-{missing}", + "plugin": "shell", + } + ) + result = mock.Mock(spec=IJobResult, outcome=IJobResult.OUTCOME_PASS) + result.get_io_log.return_value = [ + (0, "stdout", b"attr: value1\n"), + (0, "stdout", b"\n"), + (0, "stdout", b"attr: value2\n"), + ] + session_state = SessionState([template, job]) + session_state.metadata.flags.add( + session_state.metadata.FLAG_FEATURE_STRICT_TEMPLATE_EXPANSION + ) + self.ctrl.observe_result(session_state, job, result) + + job_ids = set(session_state.job_state_map.keys()) + job_ids.remove("R") + # 2 resource outputs will generate 2 jobs + self.assertEqual(len(job_ids), 2) + self.assertTrue(all(job_id.startswith("foo-") for job_id in job_ids)) + + @mock.patch("plainbox.impl.ctrl.logger") + def test__filter_invalid_log_valid(self, mock_logger): + unit_warning = mock.MagicMock(severity=Severity.warning) + valid_unit = mock.MagicMock() + valid_unit.check.return_value = [unit_warning] + kept = CheckBoxSessionStateController._filter_invalid_log(valid_unit) + + self.assertTrue(mock_logger.warning.called) + # unit is valid, has to be kept + self.assertTrue(kept) + + @mock.patch("plainbox.impl.ctrl.logger") + def test__filter_invalid_log_error(self, mock_logger): + unit_warning = mock.MagicMock(severity=Severity.warning) + unit_error = mock.MagicMock(severity=Severity.error) + invalid_unit = mock.MagicMock() + invalid_unit.check.return_value = [unit_warning, unit_error] + kept = CheckBoxSessionStateController._filter_invalid_log(invalid_unit) + + self.assertTrue(mock_logger.warning.called) + # unit contains at least 1 error, it has to be discarded + self.assertFalse(kept) + # this is dangerous as the invalid unit will be ignored + self.assertTrue(mock_logger.critical.called) + + @mock.patch("plainbox.impl.ctrl.logger") + def test__filter_invalid_log_error_missing_paramn(self, mock_logger): + invalid_unit = mock.MagicMock() + invalid_unit.check.side_effect = MissingParam( + "template_123", "id", "generated_unit_id{abc}", "abc" + ) + kept = CheckBoxSessionStateController._filter_invalid_log(invalid_unit) + + # unit contains at least 1 error, it has to be discarded + self.assertFalse(kept) + # this is dangerous as the invalid unit will be ignored + self.assertTrue(mock_logger.critical.called) + + @mock.patch("plainbox.impl.ctrl.logger") + def test__wrap_invalid_units_valid(self, mock_logger): + unit_warning = mock.MagicMock(severity=Severity.warning) + valid_unit = mock.MagicMock() + valid_unit.check.return_value = [unit_warning] + wrapped = CheckBoxSessionStateController._wrap_invalid_units( + valid_unit + ) + + self.assertTrue(mock_logger.warning.called) + # unit is valid, has should return the unit unchanged + self.assertEqual(wrapped, valid_unit) + + @mock.patch("plainbox.impl.ctrl.logger") + def test__wrap_invalid_units_errors(self, mock_logger): + unit_warning = mock.MagicMock(severity=Severity.warning) + unit_error = mock.MagicMock(severity=Severity.error) + invalid_unit = mock.MagicMock() + invalid_unit.check.return_value = [unit_warning, unit_error] + wrapped = CheckBoxSessionStateController._wrap_invalid_units( + invalid_unit + ) + + self.assertTrue(mock_logger.warning.called) + # unit contains at least 1 error, has to be wrapped to make reporting + # possible + self.assertIsInstance(wrapped, InvalidJob) + + def test__wrap_invalid_units_missing_param(self): + invalid_unit = mock.MagicMock() + invalid_unit.check.side_effect = MissingParam( + "template_123", "id", "generated_unit_id{abc}", "abc" + ) + wrapped = CheckBoxSessionStateController._wrap_invalid_units( + invalid_unit + ) + + # unit contains at least 1 error, it has to be discarded + self.assertIsInstance(wrapped, InvalidJob) + class FunctionTests(TestCase): """ diff --git a/checkbox-ng/plainbox/impl/test_execution.py b/checkbox-ng/plainbox/impl/test_execution.py new file mode 100644 index 0000000000..e922af51a3 --- /dev/null +++ b/checkbox-ng/plainbox/impl/test_execution.py @@ -0,0 +1,42 @@ +# This file is part of Checkbox. +# +# Copyright 2024 Canonical Ltd. +# Written by: +# Massimiliano Girardi +# +# Checkbox is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License version 3, +# as published by the Free Software Foundation. +# +# Checkbox is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with Checkbox. If not, see . + +from plainbox.impl.execution import UnifiedRunner +from plainbox.impl.unit.job import InvalidJob + +from unittest import TestCase, mock + + +class UnifiedRunnerTests(TestCase): + def test_run_job_invalid_job(self): + self_mock = mock.MagicMock() + + invalid_unit = mock.MagicMock( + _data={"id": "generated_id_{param}"}, parameters={} + ) + invalid_job = InvalidJob.from_unit(invalid_unit, errors=["Some error"]) + ui = mock.MagicMock() + + result = UnifiedRunner.run_job(self_mock, invalid_job, None, ui=ui) + + output_writer = self_mock._job_runner_ui_delegate + self.assertTrue(output_writer.on_begin.called) + # error is reported via the ui + self.assertTrue(output_writer.on_chunk.called) + self.assertTrue(output_writer.on_end.called) + self.assertEqual(result.outcome, "fail") diff --git a/checkbox-ng/plainbox/impl/unit/job.py b/checkbox-ng/plainbox/impl/unit/job.py index 44d35f9443..48388f201b 100644 --- a/checkbox-ng/plainbox/impl/unit/job.py +++ b/checkbox-ng/plainbox/impl/unit/job.py @@ -26,35 +26,30 @@ import json import logging import re -import os + +from collections import defaultdict from plainbox.abc import IJobDefinition -from plainbox.i18n import gettext as _ -from plainbox.i18n import gettext_noop as N_ -from plainbox.impl.decorators import cached_property -from plainbox.impl.decorators import instance_method_lru_cache -from plainbox.impl.resource import ResourceProgram -from plainbox.impl.resource import parse_imports_stmt -from plainbox.impl.secure.origin import JobOutputTextSource +from plainbox.i18n import gettext as _, gettext_noop as N_ +from plainbox.impl.decorators import cached_property, instance_method_lru_cache +from plainbox.impl.resource import ResourceProgram, parse_imports_stmt from plainbox.impl.secure.origin import Origin from plainbox.impl.symbol import SymbolDef from plainbox.impl.unit import concrete_validators from plainbox.impl.unit.unit_with_id import UnitWithId -from plainbox.impl.unit.validators import CorrectFieldValueValidator -from plainbox.impl.unit.validators import DeprecatedFieldValidator -from plainbox.impl.unit.validators import MemberOfFieldValidator -from plainbox.impl.unit.validators import PresentFieldValidator -from plainbox.impl.unit.validators import ReferenceConstraint -from plainbox.impl.unit.validators import ShellProgramValidator -from plainbox.impl.unit.validators import UnitReferenceValidator -from plainbox.impl.unit.validators import UselessFieldValidator - -from plainbox.impl.validation import Problem -from plainbox.impl.validation import Severity -from plainbox.impl.xparsers import Error -from plainbox.impl.xparsers import Text -from plainbox.impl.xparsers import Visitor -from plainbox.impl.xparsers import WordList +from plainbox.impl.unit.validators import ( + CorrectFieldValueValidator, + DeprecatedFieldValidator, + MemberOfFieldValidator, + PresentFieldValidator, + ReferenceConstraint, + ShellProgramValidator, + UnitReferenceValidator, + UselessFieldValidator, +) + +from plainbox.impl.validation import Problem, Severity +from plainbox.impl.xparsers import Error, Text, Visitor, WordList __all__ = ["JobDefinition", "propertywithsymbols"] @@ -1081,3 +1076,75 @@ class fields(SymbolDef): MemberOfFieldValidator(_AutoRetryValues.get_all_symbols()), ], } + + +class InvalidJob(JobDefinition): + + def __init__(self, *args, errors=[], **kwargs): + assert errors, "Unit is not invalid" + self.errors = errors + super().__init__(*args, **kwargs) + + @property + def error_lines(self) -> str: + return [str(x) for x in self.errors] + + @classmethod + def wrap_default_missing(cls, dct): + def missing_param(): + # if the __index__ wasn't generated, something is extra wrong here + # lets crash + return "MISSING_PARAM_{}".format(dct["__index__"]) + + return defaultdict(missing_param, dct) + + @classmethod + def from_unit(cls, unit, errors): + # values for the parameters. Given that this may be invalid due to + # missing parameters, lets give a default so that we can always expand + # the unit + parameters = cls.wrap_default_missing(unit.parameters) + return cls( + unit._data, + parameters=parameters, + provider=unit.provider, + errors=errors, + ) + + class Meta: + + name = N_("job") + + class fields(SymbolDef): + """ + Symbols for each field that a InvalidJob can have + """ + + name = "name" + summary = "summary" + plugin = "plugin" + command = "command" + description = "description" + user = "user" + environ = "environ" + estimated_duration = "estimated_duration" + depends = "depends" + after = "after" + salvages = "salvages" + requires = "requires" + shell = "shell" + imports = "imports" + flags = "flags" + category_id = "category_id" + purpose = "purpose" + steps = "steps" + verification = "verification" + certification_status = "certification_status" + siblings = "siblings" + auto_retry = "auto_retry" + + def __str__(self): + return self.summary + + def __repr__(self): + return "".format(self.id, self.plugin) diff --git a/checkbox-ng/plainbox/impl/unit/test_unit.py b/checkbox-ng/plainbox/impl/unit/test_unit.py index ce8e27b847..8b65990a1e 100644 --- a/checkbox-ng/plainbox/impl/unit/test_unit.py +++ b/checkbox-ng/plainbox/impl/unit/test_unit.py @@ -203,8 +203,14 @@ def test_get_record_value(self): self.assertEqual(unit1.get_record_value("key"), "value") self.assertEqual(unit2.get_record_value("key"), "value") self.assertEqual(unit3.get_record_value("key"), "value") - with self.assertRaises(MissingParam): + with self.assertRaises(MissingParam) as mp: + # TODO: this is structured badly, if a unit has a template-engine + # it is a template, but the definition is in Unit, not + # TemplateUnit. Once you fix it, remove the following line + unit4.template_id = "template_id" unit4.get_record_value("key") + self.assertIn("template_id", str(mp.exception)) + self.assertIn("missing_param", repr(mp.exception)) self.assertEqual(unit5.get_record_value("key"), None) self.assertEqual(unit5.get_record_value("key", "default"), "default") self.assertEqual(unit6.get_record_value("key"), None) diff --git a/checkbox-ng/plainbox/impl/unit/unit.py b/checkbox-ng/plainbox/impl/unit/unit.py index b1c62426ce..f8bdbaed30 100644 --- a/checkbox-ng/plainbox/impl/unit/unit.py +++ b/checkbox-ng/plainbox/impl/unit/unit.py @@ -83,15 +83,25 @@ class MissingParam(Exception): the name of the missing parameter. """ - def __init__(self, parameter): + def __init__(self, template_id, field, value, parameter): + self.template_id = template_id + self.field = field + self.value = value self.parameter = parameter def __str__(self): - return _("failed to find value for paramter {}").format(self.parameter) + return ( + "Template '{}' generated an invalid unit. Field '{}' has value " + "'{}' but the template-resource didn't generate any value for " + "'{}'" + ).format(self.template_id, self.field, self.value, self.parameter) def __repr__(self): - return "<{} paramter:{}>".format( - self.__class__.__name__, self.parameter + return "<{} template_id:{} field: {} paramter:{}>".format( + self.__class__.__name__, + self.template_id, + self.field, + self.parameter, ) @@ -708,7 +718,9 @@ def get_record_value(self, name, default=None): value, (), self.parameters ) except KeyError as e: - raise MissingParam(e.args[0]) + raise MissingParam( + self.template_id, name, value, e.args[0] + ) elif ( value is not None and self.template_engine == "jinja2" diff --git a/metabox/metabox/metabox-provider/units/basic-jobs.pxu b/metabox/metabox/metabox-provider/units/basic-jobs.pxu index b18e0a1a43..024db03129 100644 --- a/metabox/metabox/metabox-provider/units/basic-jobs.pxu +++ b/metabox/metabox/metabox-provider/units/basic-jobs.pxu @@ -255,3 +255,44 @@ id: dependency_installation command: dependency_installation.py _summary: Verify that all required modules are installed _description: Verify that all required modules are installed + +id: resource_names +_summary: Returns names usable in templates along with their validity as an id +plugin: resource +environ: + CRASH +command: + echo "name: somename" + echo "id_valid: True" + echo "body: body" + echo + echo 'name: invalid_body' + echo "id_valid: True" + echo "body: 'this breaks the body" + echo + echo "id_valid: noname" + +unit: template +template-resource: resource_names +template-unit: job +id: template_validation_invalid_fields_{name} +template-id: template_validation_invalid_fields +template-filter: resource_names.id_valid in ["True", "False"] +command: + echo Testing {body} +_summary: Tests that generated units are validated and reported when invalid +flags: simple + + +unit: template +template-resource: resource_names +template-unit: job +id: template_validation_missing_parameter_{name} +template-filter: resource_names.id_valid == "noname" +template-id: template_validation_missing_parameter +command: + # note: missing params is a special case because it triggers a different kind + # of error that is not a validation error + echo Testing +_summary: Tests that generated units are validated and reported when params are missing +flags: simple diff --git a/metabox/metabox/metabox-provider/units/basic-tps.pxu b/metabox/metabox/metabox-provider/units/basic-tps.pxu index dd47b701f1..9cc83f1e99 100644 --- a/metabox/metabox/metabox-provider/units/basic-tps.pxu +++ b/metabox/metabox/metabox-provider/units/basic-tps.pxu @@ -69,3 +69,12 @@ _name: Pass and Crash include: basic-shell-passing basic-shell-crashing + +unit: test plan +id: report_missing_parameters_generated_units +_name: Check that invalid units generated by templates are reported +bootstrap_include: + resource_names +include: + template_validation_invalid_fields + template_validation_missing_parameter diff --git a/metabox/metabox/metabox-provider/units/resume.pxu b/metabox/metabox/metabox-provider/units/resume.pxu index 0b7fb520df..dcc6c560c5 100644 --- a/metabox/metabox/metabox-provider/units/resume.pxu +++ b/metabox/metabox/metabox-provider/units/resume.pxu @@ -39,3 +39,13 @@ _summary: Test that passes only at the secont try include: pass-rerun basic-shell-failing + +unit: test plan +id: resume_report_missing_parameters_generated_units +_name: Check that invalid units generated by templates are reported +bootstrap_include: + resource_names +include: + reboot-emulator + template_validation_invalid_fields + template_validation_missing_parameter diff --git a/metabox/metabox/scenarios/basic/template_expansion.py b/metabox/metabox/scenarios/basic/template_expansion.py new file mode 100644 index 0000000000..f13ab12581 --- /dev/null +++ b/metabox/metabox/scenarios/basic/template_expansion.py @@ -0,0 +1,219 @@ +# This file is part of Checkbox. +# +# Copyright 2024 Canonical Ltd. +# +# Checkbox is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License version 3, +# as published by the Free Software Foundation. +# +# Checkbox is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with Checkbox. If not, see . + +import textwrap + +from metabox.core.scenario import Scenario +from metabox.core.actions import ( + AssertRetCode, + Start, + AssertPrinted, + AssertNotPrinted, +) +from metabox.core.utils import tag + + +@tag("template", "basic") +class InvalidUnitErrorUserVisible(Scenario): + """ + Check that when Checkbox expands a template it correctly detects + if some of the expanded units are invalid and reports it to the user + """ + + launcher = textwrap.dedent( + """ + #!/usr/bin/env checkbox-cli + [launcher] + launcher_version = 1 + stock_reports = text, submission_files + [test plan] + unit = 2021.com.canonical.certification::report_missing_parameters_generated_units + forced = yes + [ui] + type=silent + [test selection] + forced=yes + [transport:local_file] + type=file + path=c3-local-submission.tar.xz + [exporter:example_tar] + unit = com.canonical.plainbox::tar + [report:file] + transport = local_file + exporter = tar + forced = yes + """ + ) + steps = [ + Start(), + AssertPrinted("template_validation_invalid_fields_somename"), + AssertPrinted("Outcome: job passed"), + AssertPrinted("template_validation_invalid_fields_invalid_body"), + AssertPrinted("Outcome: job failed"), + AssertPrinted("template_validation_missing_parameter_MISSING_PARAM_1"), + AssertPrinted("Outcome: job failed"), + AssertRetCode(1), + ] + + +@tag("template", "resume", "basic") +class LocalResumeInvalidUnitErrorUserVisible(Scenario): + """ + Check that when Checkbox expands a template it correctly detects + if some of the expanded units are invalid and reports it to the user + """ + + modes = ["local"] + launcher = textwrap.dedent( + """ + #!/usr/bin/env checkbox-cli + [launcher] + launcher_version = 1 + stock_reports = text, submission_files + [test plan] + unit = 2021.com.canonical.certification::resume_report_missing_parameters_generated_units + forced = yes + [ui] + type=silent + [test selection] + forced=yes + [transport:local_file] + type=file + path=c3-local-submission.tar.xz + [exporter:example_tar] + unit = com.canonical.plainbox::tar + [report:file] + transport = local_file + exporter = tar + forced = yes + """ + ) + steps = [ + Start(), + AssertPrinted("reboot-emulator"), + Start(), + AssertPrinted("template_validation_invalid_fields_somename"), + AssertPrinted("job passed"), + AssertPrinted("template_validation_invalid_fields_invalid_body"), + AssertPrinted("job failed"), + AssertPrinted("template_validation_missing_parameter_MISSING_PARAM_1"), + AssertPrinted("job failed"), + AssertPrinted("job passed"), + AssertPrinted("job passed"), + AssertPrinted("job failed"), + AssertPrinted("job failed"), + AssertRetCode(1), + ] + + +@tag("template", "resume", "basic") +class RemoteResumeInvalidUnitErrorUserVisible(Scenario): + """ + Check that when Checkbox expands a template it correctly detects + if some of the expanded units are invalid and reports it to the user + """ + + modes = ["remote"] + launcher = textwrap.dedent( + """ + #!/usr/bin/env checkbox-cli + [launcher] + launcher_version = 1 + stock_reports = text, submission_files + [test plan] + unit = 2021.com.canonical.certification::resume_report_missing_parameters_generated_units + forced = yes + [ui] + type=silent + [test selection] + forced=yes + [transport:local_file] + type=file + path=c3-local-submission.tar.xz + [exporter:example_tar] + unit = com.canonical.plainbox::tar + [report:file] + transport = local_file + exporter = tar + forced = yes + """ + ) + steps = [ + Start(), + AssertPrinted("reboot-emulator"), + AssertPrinted("template_validation_invalid_fields_somename"), + AssertPrinted("job passed"), + AssertPrinted("template_validation_invalid_fields_invalid_body"), + AssertPrinted("job failed"), + AssertPrinted("template_validation_missing_parameter_MISSING_PARAM_1"), + AssertPrinted("job failed"), + AssertPrinted("job passed"), + AssertPrinted("job passed"), + AssertPrinted("job failed"), + AssertPrinted("job failed"), + AssertRetCode(1), + ] + + +@tag("template", "resume", "basic") +class RemoteResumeInvalidUnitErrorIgnoredBackwardCompatibility(Scenario): + """ + Check that when Checkbox expands a template it correctly detects + if some of the expanded units are invalid and reports it to the user + """ + + modes = ["remote"] + launcher = textwrap.dedent( + """ + #!/usr/bin/env checkbox-cli + [launcher] + launcher_version = 1 + stock_reports = text, submission_files + [test plan] + unit = 2021.com.canonical.certification::resume_report_missing_parameters_generated_units + forced = yes + [ui] + type=silent + [test selection] + forced=yes + [transport:local_file] + type=file + path=c3-local-submission.tar.xz + [exporter:example_tar] + unit = com.canonical.plainbox::tar + [report:file] + transport = local_file + exporter = tar + forced = yes + [features] + strict_template_expansion=false + """ + ) + steps = [ + Start(), + AssertPrinted("reboot-emulator"), + AssertPrinted("template_validation_invalid_fields_somename"), + AssertPrinted("job passed"), + AssertNotPrinted("template_validation_invalid_fields_invalid_body"), + AssertNotPrinted("job failed"), + AssertNotPrinted( + "template_validation_missing_parameter_MISSING_PARAM_1" + ), + AssertNotPrinted("job failed"), + AssertPrinted("job passed"), + AssertPrinted("job passed"), + AssertRetCode(0), + ]