From f4a4118c5f5a9642d9cd8cc5b9ec282f4a31dc42 Mon Sep 17 00:00:00 2001 From: Dan Callaghan Date: Jun 19 2017 00:58:30 +0000 Subject: refactor to make policies, rules, and answers their own types --- diff --git a/functional-tests/test_api_v1.py b/functional-tests/test_api_v1.py index ffc87da..7dc3ffd 100644 --- a/functional-tests/test_api_v1.py +++ b/functional-tests/test_api_v1.py @@ -130,8 +130,7 @@ def test_make_a_decison_on_failed_result(requests_session, greenwave_server, tes res_data = r.json() assert res_data['policies_satisified'] is False assert res_data['applicable_policies'] == ['1'] - # XXX actually 1 failed and 4 are missing, need to improve this summary - expected_summary = '{}: 5 of 5 required tests failed, the policy 1 is not satisfied'.format(nvr) + expected_summary = '{}: 1 of 5 required tests failed, the policy 1 is not satisfied'.format(nvr) assert res_data['summary'] == expected_summary expected_unsatisfied_requirements = [ { diff --git a/greenwave/api_v1.py b/greenwave/api_v1.py index b0f369b..9f787e7 100644 --- a/greenwave/api_v1.py +++ b/greenwave/api_v1.py @@ -3,7 +3,7 @@ import requests from flask import Blueprint, request, current_app, jsonify from werkzeug.exceptions import BadRequest, NotFound, UnsupportedMediaType -from greenwave.policies import policies +from greenwave.policies import policies, summarize_answers api = (Blueprint('api_v1', __name__)) @@ -40,73 +40,46 @@ def make_decision(): raise BadRequest('Invalid subject, must be a list of items') product_version = request.get_json()['product_version'] decision_context = request.get_json()['decision_context'] - applicable_policies = {} - for policy_id, policy in policies.items(): - if product_version == policy['product_version'] and \ - decision_context == policy['decision_context']: - applicable_policies[policy_id] = policy + applicable_policies = [policy for policy in policies + if policy.product_version == product_version and + policy.decision_context == decision_context] if not applicable_policies: raise NotFound('Cannot find any applicable policies for %s' % product_version) subjects = [item.strip() for item in request.get_json()['subject'] if item] policies_satisified = True - summary = [] + summary_lines = [] unsatisfied_requirements = [] timeout = current_app.config['REQUESTS_TIMEOUT'] - for policy_id, policy in applicable_policies.items(): + for policy in applicable_policies: for item in subjects: - url = '{0}/results?item={1}&testcases={2}'.format( - current_app.config['RESULTSDB_API_URL'], item, ','.join(policy['rules'])) - res = requests_session.get(url, timeout=timeout) - res.raise_for_status() - results = res.json()['data'] - total_failed_results = 0 + # XXX make this more efficient than just fetching everything + response = requests_session.get( + current_app.config['RESULTSDB_API_URL'] + '/results', + params={'item': item}, timeout=timeout) + response.raise_for_status() + results = response.json()['data'] if results: - for result in results: - if result['outcome'] not in ('PASSED', 'INFO'): - # query WaiverDB to check whether the result has a waiver - url = '{0}/waivers/?product_version={1}&result_id={2}'.format( - current_app.config['WAIVERDB_API_URL'], product_version, result['id']) - res = requests_session.get(url, timeout=timeout) - res.raise_for_status() - waiver = res.json()['data'] - if not waiver or not waiver[0]['waived']: - policies_satisified = False - total_failed_results += 1 - unsatisfied_requirements.append({ - 'type': 'test-result-failed', - 'item': item, - 'testcase': result['testcase']['name'], - 'result_id': result['id']}) - # find missing results - rules_applied = [result['testcase']['name'] for result in results] - for rule in policy['rules']: - if rule not in rules_applied: - total_failed_results += 1 - unsatisfied_requirements.append({ - 'type': 'test-result-missing', - 'item': item, - 'testcase': rule}) - if total_failed_results: - summary.append( - '{0}: {1} of {2} required tests failed, the policy {3} is not satisfied' - .format(item, total_failed_results, len(policy['rules']), - policy_id)) - else: - summary.append( - '%s: policy %s is satisfied as all required tests are passing' % ( - item, policy_id)) + response = requests_session.get( + current_app.config['WAIVERDB_API_URL'] + '/waivers/', + params={'product_version': product_version, + 'result_id': ','.join(str(result['id']) for result in results)}, + timeout=timeout) + response.raise_for_status() + waivers = response.json()['data'] else: + waivers = [] + + answers = policy.check(item, results, waivers) + if not all(answer.is_satisfied for answer in answers): policies_satisified = False - summary.append('%s: no test results found' % item) - for rule in policy['rules']: - unsatisfied_requirements.append({ - 'type': 'test-result-missing', - 'item': item, - 'testcase': rule}) + summary_lines.append('{}: {}'.format(item, summarize_answers(answers, policy.id))) + unsatisfied_requirements.extend(answer for answer in answers + if not answer.is_satisfied) + res = { 'policies_satisified': policies_satisified, - 'summary': '\n'.join(summary), - 'applicable_policies': list(applicable_policies.keys()), - 'unsatisfied_requirements': unsatisfied_requirements + 'summary': '\n'.join(summary_lines), + 'applicable_policies': [policy.id for policy in applicable_policies], + 'unsatisfied_requirements': [a.to_json() for a in unsatisfied_requirements], } return jsonify(res), 200 diff --git a/greenwave/policies.py b/greenwave/policies.py index 45f4a18..7efbc53 100644 --- a/greenwave/policies.py +++ b/greenwave/policies.py @@ -1,18 +1,179 @@ # SPDX-License-Identifier: GPL-2.0+ -policies = { + +class Answer(object): + """ + Represents the result of evaluating a policy rule against a particular + item. But we call it an "answer" because the word "result" is a bit + overloaded in here. :-) + + This base class is not used directly -- each answer is an instance of + a subclass, depending on what the answer was. + """ + + pass + + +class RuleSatisfied(Answer): + """ + The rule's requirements are satisfied for this item. + """ + + is_satisfied = True + + +class RuleNotSatisfied(Answer): + """ + The rule's requirements are not satisfied for this item. + + Not used directly -- the answer is an instance of a subclass, specifying + exactly what was not satisfied. + """ + + is_satisfied = False + + def to_json(self): + """ + Returns a machine-readable description of the problem for API responses. + """ + raise NotImplementedError() + + +class TestResultMissing(RuleNotSatisfied): + """ + A required test case is missing (that is, we did not find any result in + ResultsDB with a matching item and test case name). + """ + + def __init__(self, item, test_case_name): + self.item = item + self.test_case_name = test_case_name + + def to_json(self): + return { + 'type': 'test-result-missing', + 'item': self.item, + 'testcase': self.test_case_name, + } + + +class TestResultFailed(RuleNotSatisfied): + """ + A required test case did not pass (that is, its outcome in ResultsDB was + not ``PASSED`` or ``INFO``) and no corresponding waiver was found. + """ + + def __init__(self, item, test_case_name, result_id): + self.item = item + self.test_case_name = test_case_name + self.result_id = result_id + + def to_json(self): + return { + 'type': 'test-result-failed', + 'item': self.item, + 'testcase': self.test_case_name, + 'result_id': self.result_id, + } + + +def summarize_answers(answers, policy_id): + """ + Produces a one-sentence human-readable summary of the result of evaluating a policy. + + Args: + answers (list): List of :py:class:`Answers ` from evaluating a policy. + + Returns: + str: Human-readable summary. + """ + if all(answer.is_satisfied for answer in answers): + return 'policy {} is satisfied as all required tests are passing'.format(policy_id) + failure_count = len([answer for answer in answers if isinstance(answer, TestResultFailed)]) + if failure_count: + return ('{} of {} required tests failed, the policy {} is not satisfied'.format( + failure_count, len(answers), policy_id)) + if all(isinstance(answer, TestResultMissing) for answer in answers): + return 'no test results found' + # XXX need to handle some missing but others passing + return 'inexplicable result' + + +class Rule(object): + """ + An individual rule within a policy. A policy consists of multiple rules. + When the policy is evaluated, each rule returns an answer + (instance of :py:class:`Answer`). + + This base class is not used directly. + """ + + def check(self, item, results, waivers): + """ + Evaluate this policy rule for the given item. + + Args: + item (str): The item we are evaluating ('item' key in ResultsDB, + for example a build NVR). + results (list): List of result objects looked up in ResultsDB for this item. + 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. + """ + raise NotImplementedError() + + +class PassingTestCaseRule(Rule): + """ + This rule requires either a passing result for the given test case, or + a non-passing result with a waiver. + """ + + def __init__(self, test_case_name): + self.test_case_name = test_case_name + + def check(self, item, results, waivers): + matching_results = [r for r in results if r['testcase']['name'] == self.test_case_name] + if not matching_results: + return TestResultMissing(item, self.test_case_name) + # XXX need to handle multiple results (take the latest) + matching_result = matching_results[0] + if matching_result['outcome'] in ['PASSED', 'INFO']: + return RuleSatisfied() + # XXX limit who is allowed to waive + if any(w['result_id'] == matching_result['id'] and w['waived'] for w in waivers): + return RuleSatisfied() + return TestResultFailed(item, self.test_case_name, matching_result['id']) + + +class Policy(object): + + def __init__(self, id, product_version, decision_context, rules): + self.id = id + self.product_version = product_version + self.decision_context = decision_context + self.rules = rules + + def check(self, item, results, waivers): + return [rule.check(item, results, waivers) for rule in self.rules] + + +policies = [ # Mimic the default Errata rule used for RHEL-7 https://errata.devel.redhat.com/workflow_rules/1 # In Errata, in order to transition to QE state, an advisory must complete rpmdiff test. # A completed rpmdiff test could be some dist.rpmdiff.* testcases in ResultsDB and all the # tests need to be passed. - '1': { - 'product_version': 'rhel-7', - 'decision_context': 'errata_newfile_to_qe', - 'rules': [ - 'dist.rpmdiff.comparison.xml_validity', - 'dist.rpmdiff.comparison.virus_scan', - 'dist.rpmdiff.comparison.upstream_source', - 'dist.rpmdiff.comparison.symlinks', - 'dist.rpmdiff.comparison.binary_stripping'] - } -} + Policy( + id='1', + product_version='rhel-7', + decision_context='errata_newfile_to_qe', + rules=[ + PassingTestCaseRule('dist.rpmdiff.comparison.xml_validity'), + PassingTestCaseRule('dist.rpmdiff.comparison.virus_scan'), + PassingTestCaseRule('dist.rpmdiff.comparison.upstream_source'), + PassingTestCaseRule('dist.rpmdiff.comparison.symlinks'), + PassingTestCaseRule('dist.rpmdiff.comparison.binary_stripping'), + ], + ), +] diff --git a/greenwave/tests/test_policies.py b/greenwave/tests/test_policies.py index fe9e18d..d997c18 100644 --- a/greenwave/tests/test_policies.py +++ b/greenwave/tests/test_policies.py @@ -1,7 +1,19 @@ # SPDX-License-Identifier: GPL-2.0+ +from greenwave.policies import summarize_answers, RuleSatisfied, TestResultMissing, TestResultFailed -# This is just a placeholder for where unit tests could go. -def test_it(): - pass + +def test_summarize_answers(): + assert summarize_answers([RuleSatisfied()], '1') == \ + 'policy 1 is satisfied as all required tests are passing' + assert summarize_answers([TestResultFailed('item', 'test', 'id'), RuleSatisfied()], '1') == \ + '1 of 2 required tests failed, the policy 1 is not satisfied' + assert summarize_answers([TestResultMissing('item', 'test')], '1') == \ + 'no test results found' + assert summarize_answers([TestResultMissing('item', 'test'), + TestResultFailed('item', 'test', 'id')], '1') == \ + '1 of 2 required tests failed, the policy 1 is not satisfied' + # XXX fix this one + assert summarize_answers([TestResultMissing('item', 'test'), RuleSatisfied()], '1') == \ + 'inexplicable result' diff --git a/pytest.ini b/pytest.ini new file mode 100644 index 0000000..ac67151 --- /dev/null +++ b/pytest.ini @@ -0,0 +1,8 @@ +[pytest] +# By default pytest collects any class with Test in its name, +# which means it picks up greenwave.policies.TestResultFailed etc +# as test classes (even though they're not) and then complains +# that they have a constructor. +# Disable the name-based class collection entirely +# (we can still inherit from unittest.TestCase if necessary). +python_classes =