From 406301d40fbfad0d8b7ede885abcedc1f415cdf2 Mon Sep 17 00:00:00 2001 From: Adam Williamson Date: Oct 17 2019 19:40:06 +0000 Subject: Wiki reporting: ignore overall job result for 'modules' condition When we implemented the 'modules' condition for wiki reporting, we used the logic that for a pass to be reported for the test case in question, *both* the overall openQA job must pass *and* the specified modules must pass. But now we have a problem: we implemented the workstation_core_applications test as a sort of bolt-on to the apps_startstop test, and the way we did that, the overall result for the apps_startstop job can be 'failed' but we still want to report a pass for workstation_core_applications (if all the core applications work but some other apps don't). So, this changes that logic so for the 'modules' condition, we consider it satisfied so long as the specified modules passed *regardless* of the overall job result. The other few things that use this condition should be fine with that. Unfortunately the implementation of this gets pretty ugly, but it works and it has tests. I even managed to make it theoretically possible to express "this test case is passed if these modules passed and some other test suite also passed" (combining 'modules' and 'testsuites' conditionals), even though we don't actually use that at present. There is no ability to specify the old behaviour; we can work that in somehow later if we need it, but right now we don't so let's avoid the complexity... Signed-off-by: Adam Williamson --- diff --git a/fedora_openqa/conf_test_suites.py b/fedora_openqa/conf_test_suites.py index 30daf74..d81befe 100644 --- a/fedora_openqa/conf_test_suites.py +++ b/fedora_openqa/conf_test_suites.py @@ -565,21 +565,19 @@ TESTSUITES = { # # The dict may contain a 'modules' key, whose value must be an # iterable. If so, the test case will only be considered 'passed' - # if the overall openQA job passed *and* each of the individual - # openQA test modules named in the iterable were present in the - # openQA job and passed. e.g.: + # if each of the individual openQA test modules named in the + # iterable were present in the openQA job and passed. e.g.: # # "testsuite_c": { # "testcase_c": {"modules": ["module_1"],}, # }, # - # In this case, testcase_c would only be considered 'passed' if - # both testsuite_c passed *and* it contained a test module named - # module_1 and that module passed. This handles test suites like - # realmd_join_cockpit, which has an optional module that covers - # QA:Testcase_FreeIPA_web_ui ; if that module passes we want to - # report a pass for that test case, if the job passes but that - # module fails we do *not* want to report a pass. + # The overall job result is *disregarded* in this case: so long as + # all the listed modules passed, the test case is considered + # passed. This handles test suites like realmd_join_cockpit, which + # has an optional module that covers QA:Testcase_FreeIPA_web_ui ; + # if that module passes we want to report a pass for that test + # case, if that module fails we do *not* want to report a pass. "mediakit_repoclosure": [ "QA:Testcase_Mediakit_Repoclosure", ], @@ -997,7 +995,7 @@ TESTSUITES = { ], # _server is the key test here: it will only pass if client # passes, and the wiki test is not passed unless both pass, so - # need to list anything for _client on its own + # no need to list anything for _client on its own "server_remote_logging_client": [], "realmd_join_cockpit": { "QA:Testcase_realmd_join_cockpit": {}, diff --git a/fedora_openqa/report.py b/fedora_openqa/report.py index 917ed18..e852d2e 100644 --- a/fedora_openqa/report.py +++ b/fedora_openqa/report.py @@ -94,11 +94,15 @@ def _uniqueres_replacements(job, tcdict): return changed -def _get_passed_tcnames(job, composeid, client=None): +def _get_passed_tcnames(job, result, composeid, client=None): """Given a job dict, find the corresponding entry from TESTSUITES and return the test case names that are considered to have passed. This is splitting out a chunk of logic from the middle of - get_passed_testcases to make it shorter and more readable. + get_passed_testcases to make it shorter and more readable. result + is the overall result of the job: usually only pass or softfail + jobs will generate any passed test cases, but one special case of + the 'modules' condition allows even an overall-failed job to + generate passed test cases if a particular module passed. composeid is the compose ID, passed in from get_passed_testcases. client can be an OpenQA_Client instance (to save us instantiating a new one, and also so checkwiki can use a fake one); it's used @@ -133,12 +137,15 @@ def _get_passed_tcnames(job, composeid, client=None): for (testcase, conds) in testsuite.items(): if not conds: # life is easy! - passed.append(testcase) + if result in ('passed', 'softfailed'): + passed.append(testcase) continue # otherwise, handle the conditions... - # modules: the test case is only 'passed' if all listed openQA job modules - # are present in the job and passed + # modules: the test case is 'passed' if all listed openQA job modules + # are present in the job and passed, the overall job result *is not + # considered*. we allow this to be combined with a 'testsuites' + # conditional by altering 'result' here. if 'modules' in conds: tcpass = True for modname in conds['modules']: @@ -155,8 +162,12 @@ def _get_passed_tcnames(job, composeid, client=None): tcpass = False logger.warning("Did not find module %s in job data!", modname) break - if not tcpass: - # skip to next testsuite item + if tcpass: + # overwrite overall job result with 'passed' + result = 'passed' + else: + # we cannot possibly get a pass now, so skip to next + # testsuite item continue # test suites: the test case is only 'passed' if there are jobs for all listed @@ -180,10 +191,15 @@ def _get_passed_tcnames(job, composeid, client=None): if any(_job['result'] not in ('passed', 'softfailed') for _job in _jobs): continue - # we only get here if all the conditions are satisfied - passed.append(testcase) + # we only get here if all the conditions are satisfied, + # now we check result, which is either the overall job + # result that we were passed or 'passed' if a 'modules' + # condition was present and satisfied + if result in ('passed', 'softfailed'): + passed.append(testcase) - else: # isdict - this is the simple list case. + # other side of 'isdict' condition - this is the simple list case. + elif result in ('passed', 'softfailed'): passed = testsuite return passed @@ -199,37 +215,36 @@ def get_passed_testcases(jobs, client=None): """ passed_testcases = set() for job in jobs: - if job['result'] in ('passed', 'softfailed'): - # it's wikitcms' job to take a compose ID and figure out - # what the validation event for it is. - composeid = job['settings']['BUILD'] - if composeid.endswith('EXTRA') or composeid.endswith('NOREPORT'): - # this is a 'dirty' test run with extra parameters - # (usually a test with an updates.img) or some kind - # of throwaway run, we never want to report results - # for these - logger.debug("Job %d is a NOREPORT job or was run with extra params! Will not report", job['id']) + # it's wikitcms' job to take a compose ID and figure out + # what the validation event for it is. + composeid = job['settings']['BUILD'] + if composeid.endswith('EXTRA') or composeid.endswith('NOREPORT'): + # this is a 'dirty' test run with extra parameters + # (usually a test with an updates.img) or some kind + # of throwaway run, we never want to report results + # for these + logger.debug("Job %d is a NOREPORT job or was run with extra params! Will not report", job['id']) + continue + # find the TESTSUITES entry for the job and parse it to + # get a list of passed test case names (TESTCASES keys) + passed = _get_passed_tcnames(job, job['result'], composeid, client) + for testcase in passed: + # skip with warning if testcase is not in TESTCASES + if testcase not in conf_test_suites.TESTCASES: + logger.warning("No TESTCASES entry found for %s!", testcase) continue - # find the TESTSUITES entry for the job and parse it to - # get a list of passed test case names (TESTCASES keys) - passed = _get_passed_tcnames(job, composeid, client) - for testcase in passed: - # skip with warning if testcase is not in TESTCASES - if testcase not in conf_test_suites.TESTCASES: - logger.warning("No TESTCASES entry found for %s!", testcase) - continue - # Each testcase name now in the `passed` list is the name of a dict in the - # TESTCASES dict-of-dicts which more precisely identifies the 'test instance' (when - # there is more than one for a testcase) and environment for which the result - # should be filed. - - # create new dict based on the testcase dict with $FOO$ values replaced - uniqueres = _uniqueres_replacements(job, conf_test_suites.TESTCASES[testcase]) - result = ResTuple( - testtype=uniqueres['type'], testcase=testcase, - section=uniqueres.get('section'), testname=uniqueres.get('name', ''), - env=uniqueres.get('env', ''), status='pass', bot=True, cid=composeid) - passed_testcases.add(result) + # Each testcase name now in the `passed` list is the name of a dict in the + # TESTCASES dict-of-dicts which more precisely identifies the 'test instance' (when + # there is more than one for a testcase) and environment for which the result + # should be filed. + + # create new dict based on the testcase dict with $FOO$ values replaced + uniqueres = _uniqueres_replacements(job, conf_test_suites.TESTCASES[testcase]) + result = ResTuple( + testtype=uniqueres['type'], testcase=testcase, + section=uniqueres.get('section'), testname=uniqueres.get('name', ''), + env=uniqueres.get('env', ''), status='pass', bot=True, cid=composeid) + passed_testcases.add(result) return sorted(list(passed_testcases), key=attrgetter('testcase')) diff --git a/tests/test_report.py b/tests/test_report.py index 5ec7c33..7fefba1 100644 --- a/tests/test_report.py +++ b/tests/test_report.py @@ -98,7 +98,7 @@ class TestGetPassedTcNames: def test_list(self, jobdict01): """Test the simple list case of _get_passed_tcnames function.""" # test is server_realmd_join_kickstart - ret = fosreport._get_passed_tcnames(jobdict01, 'somecompose') + ret = fosreport._get_passed_tcnames(jobdict01, 'passed', 'somecompose') assert sorted(ret) == [ "QA:Testcase_FreeIPA_realmd_login", "QA:Testcase_domain_client_authenticate", @@ -120,7 +120,7 @@ class TestGetPassedTcNames: } ] } - ret = fosreport._get_passed_tcnames(jobdict01, 'somecompose', fakeclient) + ret = fosreport._get_passed_tcnames(jobdict01, 'passed', 'somecompose', fakeclient) assert sorted(ret) == [ "QA:Testcase_desktop_error_checks", "QA:Testcase_desktop_update_notification", @@ -135,7 +135,7 @@ class TestGetPassedTcNames: } ] } - ret = fosreport._get_passed_tcnames(jobdict01, 'somecompose', fakeclient) + ret = fosreport._get_passed_tcnames(jobdict01, 'passed', 'somecompose', fakeclient) assert ret == [] # now try again with the dependent test missing @@ -147,12 +147,16 @@ class TestGetPassedTcNames: } ] } - ret = fosreport._get_passed_tcnames(jobdict01, 'somecompose', fakeclient) + ret = fosreport._get_passed_tcnames(jobdict01, 'passed', 'somecompose', fakeclient) assert ret == [] def test_modules(self, jobdict01): """Test the dict case of _get_passed_tcnames where test modules - must be passed. + must be passed. We test behaviour both with overall job result + 'passed' (where the passed module test case and the test cases + related to the overall job result should be returned) and with + overall job result 'failed' (where only the passed module test + case should be returned). """ jobdict01['test'] = 'realmd_join_cockpit' # say one of the required modules passed, one failed @@ -166,13 +170,20 @@ class TestGetPassedTcNames: 'result': 'failed', }, ] - ret = fosreport._get_passed_tcnames(jobdict01, 'somecompose') + # with overall job result 'passed'... + ret = fosreport._get_passed_tcnames(jobdict01, 'passed', 'somecompose') assert sorted(ret) == [ "QA:Testcase_FreeIPA_realmd_login", "QA:Testcase_FreeIPA_web_ui", "QA:Testcase_domain_client_authenticate", "QA:Testcase_realmd_join_cockpit", ] + # now with overall job result 'failed' + ret = fosreport._get_passed_tcnames(jobdict01, 'failed', 'somecompose') + assert sorted(ret) == [ + # only the case that has a module conditional + "QA:Testcase_FreeIPA_web_ui", + ] def test_module_missing(self, jobdict01): """Test the dict case of _get_passed_tcnames where test modules @@ -183,7 +194,7 @@ class TestGetPassedTcNames: jobdict01['modules'] = [] # shouldn't crash or do anything weird, just not report the # module dependent tests as passed - ret = fosreport._get_passed_tcnames(jobdict01, 'somecompose') + ret = fosreport._get_passed_tcnames(jobdict01, 'passed', 'somecompose') assert sorted(ret) == [ "QA:Testcase_FreeIPA_realmd_login", "QA:Testcase_domain_client_authenticate", @@ -194,7 +205,7 @@ class TestGetPassedTcNames: """Check _get_passed_tcnames returns if testcase name isn't in TESTSUITES. """ - ret = fosreport._get_passed_tcnames({'test': 'nonexistenttest'}, 'somecompose') + ret = fosreport._get_passed_tcnames({'test': 'nonexistenttest'}, 'passed', 'somecompose') assert ret == []