From aa26996f7df709e996929f48de3fe903769bb3f9 Mon Sep 17 00:00:00 2001 From: Lukas Holecek Date: Jul 09 2019 05:32:32 +0000 Subject: Do at most single waivers request Retrieves waivers only when needed and in single request per decision. Signed-off-by: Lukas Holecek --- diff --git a/greenwave/api_v1.py b/greenwave/api_v1.py index a6d63c5..c145951 100644 --- a/greenwave/api_v1.py +++ b/greenwave/api_v1.py @@ -10,9 +10,10 @@ from greenwave.policies import (summarize_answers, RemotePolicy, OnDemandPolicy, _missing_decision_contexts_in_parent_policies) -from greenwave.resources import ResultsRetriever, retrieve_waivers +from greenwave.resources import ResultsRetriever, WaiversRetriever from greenwave.safe_yaml import SafeYAMLError from greenwave.utils import insert_headers, jsonp +from greenwave.waivers import waive_answers from greenwave.monitor import ( registry, decision_exception_counter, @@ -396,14 +397,20 @@ def make_decision(): answers = [] verbose_results = [] - verbose_waivers = [] applicable_policies = [] - results_retriever = ResultsRetriever( - ignore_results=ignore_results, + retriever_args = dict( when=when, timeout=current_app.config['REQUESTS_TIMEOUT'], - verify=current_app.config['REQUESTS_VERIFY'], - url=current_app.config['RESULTSDB_API_URL']) + verify=current_app.config['REQUESTS_VERIFY']) + results_retriever = ResultsRetriever( + ignore_ids=ignore_results, + url=current_app.config['RESULTSDB_API_URL'], + **retriever_args) + waivers_retriever = WaiversRetriever( + ignore_ids=ignore_waivers, + url=current_app.config['WAIVERDB_API_URL'], + **retriever_args) + waiver_filters = [] policies = on_demand_policies or current_app.config['policies'] for subject_type, subject_identifier in _decision_subjects_for_request(data): @@ -427,22 +434,40 @@ def make_decision(): 'Cannot find any applicable policies for %s subjects at gating point %s in %s' % ( subject_type, decision_context, product_version)) - waivers = retrieve_waivers( - product_version, subject_type, [subject_identifier], when) - if ignore_waivers: - waivers = [w for w in waivers if w['id'] not in ignore_waivers] - for policy in subject_policies: answers.extend( - policy.check(product_version, subject_identifier, results_retriever, waivers)) + policy.check( + product_version, + subject_identifier, + results_retriever)) applicable_policies.extend(subject_policies) if verbose: - # Retrieve test results for all items when verbose output is requested. + # Retrieve test results and waivers for all items when verbose output is requested. verbose_results.extend( results_retriever.retrieve(subject_type, subject_identifier)) - verbose_waivers.extend(waivers) + waiver_filters.append(dict( + subject_type=subject_type, + subject_identifier=subject_identifier, + product_version=product_version, + )) + + if not verbose: + for answer in answers: + if not answer.is_satisfied: + waiver_filters.append(dict( + subject_type=answer.subject_type, + subject_identifier=answer.subject_identifier, + product_version=product_version, + testcase=answer.test_case_name, + )) + + if waiver_filters: + waivers = waivers_retriever.retrieve(waiver_filters) + else: + waivers = [] + answers = waive_answers(answers, waivers) response = { 'policies_satisfied': all(answer.is_satisfied for answer in answers), @@ -458,10 +483,9 @@ def make_decision(): response.update({'applicable_policies': [policy.id for policy in applicable_policies]}) if verbose: - # removing duplicated elements... response.update({ 'results': list({result['id']: result for result in verbose_results}.values()), - 'waivers': list({waiver['id']: waiver for waiver in verbose_waivers}.values()), + 'waivers': list({waiver['id']: waiver for waiver in waivers}.values()), }) log.debug('Response: %s', response) diff --git a/greenwave/policies.py b/greenwave/policies.py index c8bf3b5..e1ee597 100644 --- a/greenwave/policies.py +++ b/greenwave/policies.py @@ -93,6 +93,12 @@ class RuleNotSatisfied(Answer): def to_json(self): raise NotImplementedError() + def to_waived(self): + """ + Transform unsatisfied answer to waived one. + """ + raise NotImplementedError() + class TestResultMissing(RuleNotSatisfied): """ @@ -117,6 +123,13 @@ class TestResultMissing(RuleNotSatisfied): 'item': subject_type_identifier_to_item(self.subject_type, self.subject_identifier), } + def to_waived(self): + return TestResultMissingWaived( + self.subject_type, + self.subject_identifier, + self.test_case_name, + self.scenario) + class TestResultMissingWaived(RuleSatisfied): """ @@ -163,6 +176,9 @@ class TestResultFailed(RuleNotSatisfied): 'scenario': self.scenario, } + def to_waived(self): + return TestResultPassed(self.test_case_name, self.result_id) + class InvalidGatingYaml(RuleNotSatisfied): """ @@ -184,6 +200,9 @@ class InvalidGatingYaml(RuleNotSatisfied): 'details': self.details } + def to_waived(self): + return None + class TestResultPassed(RuleSatisfied): """ @@ -275,7 +294,12 @@ class Rule(SafeYAMLObject): This base class is not used directly. """ - def check(self, policy, product_version, subject_identifier, results_retriever, waivers): + def check( + self, + policy, + product_version, + subject_identifier, + results_retriever): """ Evaluate this policy rule for the given item. @@ -286,7 +310,6 @@ class Rule(SafeYAMLObject): example, Koji build NVR, Bodhi update id, ...) results_retriever (ResultsRetriever): Object for retrieving data from ResultsDB. - waivers (list): List of waiver objects looked up in WaiverDB for the results. Returns: Answer: An instance of a subclass of :py:class:`Answer` describing the result. @@ -337,12 +360,6 @@ class Rule(SafeYAMLObject): return processed_rules -def waives_invalid_gating_yaml(waiver, subject_type, subject_identifier): - return (waiver['testcase'] == 'invalid-gating-yaml' and - waiver['subject']['type'] == subject_type and - waiver['subject']['item'] == subject_identifier) - - class RemoteRule(Rule): yaml_tag = '!RemoteRule' safe_yaml_attributes = {} @@ -376,14 +393,15 @@ class RemoteRule(Rule): if sub_policy.decision_context == policy.decision_context ] - def check(self, policy, product_version, subject_identifier, results_retriever, waivers): - + def check( + self, + policy, + product_version, + subject_identifier, + results_retriever): try: policies = self._get_sub_policies(policy, subject_identifier) except SafeYAMLError as e: - if any(waives_invalid_gating_yaml(waiver, policy.subject_type, subject_identifier) - for waiver in waivers): - return [] return [ InvalidGatingYaml( policy.subject_type, subject_identifier, 'invalid-gating-yaml', str(e)) @@ -393,7 +411,7 @@ class RemoteRule(Rule): for remote_policy in policies: if remote_policy.matches_product_version(product_version): response = remote_policy.check( - product_version, subject_identifier, results_retriever, waivers) + product_version, subject_identifier, results_retriever) if isinstance(response, list): answers.extend(response) @@ -438,11 +456,14 @@ class PassingTestCaseRule(Rule): 'scenario': SafeYAMLString(optional=True), } - def check(self, policy, product_version, subject_identifier, results_retriever, waivers): + def check( + self, + policy, + product_version, + subject_identifier, + results_retriever): matching_results = results_retriever.retrieve( policy.subject_type, subject_identifier, self.test_case_name) - matching_waivers = [ - w for w in waivers if (w['testcase'] == self.test_case_name and w['waived'] is True)] if self.scenario is not None: matching_results = [ @@ -451,10 +472,7 @@ class PassingTestCaseRule(Rule): # Investigate the absence of result first. if not matching_results: - if not matching_waivers: - return TestResultMissing( - policy.subject_type, subject_identifier, self.test_case_name, self.scenario) - return TestResultMissingWaived( + return TestResultMissing( policy.subject_type, subject_identifier, self.test_case_name, self.scenario) # For compose make decisions based on all architectures and variants. @@ -474,7 +492,10 @@ class PassingTestCaseRule(Rule): if arch_variant not in visited_arch_variants: visited_arch_variants.add(arch_variant) answer = self._answer_for_result( - result, waivers, policy.subject_type, subject_identifier) + result, + product_version, + policy.subject_type, + subject_identifier) answers.append(answer) return answers @@ -485,7 +506,10 @@ class PassingTestCaseRule(Rule): answers = [] for result in matching_results: answers.append(self._answer_for_result( - result, waivers, policy.subject_type, subject_identifier)) + result, + product_version, + policy.subject_type, + subject_identifier)) return answers def matches(self, policy, **attributes): @@ -499,25 +523,14 @@ class PassingTestCaseRule(Rule): 'scenario': self.scenario, } - def _answer_for_result(self, result, waivers, subject_type, subject_identifier): + def _answer_for_result( + self, result, product_version, subject_type, subject_identifier): if result['outcome'] in ('PASSED', 'INFO'): log.debug('Test result passed for the result_id %s and testcase %s,' ' because the outcome is %s', result['id'], self.test_case_name, result['outcome']) return TestResultPassed(self.test_case_name, result['id']) - # TODO limit who is allowed to waive - - matching_waivers = [w for w in waivers if ( - w['subject_type'] == subject_type and - w['subject_identifier'] == result['data']['item'][0] and - w['testcase'] == result['testcase']['name'] and - w['waived'] is True - )] - if matching_waivers: - log.debug('Found matching waivers for the result_id %s and the testcase %s,' - ' so the Test result is PASSED', result['id'], self.test_case_name) - return TestResultPassed(self.test_case_name, result['id']) if result['outcome'] in ('QUEUED', 'RUNNING'): log.debug('Test result MISSING for the subject_type %s, subject_identifier %s and ' 'testcase %s, because the outcome is %s', subject_type, subject_identifier, @@ -544,7 +557,12 @@ class ObsoleteRule(Rule): tag = self.yaml_tag or '!' + type(self).__name__ raise SafeYAMLError('{} is obsolete. {}'.format(tag, self.advice)) - def check(self, policy, product_version, subject_identifier, results_retriever, waivers): + def check( + self, + policy, + product_version, + subject_identifier, + results_retriever): raise ValueError('This rule is obsolete and can\'t be checked') @@ -598,7 +616,11 @@ class Policy(SafeYAMLObject): return not self.rules or any(rule.matches(self, **attributes) for rule in self.rules) - def check(self, product_version, subject_identifier, results_retriever, waivers): + def check( + self, + product_version, + subject_identifier, + results_retriever): # If an item is about a package and it is in the blacklist, return RuleSatisfied() if self.subject_type == 'koji_build': name = subject_identifier.rsplit('-', 2)[0] @@ -614,7 +636,10 @@ class Policy(SafeYAMLObject): answers = [] for rule in self.rules: response = rule.check( - self, product_version, subject_identifier, results_retriever, waivers) + self, + product_version, + subject_identifier, + results_retriever) if isinstance(response, list): answers.extend(response) else: diff --git a/greenwave/resources.py b/greenwave/resources.py index bdf23ae..47f349b 100644 --- a/greenwave/resources.py +++ b/greenwave/resources.py @@ -26,59 +26,88 @@ log = logging.getLogger(__name__) requests_session = get_requests_session() -class ResultsRetriever(object): - """ - Retrieves results from ResultsDB. - """ - def __init__(self, ignore_results, when, timeout, verify, url): - self.ignore_results = ignore_results - self.when = when +class BaseRetriever: + def __init__(self, ignore_ids, when, timeout, verify, url): + self.ignore_ids = ignore_ids self.timeout = timeout self.verify = verify self.url = url - def retrieve(self, subject_type, subject_identifier, testcase=None, scenarios=None): - """ - Return generator over results. - """ - params = {} - if self.when: - params.update({'since': '1900-01-01T00:00:00.000000,{}'.format(self.when)}) + if when: + self.since = '1900-01-01T00:00:00.000000,{}'.format(when) + else: + self.since = None + + def retrieve(self, *args, **kwargs): + items = self._retrieve_all(*args, **kwargs) + return [item for item in items if item['id'] not in self.ignore_ids] + + def _retrieve_data(self, params): + response = self._make_request(params, verify=self.verify, timeout=self.timeout) + response.raise_for_status() + return response.json()['data'] + + +class ResultsRetriever(BaseRetriever): + """ + Retrieves results from cache or ResultsDB. + """ + def _retrieve_all(self, subject_type, subject_identifier, testcase=None, scenarios=None): + params = { + '_distinct_on': 'scenario,system_architecture' + } + if self.since: + params.update({'since': self.since}) if testcase: params.update({'testcases': testcase}) if scenarios: params.update({'scenario': ','.join(scenarios)}) - return self._retrieve_helper(params, subject_type, subject_identifier) - - def _make_request(self, params): - params['_distinct_on'] = 'scenario,system_architecture' - response = requests_session.get( - self.url + '/results/latest', params=params, verify=self.verify, timeout=self.timeout) - response.raise_for_status() - return response.json()['data'] - def _retrieve_helper(self, params, subject_type, subject_identifier): results = [] if subject_type == 'koji_build': params['type'] = 'koji_build,brew-build' params['item'] = subject_identifier - results = self._make_request(params=params) + results = self._retrieve_data(params) del params['type'] del params['item'] params['original_spec_nvr'] = subject_identifier - results.extend(self._make_request(params=params)) + results.extend(self._retrieve_data(params)) elif subject_type == 'compose': params['productmd.compose.id'] = subject_identifier - results = self._make_request(params=params) + results = self._retrieve_data(params) else: params['type'] = subject_type params['item'] = subject_identifier - results = self._make_request(params=params) + results = self._retrieve_data(params) - results = [r for r in results if r['id'] not in self.ignore_results] return results + def _make_request(self, params, **request_args): + return requests_session.get( + self.url + '/results/latest', + params=params, + **request_args) + + +class WaiversRetriever(BaseRetriever): + """ + Retrieves waivers from WaiverDB. + """ + def _retrieve_all(self, filters): + if self.since: + for filter_ in filters: + filter_.update({'since': self.since}) + waivers = self._retrieve_data(filters) + return [waiver for waiver in waivers if waiver['waived']] + + def _make_request(self, params, **request_args): + return requests_session.post( + self.url + '/waivers/+filtered', + headers={'Content-Type': 'application/json'}, + data=json.dumps({'filters': params}), + **request_args) + @cached def retrieve_scm_from_koji(nvr): @@ -192,33 +221,6 @@ def _retrieve_yaml_remote_rule_git_archive(rev, pkg_name, pkg_namespace): return gating_yaml -# NOTE - not cached, for now. -def retrieve_waivers(product_version, subject_type, subject_identifiers, when): - if not subject_identifiers: - return [] - - timeout = current_app.config['REQUESTS_TIMEOUT'] - verify = current_app.config['REQUESTS_VERIFY'] - filters = [] - for subject_identifier in subject_identifiers: - d = { - 'product_version': product_version, - 'subject_type': subject_type, - 'subject_identifier': subject_identifier - } - if when: - d['since'] = '1900-01-01T00:00:00.000000,{}'.format(when) - filters.append(d) - response = requests_session.post( - current_app.config['WAIVERDB_API_URL'] + '/waivers/+filtered', - headers={'Content-Type': 'application/json'}, - data=json.dumps({'filters': filters}), - verify=verify, - timeout=timeout) - response.raise_for_status() - return response.json()['data'] - - # NOTE - not cached. def retrieve_decision(greenwave_url, data): timeout = current_app.config['REQUESTS_TIMEOUT'] diff --git a/greenwave/tests/test_api_v1.py b/greenwave/tests/test_api_v1.py new file mode 100644 index 0000000..e29d7d0 --- /dev/null +++ b/greenwave/tests/test_api_v1.py @@ -0,0 +1,92 @@ +# SPDX-License-Identifier: GPL-2.0+ + +import mock +import pytest + +from textwrap import dedent + +from greenwave.app_factory import create_app +from greenwave.policies import Policy + +DEFAULT_DECISION_DATA = dict( + decision_context='test_policies', + product_version='fedora-rawhide', + subject_type='koji_build', + subject_identifier='nethack-1.2.3-1.f31', +) + +DEFAULT_DECISION_POLICIES = """ + --- !Policy + id: "test_policy" + product_versions: + - fedora-rawhide + decision_context: test_policies + subject_type: koji_build + rules: + - !PassingTestCaseRule {test_case_name: sometest} +""" + + +def make_result(outcome): + return { + 'id': 123, + 'data': { + 'item': [DEFAULT_DECISION_DATA['subject_identifier']], + 'type': [DEFAULT_DECISION_DATA['subject_type']], + }, + 'testcase': {'name': 'sometest'}, + 'outcome': outcome, + } + + +@pytest.fixture +def mock_results(): + with mock.patch('greenwave.resources.ResultsRetriever.retrieve') as mocked: + mocked.return_value = [] + yield mocked + + +@pytest.fixture +def mock_waivers(): + with mock.patch('greenwave.resources.WaiversRetriever.retrieve') as mocked: + mocked.return_value = [] + yield mocked + + +def make_decision(**kwargs): + app = create_app('greenwave.config.TestingConfig') + app.config['policies'] = Policy.safe_load_all(dedent(DEFAULT_DECISION_POLICIES)) + client = app.test_client() + data = DEFAULT_DECISION_DATA.copy() + data.update(kwargs) + return client.post('/api/v1.0/decision', json=data) + + +def test_make_decision_retrieves_waivers_on_missing(mock_results, mock_waivers): + response = make_decision() + assert 200 == response.status_code + assert '1 of 1 required test results missing' == response.json['summary'] + mock_waivers.assert_called_once() + + +def test_make_decision_retrieves_waivers_on_failed(mock_results, mock_waivers): + mock_results.return_value = [make_result(outcome='FAILED')] + response = make_decision() + assert 200 == response.status_code + assert '1 of 1 required tests failed' == response.json['summary'] + mock_waivers.assert_called_once() + + +def test_make_decision_retrieves_waivers_omitted_on_passed(mock_results, mock_waivers): + mock_results.return_value = [make_result(outcome='PASSED')] + response = make_decision() + assert 200 == response.status_code + assert 'All required tests passed' == response.json['summary'] + mock_waivers.assert_not_called() + + +def test_make_decision_retrieves_waivers_once_on_verbose_and_missing(mock_results, mock_waivers): + response = make_decision(verbose=True) + assert 200 == response.status_code + assert '1 of 1 required test results missing' == response.json['summary'] + mock_waivers.assert_called_once() diff --git a/greenwave/tests/test_policies.py b/greenwave/tests/test_policies.py index 5fa4208..c8256a1 100644 --- a/greenwave/tests/test_policies.py +++ b/greenwave/tests/test_policies.py @@ -21,6 +21,7 @@ from greenwave.policies import ( ) from greenwave.resources import ResultsRetriever from greenwave.safe_yaml import SafeYAMLError +from greenwave.waivers import waive_answers class DummyResultsRetriever(ResultsRetriever): @@ -28,7 +29,7 @@ class DummyResultsRetriever(ResultsRetriever): self, subject_identifier=None, testcase=None, outcome='PASSED', subject_type='koji_build'): super(DummyResultsRetriever, self).__init__( - ignore_results=[], + ignore_ids=[], when='', timeout=0, verify=False, @@ -38,7 +39,7 @@ class DummyResultsRetriever(ResultsRetriever): self.testcase = testcase self.outcome = outcome - def _make_request(self, params): + def _retrieve_data(self, params): if (params.get('item') == self.subject_identifier and ('type' not in params or self.subject_type in params['type'].split(',')) and params.get('testcases') == self.testcase): @@ -74,7 +75,7 @@ def test_summarize_answers(): '1 of 2 required test results missing' -def test_waive_absence_of_result(tmpdir): +def test_decision_with_missing_result(tmpdir): p = tmpdir.join('fedora.yaml') p.write(dedent(""" --- !Policy @@ -89,18 +90,14 @@ def test_waive_absence_of_result(tmpdir): policies = load_policies(tmpdir.strpath) policy = policies[0] + results = DummyResultsRetriever() + subject_identifier = 'some_nevr' + # Ensure that absence of a result is failure. - item, results, waivers = {}, DummyResultsRetriever(), [] - decision = policy.check('fedora-rawhide', item, results, waivers) + decision = policy.check('fedora-rawhide', subject_identifier, results) assert len(decision) == 1 assert isinstance(decision[0], TestResultMissing) - # But also that waiving the absence works. - waivers = [{'testcase': 'sometest', 'waived': True}] - decision = policy.check('fedora-rawhide', item, results, waivers) - assert len(decision) == 1 - assert isinstance(decision[0], RuleSatisfied) - def test_waive_brew_koji_mismatch(tmpdir): """ Ensure that a koji_build waiver can match a brew-build result @@ -123,29 +120,26 @@ def test_waive_brew_koji_mismatch(tmpdir): policies = load_policies(tmpdir.strpath) policy = policies[0] - results = DummyResultsRetriever('some_nevr', 'sometest', 'FAILED', 'brew-build') - waiver = { - u'subject_identifier': u'some_nevr', - u'subject_type': u'koji_build', - u'testcase': u'sometest', - u'waived': True, - } + item = 'some_nevr' + results = DummyResultsRetriever(item, 'sometest', 'FAILED', 'brew-build') - item, waivers = 'some_nevr', [waiver] - decision = policy.check('fedora-rawhide', item, results, []) + decision = policy.check('fedora-rawhide', item, results) assert len(decision) == 1 assert isinstance(decision[0], TestResultFailed) - decision = policy.check('fedora-rawhide', item, results, waivers) + waivers = [{ + 'id': 1, + 'subject_identifier': item, + 'subject_type': 'koji_build', + 'testcase': 'sometest', + 'product_version': 'fedora-rawhide', + 'waived': True, + }] + decision = policy.check('fedora-rawhide', item, results) + decision = waive_answers(decision, waivers) assert len(decision) == 1 assert isinstance(decision[0], RuleSatisfied) - # Also, be sure that negative waivers work. - waivers[0]['waived'] = False - decision = policy.check('fedora-rawhide', item, results, waivers) - assert len(decision) == 1 - assert isinstance(decision[0], TestResultFailed) - def test_waive_bodhi_update(tmpdir): """ Ensure that a koji_build waiver can match a brew-build result @@ -170,27 +164,24 @@ def test_waive_bodhi_update(tmpdir): item = 'some_bodhi_update' results = DummyResultsRetriever(item, 'sometest', 'FAILED', 'bodhi_update') - waivers = [{ - u'subject_identifier': item, - u'subject_type': 'bodhi_update', - u'testcase': 'sometest', - u'waived': True, - }] - decision = policy.check('fedora-rawhide', item, results, []) + decision = policy.check('fedora-rawhide', item, results) assert len(decision) == 1 assert isinstance(decision[0], TestResultFailed) - decision = policy.check('fedora-rawhide', item, results, waivers) + waivers = [{ + 'id': 1, + 'subject_identifier': item, + 'subject_type': 'bodhi_update', + 'testcase': 'sometest', + 'product_version': 'fedora-rawhide', + 'waived': True, + }] + decision = policy.check('fedora-rawhide', item, results) + decision = waive_answers(decision, waivers) assert len(decision) == 1 assert isinstance(decision[0], RuleSatisfied) - # Also, be sure that negative waivers work. - waivers[0]['waived'] = False - decision = policy.check('fedora-rawhide', item, results, waivers) - assert len(decision) == 1 - assert isinstance(decision[0], TestResultFailed) - def test_load_policies(): app = create_app('greenwave.config.TestingConfig') @@ -322,23 +313,21 @@ def test_remote_rule_policy(tmpdir, namespace): policies = load_policies(tmpdir.strpath) policy = policies[0] - waivers = [] - # Ensure that presence of a result is success. results = DummyResultsRetriever(nvr, 'dist.upgradepath') - decision = policy.check('fedora-26', nvr, results, waivers) + decision = policy.check('fedora-26', nvr, results) assert len(decision) == 1 assert isinstance(decision[0], RuleSatisfied) # Ensure that absence of a result is failure. results = DummyResultsRetriever() - decision = policy.check('fedora-26', nvr, results, waivers) + decision = policy.check('fedora-26', nvr, results) assert len(decision) == 1 assert isinstance(decision[0], TestResultMissing) # And that a result with a failure, is a failure. results = DummyResultsRetriever(nvr, 'dist.upgradepath', 'FAILED') - decision = policy.check('fedora-26', nvr, results, waivers) + decision = policy.check('fedora-26', nvr, results) assert len(decision) == 1 assert isinstance(decision[0], TestResultFailed) @@ -383,25 +372,23 @@ def test_remote_rule_policy_redhat_module(tmpdir, namespace): policies = load_policies(tmpdir.strpath) policy = policies[0] - waivers = [] - # Ensure that presence of a result is success. results = DummyResultsRetriever(nvr, 'baseos-ci.redhat-module.tier0.functional', subject_type='redhat-module') - decision = policy.check('rhel-8', nvr, results, waivers) + decision = policy.check('rhel-8', nvr, results) assert len(decision) == 1 assert isinstance(decision[0], RuleSatisfied) # Ensure that absence of a result is failure. results = DummyResultsRetriever(subject_type='redhat-module') - decision = policy.check('rhel-8', nvr, results, waivers) + decision = policy.check('rhel-8', nvr, results) assert len(decision) == 1 assert isinstance(decision[0], TestResultMissing) # And that a result with a failure, is a failure. results = DummyResultsRetriever(nvr, 'baseos-ci.redhat-module.tier0.functional', 'FAILED', subject_type='redhat-module') - decision = policy.check('rhel-8', nvr, results, waivers) + decision = policy.check('rhel-8', nvr, results) assert len(decision) == 1 assert isinstance(decision[0], TestResultFailed) @@ -438,9 +425,9 @@ def test_remote_rule_policy_optional_id(tmpdir): policies = load_policies(tmpdir.strpath) policy = policies[0] - results, waivers = [], [] + results = DummyResultsRetriever() expected_details = "Policy 'untitled': Attribute 'product_versions' is required" - decision = policy.check('fedora-26', nvr, results, waivers) + decision = policy.check('fedora-26', nvr, results) assert len(decision) == 1 assert isinstance(decision[0], InvalidGatingYaml) assert decision[0].is_satisfied is False @@ -494,8 +481,8 @@ def test_remote_rule_malformed_yaml(tmpdir): policies = load_policies(tmpdir.strpath) policy = policies[0] - results, waivers = [], [] - decision = policy.check('fedora-26', nvr, results, waivers) + results = DummyResultsRetriever() + decision = policy.check('fedora-26', nvr, results) assert len(decision) == 1 assert isinstance(decision[0], InvalidGatingYaml) assert decision[0].is_satisfied is False @@ -549,16 +536,19 @@ def test_remote_rule_malformed_yaml_with_waiver(tmpdir): policies = load_policies(tmpdir.strpath) policy = policies[0] - results = [] + results = DummyResultsRetriever() waivers = [{ + 'id': 1, 'subject_type': 'koji_build', 'subject_identifier': 'nethack-1.2.3-1.el9000', 'subject': {'type': 'koji_build', 'item': 'nethack-1.2.3-1.el9000'}, 'testcase': 'invalid-gating-yaml', 'product_version': 'fedora-26', - 'comment': 'Waiving the invalig gating.yaml file' + 'comment': 'Waiving the invalid gating.yaml file', + 'waived': True, }] - decision = policy.check('fedora-26', nvr, results, waivers) + decision = policy.check('fedora-26', nvr, results) + decision = waive_answers(decision, waivers) assert len(decision) == 0 @@ -635,9 +625,8 @@ def test_policy_with_arbitrary_subject_type(tmpdir): policies = load_policies(tmpdir.strpath) policy = policies[0] - waivers = [] results = DummyResultsRetriever('nethack-1.2.3-1.el9000', 'sometest', 'PASSED', 'kind-of-magic') - decision = policy.check('rhel-9000', 'nethack-1.2.3-1.el9000', results, waivers) + decision = policy.check('rhel-9000', 'nethack-1.2.3-1.el9000', results) assert len(decision) == 1 assert isinstance(decision[0], TestResultPassed) @@ -664,9 +653,8 @@ def test_policy_with_packages_whitelist(tmpdir, package, num_decisions): policies = load_policies(tmpdir.strpath) policy = policies[0] - waivers = [] results = DummyResultsRetriever('nethack-1.2.3-1.el9000', 'sometest', 'PASSED', 'koji_build') - decision = policy.check('rhel-9000', 'nethack-1.2.3-1.el9000', results, waivers) + decision = policy.check('rhel-9000', 'nethack-1.2.3-1.el9000', results) assert len(decision) == num_decisions if num_decisions: assert isinstance(decision[0], TestResultPassed) @@ -861,8 +849,7 @@ def test_policy_with_subject_type_component_version(tmpdir): policy = policies[0] results = DummyResultsRetriever(nv, 'test_for_new_type', 'PASSED', 'component-version') - waivers = [] - decision = policy.check('fedora-29', nv, results, waivers) + decision = policy.check('fedora-29', nv, results) assert len(decision) == 1 assert isinstance(decision[0], RuleSatisfied) @@ -885,8 +872,7 @@ def test_policy_with_subject_type_redhat_module(tmpdir): policy = policies[0] results = DummyResultsRetriever(nsvc, 'test_for_redhat_module_type', 'PASSED', 'redhat-module') - waivers = [] - decision = policy.check('fedora-29', nsvc, results, waivers) + decision = policy.check('fedora-29', nsvc, results) assert len(decision) == 1 assert isinstance(decision[0], RuleSatisfied) @@ -927,22 +913,21 @@ def test_remote_rule_policy_on_demand_policy(namespace): with mock.patch('greenwave.resources.retrieve_yaml_remote_rule') as f: f.return_value = remote_fragment policy = OnDemandPolicy.create_from_json(serverside_json) # pylint: disable=W0212 - waivers = [] # Ensure that presence of a result is success. results = DummyResultsRetriever(nvr, 'dist.upgradepath') - decision = policy.check('fedora-26', nvr, results, waivers) + decision = policy.check('fedora-26', nvr, results) assert len(decision) == 1 assert isinstance(decision[0], RuleSatisfied) # Ensure that absence of a result is failure. results = DummyResultsRetriever() - decision = policy.check('fedora-26', nvr, results, waivers) + decision = policy.check('fedora-26', nvr, results) assert len(decision) == 1 assert isinstance(decision[0], TestResultMissing) # And that a result with a failure, is a failure. results = DummyResultsRetriever(nvr, 'dist.upgradepath', 'FAILED') - decision = policy.check('fedora-26', nvr, results, waivers) + decision = policy.check('fedora-26', nvr, results) assert len(decision) == 1 assert isinstance(decision[0], TestResultFailed) diff --git a/greenwave/tests/test_waive.py b/greenwave/tests/test_waive.py new file mode 100644 index 0000000..036ab6a --- /dev/null +++ b/greenwave/tests/test_waive.py @@ -0,0 +1,98 @@ +# SPDX-License-Identifier: GPL-2.0+ + +from greenwave.policies import ( + InvalidGatingYaml, + TestResultMissing, + TestResultFailed, +) +from greenwave.waivers import waive_answers + + +def test_waive_failed_result(): + answers = [ + TestResultFailed( + subject_type='koji-build', + subject_identifier='nethack-1.2.3-1.rawhide', + test_case_name='test1', + scenario='scenario1', + result_id=99, + ) + ] + + waived = waive_answers(answers, []) + assert answers == waived + + waivers = [ + dict( + subject_type='koji-build', + subject_identifier='nethack-1.2.3-1.rawhide', + product_version='rawhide', + testcase='test1', + ) + ] + waived = waive_answers(answers, waivers) + expected_json = dict( + type='test-result-passed', + testcase='test1', + result_id=99, + ) + assert 1 == len(waived) + assert expected_json == waived[0].to_json() + + +def test_waive_missing_result(): + answers = [ + TestResultMissing( + subject_type='koji-build', + subject_identifier='nethack-1.2.3-1.rawhide', + test_case_name='test1', + scenario='scenario1', + ) + ] + + waived = waive_answers(answers, []) + assert answers == waived + + waivers = [ + dict( + subject_type='koji-build', + subject_identifier='nethack-1.2.3-1.rawhide', + product_version='rawhide', + testcase='test1', + ) + ] + waived = waive_answers(answers, waivers) + expected_json = dict( + type='test-result-missing-waived', + testcase='test1', + subject_type='koji-build', + subject_identifier='nethack-1.2.3-1.rawhide', + scenario='scenario1', + ) + assert 1 == len(waived) + assert expected_json == waived[0].to_json() + + +def test_waive_invalid_gatin_yaml(): + answers = [ + InvalidGatingYaml( + subject_type='koji-build', + subject_identifier='nethack-1.2.3-1.rawhide', + test_case_name='invalid-gating-yaml', + details='', + ) + ] + + waived = waive_answers(answers, []) + assert answers == waived + + waivers = [ + dict( + subject_type='koji-build', + subject_identifier='nethack-1.2.3-1.rawhide', + product_version='rawhide', + testcase='invalid-gating-yaml', + ) + ] + waived = waive_answers(answers, waivers) + assert [] == waived diff --git a/greenwave/tests/test_waivers_retriever.py b/greenwave/tests/test_waivers_retriever.py new file mode 100644 index 0000000..4b201cb --- /dev/null +++ b/greenwave/tests/test_waivers_retriever.py @@ -0,0 +1,62 @@ +# SPDX-License-Identifier: GPL-2.0+ + +import mock + +from greenwave.resources import WaiversRetriever + +_DUMMY_RETRIEVER_ARGUMENTS = dict( + ignore_ids=[], + when=None, + timeout=None, + verify=None, + url=None, +) + +_DUMMY_FILTERS = ['dummy_filter'] + + +def test_waivers_retriever_retrieves_not_ignored_ids(): + retriever = WaiversRetriever(**_DUMMY_RETRIEVER_ARGUMENTS) + retriever.ignore_ids = [100] + waiver = dict( + id=99, + subject_type='koji-build', + subject_identifier='nethack-1.2.3-1.rawhide', + product_version='rawhide', + testcase='test1', + waived=True, + ) + retriever._retrieve_data = mock.MagicMock(return_value=[waiver]) + waivers = retriever.retrieve(_DUMMY_FILTERS) + assert [waiver] == waivers + + +def test_waivers_retriever_ignores_ids(): + retriever = WaiversRetriever(**_DUMMY_RETRIEVER_ARGUMENTS) + retriever.ignore_ids = [99] + waiver = dict( + id=99, + subject_type='koji-build', + subject_identifier='nethack-1.2.3-1.rawhide', + product_version='rawhide', + testcase='test1', + waived=True, + ) + retriever._retrieve_data = mock.MagicMock(return_value=[waiver]) + waivers = retriever.retrieve(_DUMMY_FILTERS) + assert [] == waivers + + +def test_waivers_retriever_ignores_no_waived(): + retriever = WaiversRetriever(**_DUMMY_RETRIEVER_ARGUMENTS) + waiver = dict( + id=99, + subject_type='koji-build', + subject_identifier='nethack-1.2.3-1.rawhide', + product_version='rawhide', + testcase='test1', + waived=False, + ) + retriever._retrieve_data = mock.MagicMock(return_value=[waiver]) + waivers = retriever.retrieve(_DUMMY_FILTERS) + assert [] == waivers diff --git a/greenwave/waivers.py b/greenwave/waivers.py new file mode 100644 index 0000000..4e43590 --- /dev/null +++ b/greenwave/waivers.py @@ -0,0 +1,32 @@ +# SPDX-License-Identifier: GPL-2.0+ + + +def _is_waived(answer, waivers): + """ + Returns true only if there is a matching waiver for given answer. + """ + return any( + waiver['subject_type'] == answer.subject_type and + waiver['subject_identifier'] == answer.subject_identifier and + waiver['testcase'] == answer.test_case_name + for waiver in waivers) + + +def _maybe_waive(answer, waivers): + """ + Returns waived answer if it's unsatisfied there is a matching waiver, + otherwise returns unchanged answer. + """ + if not answer.is_satisfied and _is_waived(answer, waivers): + return answer.to_waived() + return answer + + +def waive_answers(answers, waivers): + """ + Returns answers with unsatisfied answers waived + (`RuleNotSatisfied.to_waived()`) if there is a matching waiver. + """ + waived_answers = [_maybe_waive(answer, waivers) for answer in answers] + waived_answers = [answer for answer in waived_answers if answer is not None] + return waived_answers