From 9a7cd287ab19ae0a75a7800ab5484053c8f7792e Mon Sep 17 00:00:00 2001 From: Lukas Holecek Date: Apr 06 2022 15:31:42 +0000 Subject: Fix showing correct scenario for failed results Also adds other important values for distinguishing results. --- diff --git a/docs/decision_requirements.rst b/docs/decision_requirements.rst index d3b8450..15a473c 100644 --- a/docs/decision_requirements.rst +++ b/docs/decision_requirements.rst @@ -26,6 +26,10 @@ requirement. Unsatisfied requirements containing ``testcase`` property can be waived (using this value in a new waiver). +Requirements related to an existing result contain ``result_id`` attribute that +refers to the result ID in ResultsDB, and also ``scenario``, +``system_architecture`` and ``system_variant`` from the result data. + See :ref:`decision_requirements_examples` to get an idea about the data of various requirements. @@ -76,6 +80,22 @@ latest outcome is ``QUEUED`` or ``RUNNING`` (this can be overridden by "scenario": null } +Unsatisfied requirement for an incomplete result would be indicated by +additional attributes from the queued/running result data: + +.. code-block:: json + + { + "type": "test-result-missing", + "testcase": "example.test.case", + "subject_type": "koji-build", + "subject_identifier": "nethack-1.2.3-1.rawhide", + "result_id": 1004, + "scenario": null, + "system_architecture": null, + "system_variant": null + } + .. _failed_test_result: Failed test result diff --git a/functional-tests/consumers/test_resultsdb.py b/functional-tests/consumers/test_resultsdb.py index b42688c..939f904 100644 --- a/functional-tests/consumers/test_resultsdb.py +++ b/functional-tests/consumers/test_resultsdb.py @@ -59,6 +59,9 @@ def test_consume_new_result( 'subject_identifier': nvr, 'result_id': result['id'], 'testcase': 'dist.rpmdeplint', + 'scenario': None, + 'system_architecture': None, + 'system_variant': None, 'source': None, 'type': 'test-result-passed', }, @@ -138,6 +141,9 @@ def test_consume_new_result( 'subject_identifier': nvr, 'result_id': result['id'], 'testcase': 'dist.rpmdeplint', + 'scenario': None, + 'system_architecture': None, + 'system_variant': None, 'type': 'test-result-passed', 'source': None, }, @@ -257,6 +263,9 @@ def test_consume_compose_id_result( 'subject_type': 'compose', 'subject_identifier': compose_id, 'result_id': result['id'], + 'scenario': 'scenario1', + 'system_architecture': None, + 'system_variant': None, 'testcase': 'compose.install_no_user', 'source': None, 'type': 'test-result-passed' @@ -329,6 +338,9 @@ def test_consume_legacy_result( 'subject_type': 'koji_build', 'subject_identifier': nvr, 'result_id': result['id'], + 'scenario': None, + 'system_architecture': None, + 'system_variant': None, 'testcase': 'dist.rpmdeplint', 'source': None, 'type': 'test-result-passed' @@ -391,6 +403,9 @@ def test_consume_legacy_result( 'subject_type': 'koji_build', 'subject_identifier': nvr, 'result_id': result['id'], + 'scenario': None, + 'system_architecture': None, + 'system_variant': None, 'testcase': 'dist.rpmdeplint', 'source': None, 'type': 'test-result-passed' @@ -596,6 +611,9 @@ def test_consume_new_result_container_image( 'subject_type': 'container-image', 'subject_identifier': item_hash, 'result_id': result['id'], + 'scenario': None, + 'system_architecture': None, + 'system_variant': None, 'testcase': 'baseos-qe.baseos-ci.tier1.functional', 'source': None, 'type': 'test-result-passed' diff --git a/functional-tests/consumers/test_waiverdb.py b/functional-tests/consumers/test_waiverdb.py index f6be801..7fad9c0 100644 --- a/functional-tests/consumers/test_waiverdb.py +++ b/functional-tests/consumers/test_waiverdb.py @@ -80,6 +80,9 @@ def test_consume_new_waiver( 'subject_type': 'koji_build', 'subject_identifier': nvr, 'result_id': results[0]['id'], + 'scenario': None, + 'system_architecture': None, + 'system_variant': None, 'testcase': passing_tests[0], 'source': None, 'type': 'test-result-passed' @@ -88,6 +91,9 @@ def test_consume_new_waiver( 'subject_type': 'koji_build', 'subject_identifier': nvr, 'result_id': results[1]['id'], + 'scenario': None, + 'system_architecture': None, + 'system_variant': None, 'testcase': passing_tests[1], 'source': None, 'type': 'test-result-passed' @@ -97,9 +103,13 @@ def test_consume_new_waiver( { 'result_id': result['id'], 'item': {'item': nvr, 'type': 'koji_build'}, + 'subject_identifier': nvr, + 'subject_type': 'koji_build', 'testcase': failing_test, 'type': 'test-result-failed', 'scenario': None, + 'system_architecture': None, + 'system_variant': None, 'source': None, }, ], @@ -118,12 +128,17 @@ def test_consume_new_waiver( 'testcase': failing_test, 'type': 'test-result-failed-waived', 'scenario': None, + 'system_architecture': None, + 'system_variant': None, 'source': None, }, { 'subject_type': 'koji_build', 'subject_identifier': nvr, 'result_id': results[0]['id'], + 'scenario': None, + 'system_architecture': None, + 'system_variant': None, 'testcase': passing_tests[0], 'source': None, 'type': 'test-result-passed' @@ -132,6 +147,9 @@ def test_consume_new_waiver( 'subject_type': 'koji_build', 'subject_identifier': nvr, 'result_id': results[1]['id'], + 'scenario': None, + 'system_architecture': None, + 'system_variant': None, 'testcase': passing_tests[1], 'source': None, 'type': 'test-result-passed' diff --git a/functional-tests/test_api_v1.py b/functional-tests/test_api_v1.py index a30de3c..c114682 100644 --- a/functional-tests/test_api_v1.py +++ b/functional-tests/test_api_v1.py @@ -261,6 +261,9 @@ def test_make_a_decision_with_verbose_flag(requests_session, greenwave_server, t 'subject_type': 'koji_build', 'subject_identifier': nvr, 'source': None, + 'scenario': None, + 'system_architecture': None, + 'system_variant': None, } for result in results ] assert res_data['satisfied_requirements'] == expected_satisfied_requirements @@ -380,9 +383,13 @@ def test_make_a_decision_on_failed_result(requests_session, greenwave_server, te expected_unsatisfied_requirements = [ { 'item': {'item': nvr, 'type': 'koji_build'}, + 'subject_type': 'koji_build', + 'subject_identifier': nvr, 'result_id': result['id'], 'testcase': TASKTRON_RELEASE_CRITICAL_TASKS[0], 'scenario': None, + 'system_architecture': None, + 'system_variant': None, 'source': None, 'type': 'test-result-failed' }, @@ -425,7 +432,10 @@ def test_make_a_decision_on_queued_result(requests_session, greenwave_server, te 'subject_identifier': result['data']['item'][0], 'subject_type': result['data']['type'][0], 'testcase': TASKTRON_RELEASE_CRITICAL_TASKS[0], + 'result_id': result['id'], 'scenario': None, + 'system_architecture': None, + 'system_variant': None, 'source': None, 'type': 'test-result-missing' }, @@ -468,7 +478,10 @@ def test_make_a_decision_on_running_result(requests_session, greenwave_server, t 'subject_identifier': result['data']['item'][0], 'subject_type': result['data']['type'][0], 'testcase': TASKTRON_RELEASE_CRITICAL_TASKS[0], + 'result_id': result['id'], 'scenario': None, + 'system_architecture': None, + 'system_variant': None, 'source': None, 'type': 'test-result-missing' }, @@ -550,18 +563,26 @@ def test_make_a_decision_on_redhat_cont_image(requests_session, greenwave_server expected_unsatisfied_requirements = [ { 'item': {'item': item1_nvr, 'type': 'redhat-container-image'}, + 'subject_identifier': result1['data']['item'][0], + 'subject_type': 'redhat-container-image', 'result_id': result1['id'], 'testcase': 'test.testcase1', 'scenario': None, + 'system_architecture': None, + 'system_variant': None, 'source': None, 'type': 'test-result-failed' }, { 'item': {'item': item1_nvr, 'type': 'redhat-container-image'}, + 'subject_identifier': result2['data']['item'][0], + 'subject_type': 'redhat-container-image', 'result_id': result2['id'], 'testcase': 'test.testcase2', 'type': 'test-result-failed', 'scenario': None, + 'system_architecture': None, + 'system_variant': None, 'source': None, } ] @@ -693,10 +714,14 @@ def test_multiple_results_in_a_subject( expected_unsatisfied_requirements = [ { 'item': {'item': nvr, 'type': 'koji_build'}, + 'subject_identifier': nvr, + 'subject_type': 'koji_build', 'result_id': result['id'], 'testcase': 'dist.abicheck', 'type': 'test-result-failed', 'scenario': None, + 'system_architecture': None, + 'system_variant': None, 'source': None, }, ] @@ -825,10 +850,14 @@ def test_make_a_decision_on_failing_result_with_scenario( assert res_data['summary'] == expected_summary expected_unsatisfied_requirements = [{ 'item': {'productmd.compose.id': compose_id}, + 'subject_identifier': compose_id, + 'subject_type': 'compose', 'result_id': results[-1]['id'], 'testcase': testcase_name, 'type': 'test-result-failed', 'scenario': 'scenario2', + 'system_architecture': None, + 'system_variant': None, 'source': None, }] assert res_data['unsatisfied_requirements'] == expected_unsatisfied_requirements @@ -871,10 +900,14 @@ def test_ignore_waiver(requests_session, greenwave_server, testdatabuilder): expected_unsatisfied_requirements = [ { 'item': {'item': nvr, 'type': 'koji_build'}, + 'subject_identifier': nvr, + 'subject_type': 'koji_build', 'result_id': result['id'], 'testcase': TASKTRON_RELEASE_CRITICAL_TASKS[0], 'type': 'test-result-failed', 'scenario': None, + 'system_architecture': None, + 'system_variant': None, 'source': None, }, ] @@ -1148,9 +1181,13 @@ def test_make_a_decision_about_compose_all_variants_architectures( assert not res_data['policies_satisfied'] assert res_data['unsatisfied_requirements'] == [{ 'item': {'productmd.compose.id': compose_id}, + 'subject_identifier': compose_id, + 'subject_type': 'compose', 'result_id': failed_results['id'], 'scenario': None, 'source': None, + 'system_architecture': variant1['architecture'], + 'system_variant': variant1['variant'], 'testcase': 'rtt.acceptance.validation', 'type': 'test-result-failed' }] @@ -1676,6 +1713,9 @@ def test_make_a_decision_on_passed_result_with_custom_scenario( 'subject_type': 'koji_build', 'result_id': result2['id'], 'testcase': 'test.testcase1', + 'scenario': 'scenario1', + 'system_architecture': None, + 'system_variant': None, 'source': None, 'type': 'test-result-passed' } diff --git a/greenwave/config.py b/greenwave/config.py index 8de0140..61504ef 100644 --- a/greenwave/config.py +++ b/greenwave/config.py @@ -56,6 +56,12 @@ class Config(object): OUTCOMES_ERROR = ('ERROR',) OUTCOMES_INCOMPLETE = ('QUEUED', 'RUNNING') + DISTINCT_LATEST_RESULTS_ON = ( + 'scenario', + 'system_architecture', + 'system_variant', + ) + class ProductionConfig(Config): DEBUG = False diff --git a/greenwave/policies.py b/greenwave/policies.py index 892a2fa..3cd6936 100644 --- a/greenwave/policies.py +++ b/greenwave/policies.py @@ -173,6 +173,43 @@ class TestResultMissing(RuleNotSatisfied): return TestResultWaived(self) +class TestResultIncomplete(RuleNotSatisfied): + """ + A required test case is incomplete (that is, we did not find any completed + result outcomes in ResultsDB with a matching item and test case name). + """ + + def __init__(self, subject, test_case_name, source, result_id, data): + self.subject = subject + self.test_case_name = test_case_name + self.source = source + self.result_id = result_id + self.data = data + + @property + def scenario(self): + return self.data.get('scenario') + + def to_json(self): + data = { + # Same type as TestResultMissing for backwards compatibility + 'type': 'test-result-missing', + 'testcase': self.test_case_name, + 'subject_type': self.subject.type, + 'subject_identifier': self.subject.identifier, + 'source': self.source, + 'result_id': self.result_id, + + # For backwards compatibility only: + 'item': self.subject.to_dict() + } + data.update(self.data) + return data + + def to_waived(self): + return TestResultWaived(self) + + class TestResultWaived(RuleSatisfied): """ A waived unsatisfied rule. @@ -187,12 +224,7 @@ class TestResultWaived(RuleSatisfied): satisfied_rule = self.unsatisfied_rule.to_json() satisfied_rule['type'] += '-waived' - item = satisfied_rule.get('item') - if isinstance(item, dict) and 'item' in item and 'type' in item: - if 'subject_identifier' not in satisfied_rule: - satisfied_rule['subject_identifier'] = item['item'] - if 'subject_type' not in satisfied_rule: - satisfied_rule['subject_type'] = item['type'] + if 'item' in satisfied_rule: del satisfied_rule['item'] return satisfied_rule @@ -204,25 +236,33 @@ class TestResultFailed(RuleNotSatisfied): not passing). """ - def __init__(self, subject, test_case_name, scenario, source, result_id): + def __init__(self, subject, test_case_name, source, result_id, data): self.subject = subject self.test_case_name = test_case_name - self.scenario = scenario self.source = source self.result_id = result_id + self.data = data + + @property + def scenario(self): + return self.data.get('scenario') def to_json(self): - return { + data = { 'type': 'test-result-failed', 'testcase': self.test_case_name, + 'subject_type': self.subject.type, + 'subject_identifier': self.subject.identifier, 'source': self.source, 'result_id': self.result_id, + # These are for backwards compatibility only # (the values are already visible in the result data itself, the # caller shouldn't need them repeated here): 'item': self.subject.to_dict(), - 'scenario': self.scenario, } + data.update(self.data) + return data def to_waived(self): return TestResultWaived(self) @@ -239,31 +279,38 @@ class TestResultErrored(RuleNotSatisfied): self, subject, test_case_name, - scenario, source, result_id, + data, error_reason): self.subject = subject self.test_case_name = test_case_name - self.scenario = scenario self.source = source self.result_id = result_id + self.data = data self.error_reason = error_reason + @property + def scenario(self): + return self.data.get('scenario') + def to_json(self): - return { + data = { 'type': 'test-result-errored', 'testcase': self.test_case_name, + 'subject_type': self.subject.type, + 'subject_identifier': self.subject.identifier, + 'source': self.source, 'result_id': self.result_id, 'error_reason': self.error_reason, - 'source': self.source, # These are for backwards compatibility only # (the values are already visible in the result data itself, the # caller shouldn't need them repeated here): 'item': self.subject.to_dict(), - 'scenario': self.scenario, } + data.update(self.data) + return data def to_waived(self): return TestResultWaived(self) @@ -378,21 +425,24 @@ class TestResultPassed(RuleSatisfied): A required test case passed (that is, its outcome in ResultsDB was passing) or a corresponding waiver was found. """ - def __init__(self, subject, test_case_name, source, result_id): + def __init__(self, subject, test_case_name, source, result_id, data): self.subject = subject self.test_case_name = test_case_name self.source = source self.result_id = result_id + self.data = data def to_json(self): - return { + data = { 'type': 'test-result-passed', 'testcase': self.test_case_name, - 'result_id': self.result_id, 'subject_type': self.subject.type, 'subject_identifier': self.subject.identifier, 'source': self.source, + 'result_id': self.result_id, } + data.update(self.data) + return data class BlacklistedInPolicy(RuleSatisfied): @@ -430,8 +480,14 @@ class ExcludedInPolicy(RuleSatisfied): def _summarize_answers_without_errored(answers): - failure_count = len([answer for answer in answers if isinstance(answer, RuleNotSatisfied)]) - missing_count = len([answer for answer in answers if isinstance(answer, TestResultMissing)]) + failure_count = sum( + 1 for answer in answers + if isinstance(answer, RuleNotSatisfied) + ) + missing_count = sum( + 1 for answer in answers + if isinstance(answer, (TestResultIncomplete, TestResultMissing)) + ) # Missing results are also failures but we will distinguish between those # two in summary message. @@ -691,17 +747,23 @@ class PassingTestCaseRule(Rule): def _answer_for_result(self, result, subject, source): outcome = result['outcome'] + additional_keys = current_app.config['DISTINCT_LATEST_RESULTS_ON'] + data = { + key: (result['data'].get(key) or [None])[0] + for key in additional_keys + } + if outcome in current_app.config['OUTCOMES_PASSED']: log.debug('Test result passed for the result_id %s and testcase %s,' ' because the outcome is %s', result['id'], self.test_case_name, outcome) - return TestResultPassed(subject, self.test_case_name, source, result['id']) + return TestResultPassed(subject, self.test_case_name, source, result['id'], data) if outcome in current_app.config['OUTCOMES_INCOMPLETE']: log.debug('Test result MISSING for the %s and ' 'testcase %s, because the outcome is %s', subject, self.test_case_name, outcome) - return TestResultMissing(subject, self.test_case_name, self.scenario, source) + return TestResultIncomplete(subject, self.test_case_name, source, result['id'], data) if outcome in current_app.config['OUTCOMES_ERROR']: error_reason = result.get('error_reason') @@ -709,13 +771,14 @@ class PassingTestCaseRule(Rule): 'testcase %s, because the outcome is %s; error reason: %s', subject, self.test_case_name, outcome, error_reason) return TestResultErrored( - subject, self.test_case_name, self.scenario, source, result['id'], + subject, self.test_case_name, source, result['id'], data, error_reason) log.debug('Test result failed for the %s and ' 'testcase %s, because the outcome is %s and it didn\'t match any of the ' 'previous cases', subject, self.test_case_name, outcome) - return TestResultFailed(subject, self.test_case_name, self.scenario, source, result['id']) + + return TestResultFailed(subject, self.test_case_name, source, result['id'], data) class ObsoleteRule(Rule): diff --git a/greenwave/resources.py b/greenwave/resources.py index 69a1025..0e9265a 100644 --- a/greenwave/resources.py +++ b/greenwave/resources.py @@ -61,6 +61,8 @@ class ResultsRetriever(BaseRetriever): def __init__(self, **args): super().__init__(**args) + self._distinct_on = ','.join( + current_app.config['DISTINCT_LATEST_RESULTS_ON']) self.cache = {} def _retrieve_all(self, subject, testcase=None): @@ -81,7 +83,7 @@ class ResultsRetriever(BaseRetriever): return results params = { - '_distinct_on': 'scenario,system_architecture,system_variant' + '_distinct_on': self._distinct_on } if self.since: params.update({'since': self.since}) diff --git a/greenwave/tests/test_policies.py b/greenwave/tests/test_policies.py index 268911f..be04dc1 100644 --- a/greenwave/tests/test_policies.py +++ b/greenwave/tests/test_policies.py @@ -79,7 +79,7 @@ class DummyResultsRetriever(ResultsRetriever): def test_summarize_answers(): testSubject = create_subject('koji_build', 'nvr') testResultPassed = RuleSatisfied() - testResultFailed = TestResultFailed(testSubject, 'test', None, None, 1) + testResultFailed = TestResultFailed(testSubject, 'test', None, 1, {}) testResultMissing = TestResultMissing(testSubject, 'test', None, None) assert summarize_answers([testResultPassed]) == \ diff --git a/greenwave/tests/test_summary.py b/greenwave/tests/test_summary.py index 6a9c903..a7da751 100644 --- a/greenwave/tests/test_summary.py +++ b/greenwave/tests/test_summary.py @@ -5,6 +5,7 @@ from greenwave.policies import ( TestResultErrored, TestResultFailed, TestResultMissing, + TestResultIncomplete, TestResultWaived, InvalidRemoteRuleYaml, ) @@ -16,9 +17,11 @@ from greenwave.subjects.subject_type import GenericSubjectType testSubject = Subject(GenericSubjectType('koji_build'), 'nethack-1.2.3-1.el9000') testResultPassed = RuleSatisfied() testResultErrored = TestResultErrored( - testSubject, 'test', None, None, 1, 'some error') + testSubject, 'test', None, 1, {}, 'some error') testResultFailed = TestResultFailed( - testSubject, 'test', None, None, 1) + testSubject, 'test', None, 1, {}) +testResultIncomplete = TestResultIncomplete( + testSubject, 'test', None, 1, {}) testResultMissing = TestResultMissing( testSubject, 'test', None, None) testInvalidGatingYaml = InvalidRemoteRuleYaml( @@ -44,6 +47,13 @@ def test_summary_failed(): assert summarize_answers(answers) == '1 of 1 required tests failed' +def test_summary_incomplete(): + answers = [ + testResultIncomplete, + ] + assert summarize_answers(answers) == '1 of 1 required test results missing' + + def test_summary_missing(): answers = [ testResultMissing, diff --git a/greenwave/tests/test_waive.py b/greenwave/tests/test_waive.py index 670c922..04c61a0 100644 --- a/greenwave/tests/test_waive.py +++ b/greenwave/tests/test_waive.py @@ -1,9 +1,10 @@ # SPDX-License-Identifier: GPL-2.0+ - from greenwave.policies import ( InvalidRemoteRuleYaml, - TestResultMissing, + TestResultErrored, TestResultFailed, + TestResultIncomplete, + TestResultMissing, ) from greenwave.subjects.subject import Subject from greenwave.subjects.subject_type import GenericSubjectType @@ -19,9 +20,9 @@ def test_waive_failed_result(): TestResultFailed( subject=test_subject(), test_case_name='test1', - scenario='scenario1', source='https://greenwave_tests.example.com', result_id=99, + data={'scenario': 'scenario1'}, ) ] @@ -84,6 +85,80 @@ def test_waive_missing_result(): assert expected_json == waived[0].to_json() +def test_waive_incomplete_result(): + answers = [ + TestResultIncomplete( + subject=test_subject(), + test_case_name='test1', + source='https://greenwave_tests.example.com', + result_id=99, + data={'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', + result_id=99, + scenario='scenario1', + source='https://greenwave_tests.example.com', + ) + assert 1 == len(waived) + assert expected_json == waived[0].to_json() + + +def test_waive_errored_result(): + answers = [ + TestResultErrored( + subject=test_subject(), + test_case_name='test1', + source='https://greenwave_tests.example.com', + result_id=99, + data={'scenario': 'scenario1'}, + error_reason='Failed', + ) + ] + + 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-errored-waived', + testcase='test1', + subject_type='koji_build', + subject_identifier='nethack-1.2.3-1.rawhide', + result_id=99, + scenario='scenario1', + source='https://greenwave_tests.example.com', + error_reason='Failed', + ) + assert 1 == len(waived) + assert expected_json == waived[0].to_json() + + def test_waive_invalid_gatin_yaml(): answers = [ InvalidRemoteRuleYaml( @@ -114,9 +189,9 @@ def test_waive_scenario(): TestResultFailed( subject=test_subject(), test_case_name='test1', - scenario='scenario1', source='https://greenwave_tests.example.com', result_id=99, + data={'scenario': 'scenario1'}, ) ]