From 4d2a6cc87a3fe6924c3d84ab02f1aeb23b59abb1 Mon Sep 17 00:00:00 2001 From: Matthew Treinish Date: Thu, 22 Sep 2022 16:01:11 -0400 Subject: [PATCH] Revert "Fix #330 - Preserve load-list order" Several issues have been reported about the run order and other odd behavior in running of tests since the 4.0.0 release including a high degree of random failures in stestr CI. #331 is the only likely candidate as to the root cause. In the interest of fixing these issues for people this commit reverts #331. This revert was already released as part of 4.0.1. Since the main branch adopted `black` code formatting recently it was easier to do the straight revert on the stable branch first as there was conflicts. This reverts commit ff25c999a59a5d8fa82ed118c53b11ed479de9bd. --- stestr/commands/run.py | 26 ++-- stestr/selection.py | 40 +++--- stestr/subunit_runner/program.py | 48 +++---- stestr/test_processor.py | 4 +- stestr/tests/subunit_runner/__init__.py | 0 stestr/tests/subunit_runner/test_program.py | 140 -------------------- stestr/tests/test_load.py | 3 - stestr/tests/test_run.py | 52 -------- stestr/tests/test_selection.py | 32 +---- 9 files changed, 52 insertions(+), 293 deletions(-) delete mode 100644 stestr/tests/subunit_runner/__init__.py delete mode 100644 stestr/tests/subunit_runner/test_program.py diff --git a/stestr/commands/run.py b/stestr/commands/run.py index 5d9a948e..4cfac09d 100644 --- a/stestr/commands/run.py +++ b/stestr/commands/run.py @@ -367,20 +367,6 @@ def gather_errors(test_dict): return ids -def _load_list_and_filter(list_filename, filter_ids): - """Load list of IDs and optionally filter them by a list of ids.""" - list_ids = [] - set_ids = set() - # Should perhaps be text.. currently does its own decode. - with open(list_filename, "rb") as list_file: - # Remove duplicates and filter out non failing tests when required - for test_id in parse_list(list_file.read()): - if test_id not in set_ids and (filter_ids is None or test_id in filter_ids): - list_ids.append(test_id) - set_ids.add(test_id) - return list_ids - - def run_command( config=".stestr.conf", repo_url=None, @@ -646,7 +632,17 @@ def run_tests(): else: ids = None if load_list: - ids = _load_list_and_filter(load_list, ids) + list_ids = set() + # Should perhaps be text.. currently does its own decode. + with open(load_list, "rb") as list_file: + list_ids = set(parse_list(list_file.read())) + if ids is None: + # Use the supplied list verbatim + ids = list_ids + else: + # We have some already limited set of ids, just reduce to ids + # that are both failing and listed. + ids = list_ids.intersection(ids) if config and os.path.isfile(config): conf = config_file.TestrConf(config) diff --git a/stestr/selection.py b/stestr/selection.py index e53470fe..9d3cf284 100644 --- a/stestr/selection.py +++ b/stestr/selection.py @@ -11,7 +11,6 @@ # under the License. import contextlib -import random import re import sys @@ -93,12 +92,7 @@ def _get_regex_from_include_list(file_path): def construct_list( - test_ids, - regexes=None, - exclude_list=None, - include_list=None, - exclude_regex=None, - randomize=False, + test_ids, regexes=None, exclude_list=None, include_list=None, exclude_regex=None ): """Filters the discovered test cases @@ -110,11 +104,10 @@ def construct_list( :param str exclude_list: The path to an exclusion_list file :param str include_list: The path to an inclusion_list file :param str exclude_regex: regex pattern to exclude tests - :param str randomize: Randomize the result :return: iterable of strings. The strings are full test_ids - :rtype: list + :rtype: set """ if not regexes: @@ -150,15 +143,20 @@ def construct_list( exclude_data = [record] list_of_test_cases = filter_tests(regexes, test_ids) - - if exclude_data: - # NOTE(afazekas): We might use a faster logic when the - # print option is not requested - for (rex, msg, s_list) in exclude_data: - # NOTE(mtreinish): In the case of overlapping regex the test case - # might have already been removed from the set of tests - list_of_test_cases = [tc for tc in list_of_test_cases if not rex.search(tc)] - if randomize: - random.shuffle(list_of_test_cases) - - return list_of_test_cases + set_of_test_cases = set(list_of_test_cases) + + if not exclude_data: + return set_of_test_cases + + # NOTE(afazekas): We might use a faster logic when the + # print option is not requested + for (rex, msg, s_list) in exclude_data: + for test_case in list_of_test_cases: + if rex.search(test_case): + # NOTE(mtreinish): In the case of overlapping regex the test + # case might have already been removed from the set of tests + if test_case in set_of_test_cases: + set_of_test_cases.remove(test_case) + s_list.append(test_case) + + return set_of_test_cases diff --git a/stestr/subunit_runner/program.py b/stestr/subunit_runner/program.py index 622e3f78..67e42380 100644 --- a/stestr/subunit_runner/program.py +++ b/stestr/subunit_runner/program.py @@ -22,7 +22,7 @@ def filter_by_ids(suite_or_case, test_ids): - """Return a test suite with the intersection of suite_or_case and test_ids. + """Remove tests from suite_or_case where their id is not in test_ids. :param suite_or_case: A test suite or test case. :param test_ids: Something that supports the __contains__ protocol. @@ -58,32 +58,23 @@ def filter_by_ids(suite_or_case, test_ids): thus the backwards-compatible code paths attempt to mutate in place rather than guessing how to reconstruct a new suite. """ - - def _filter_tests(to_filter, id_map): - # Compatible objects - if extras.safe_hasattr(to_filter, "filter_by_ids"): - res = to_filter.filter_by_ids(test_ids) - _filter_tests(res, id_map) - # TestCase objects. - elif extras.safe_hasattr(to_filter, "id"): - test_id = to_filter.id() - if test_id in test_ids: - id_map[test_id] = to_filter - # Standard TestSuites or derived classes. - elif isinstance(to_filter, unittest.TestSuite): - for item in to_filter: - _filter_tests(item, id_map) - - id_map = {} - _filter_tests(suite_or_case, id_map) - - result = unittest.TestSuite() - for test_id in test_ids: - if test_id in id_map: - # Use pop to avoid duplicates - result.addTest(id_map.pop(test_id)) - - return result + # Compatible objects + if extras.safe_hasattr(suite_or_case, "filter_by_ids"): + return suite_or_case.filter_by_ids(test_ids) + # TestCase objects. + if extras.safe_hasattr(suite_or_case, "id"): + if suite_or_case.id() in test_ids: + return suite_or_case + else: + return unittest.TestSuite() + # Standard TestSuites or derived classes [assumed to be mutable]. + if isinstance(suite_or_case, unittest.TestSuite): + filtered = [] + for item in suite_or_case: + filtered.append(filter_by_ids(item, test_ids)) + suite_or_case._tests[:] = filtered + # Everything else: + return suite_or_case def iterate_tests(test_suite_or_case): @@ -198,9 +189,8 @@ def __init__( lines = source.readlines() finally: source.close() - test_ids = [line.strip().decode("utf-8") for line in lines] + test_ids = {line.strip().decode("utf-8") for line in lines} self.test = filter_by_ids(self.test, test_ids) - # XXX: Local edit (see http://bugs.python.org/issue22860) if not self.listtests: self.runTests() diff --git a/stestr/test_processor.py b/stestr/test_processor.py index 147bf8be..f52596a3 100644 --- a/stestr/test_processor.py +++ b/stestr/test_processor.py @@ -108,6 +108,7 @@ def __init__( self._listpath = listpath self.test_filters = test_filters self._group_callback = group_callback + self.worker_path = None self.worker_path = worker_path self.concurrency_value = concurrency self.exclude_list = exclude_list @@ -158,15 +159,12 @@ def list_subst(match): name = "" idlist = "" else: - # Randomize now if it's not going to be partitioned - randomize = self.randomize and self.concurrency == 1 self.test_ids = selection.construct_list( self.test_ids, exclude_list=self.exclude_list, include_list=self.include_list, regexes=self.test_filters, exclude_regex=self.exclude_regex, - randomize=randomize, ) name = self.make_listfile() variables["IDFILE"] = name diff --git a/stestr/tests/subunit_runner/__init__.py b/stestr/tests/subunit_runner/__init__.py deleted file mode 100644 index e69de29b..00000000 diff --git a/stestr/tests/subunit_runner/test_program.py b/stestr/tests/subunit_runner/test_program.py deleted file mode 100644 index dfa2e8ce..00000000 --- a/stestr/tests/subunit_runner/test_program.py +++ /dev/null @@ -1,140 +0,0 @@ -# Licensed under the Apache License, Version 2.0 (the "License"); you may -# not use this file except in compliance with the License. You may obtain -# a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT -# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the -# License for the specific language governing permissions and limitations -# under the License. - -import unittest - -from stestr.subunit_runner import program -from stestr.tests import base - - -class TestGetById(base.TestCase): - class CompatibleSuite(unittest.TestSuite): - def filter_by_ids(self, test_ids): - return unittest.TestSuite([item for item in self if item.id() in test_ids]) - - @classmethod - def setUpClass(cls): - cls.test_suite = unittest.TestSuite( - [cls(name) for name in dir(cls) if name.startswith("test")] - ) - - @classmethod - def _test_name(cls, name): - return f"{__name__}.{cls.__name__}.{name}" - - @staticmethod - def _test_ids(suite): - return [item.id() for item in suite] - - def test_filter_by_ids_case_no_tests(self): - test_case = type(self)("test_filter_by_ids_case_no_tests") - result = program.filter_by_ids( - test_case, - ["stestr.tests.test_load.TestLoadCommand.test_empty_with_pretty_out"], - ) - self.assertEqual([], self._test_ids(result)) - - def test_filter_by_ids_case_simple(self): - name = "test_filter_by_ids_case_simple" - test_name = self._test_name(name) - test_case = type(self)(name) - result = program.filter_by_ids(test_case, [test_name]) - self.assertEqual([test_name], self._test_ids(result)) - - def test_filter_by_ids_suite_no_tests(self): - result = program.filter_by_ids( - self.test_suite, - ["stestr.tests.test_load.TestLoadCommand.test_empty_with_pretty_out"], - ) - self.assertEqual([], self._test_ids(result)) - - def test_filter_by_ids_suite_simple(self): - test_name = self._test_name("test_filter_by_ids_suite_simple") - result = program.filter_by_ids(self.test_suite, [test_name]) - self.assertEqual([test_name], self._test_ids(result)) - - def test_filter_by_ids_suite_preserves_order(self): - names = ( - "test_filter_by_ids_suite_simple", - "test_filter_by_ids_suite_preserves_order", - "test_filter_by_ids_suite_no_tests", - ) - test_names = [self._test_name(name) for name in names] - result = program.filter_by_ids(self.test_suite, test_names) - self.assertEqual(test_names, self._test_ids(result)) - - def test_filter_by_ids_suite_no_duplicates(self): - names = ( - "test_filter_by_ids_suite_simple", - "test_filter_by_ids_suite_preserves_order", - ) - expected = [self._test_name(name) for name in names] - # Create duplicates in reversed order - test_names = expected + expected[::-1] - result = program.filter_by_ids(self.test_suite, test_names) - self.assertEqual(expected, self._test_ids(result)) - - def test_filter_by_ids_compatible_no_tests(self): - test_suite = self.CompatibleSuite(self.test_suite) - result = program.filter_by_ids( - test_suite, - ["stestr.tests.test_load.TestLoadCommand.test_empty_with_pretty_out"], - ) - self.assertEqual([], self._test_ids(result)) - - def test_filter_by_ids_compatible_simple(self): - test_suite = self.CompatibleSuite(self.test_suite) - test_name = self._test_name("test_filter_by_ids_compatible_simple") - result = program.filter_by_ids(test_suite, [test_name]) - self.assertEqual([test_name], self._test_ids(result)) - - def test_filter_by_ids_compatible_preserves_order(self): - test_suite = self.CompatibleSuite(self.test_suite) - names = ( - "test_filter_by_ids_compatible_simple", - "test_filter_by_ids_compatible_preserves_order", - "test_filter_by_ids_compatible_no_tests", - ) - test_names = [self._test_name(name) for name in names] - result = program.filter_by_ids(test_suite, test_names) - self.assertEqual(test_names, self._test_ids(result)) - - def test_filter_by_ids_compatible_no_duplicates(self): - test_suite = self.CompatibleSuite(self.test_suite) - names = ( - "test_filter_by_ids_compatible_simple", - "test_filter_by_ids_compatible_preserves_order", - ) - expected = [self._test_name(name) for name in names] - # Create duplicates in reversed order - test_names = expected + expected[::-1] - result = program.filter_by_ids(test_suite, test_names) - self.assertEqual(expected, self._test_ids(result)) - - def test_filter_by_ids_no_duplicates_preserve_order(self): - test_suite = unittest.TestSuite( - [ - self.CompatibleSuite(self.test_suite), - self.test_suite, - type(self)("test_filter_by_ids_no_duplicates_preserve_order"), - ] - ) - names = ( - "test_filter_by_ids_compatible_simple", - "test_filter_by_ids_suite_simple", - "test_filter_by_ids_compatible_preserves_order", - ) - expected = [self._test_name(name) for name in names] - # Create duplicates in reversed order - test_names = expected + expected[::-1] - result = program.filter_by_ids(test_suite, test_names) - self.assertEqual(expected, self._test_ids(result)) diff --git a/stestr/tests/test_load.py b/stestr/tests/test_load.py index 62b12e50..f9943e14 100644 --- a/stestr/tests/test_load.py +++ b/stestr/tests/test_load.py @@ -13,14 +13,11 @@ import io from stestr.commands import load -from stestr import subunit_trace from stestr.tests import base class TestLoadCommand(base.TestCase): def test_empty_with_pretty_out(self): - # Clear results that may be there - subunit_trace.RESULTS.clear() stream = io.BytesIO() output = io.BytesIO() res = load.load( diff --git a/stestr/tests/test_run.py b/stestr/tests/test_run.py index 2a900c32..2995328d 100644 --- a/stestr/tests/test_run.py +++ b/stestr/tests/test_run.py @@ -11,7 +11,6 @@ # under the License. import io -from unittest import mock from stestr.commands import run from stestr.tests import base @@ -43,54 +42,3 @@ def test_to_int_none(self): expected = 'Unable to convert "None" to an integer. ' "Using 0.\n" self.assertEqual(fake_stderr.getvalue(), expected) self.assertEqual(0, out) - - -class TestLoadListAndFilter(base.TestCase): - @mock.patch("builtins.open", side_effect=FileNotFoundError) - def test__load_list_and_filter_file_not_found(self, mock_open): - filename = "my_filename" - self.assertRaises(FileNotFoundError, run._load_list_and_filter, filename, None) - - mock_open.assert_called_once_with(filename, "rb") - mock_open.return_value.__enter__.return_value.read.assert_not_called() - - @mock.patch("builtins.open") - def test__load_list_and_filter_empty_file(self, mock_open): - mock_read = mock_open.return_value.__enter__.return_value.read - mock_read.return_value = b"" - filename = "my_filename" - res = run._load_list_and_filter(filename, ["mytest"]) - self.assertListEqual([], res) - mock_open.assert_called_once_with(filename, "rb") - mock_read.assert_called_once_with() - - @mock.patch("builtins.open") - def test__load_list_and_filter_no_filter(self, mock_open): - mock_read = mock_open.return_value.__enter__.return_value.read - tests = ["test1", "tests2", "test3"] - mock_read.return_value = "\n".join(tests).encode() - filename = "my_filename" - - res = run._load_list_and_filter(filename, None) - - self.assertListEqual(tests, res) - mock_open.assert_called_once_with(filename, "rb") - mock_read.assert_called_once_with() - - @mock.patch("builtins.open") - def test__load_list_and_filter_filter(self, mock_open): - mock_read = mock_open.return_value.__enter__.return_value.read - tests = ["test1", "tests2", "test3"] - mock_read.return_value = "\n".join(tests).encode() - filename = "my_filename" - - # Failed tests in different order from list and with additional ids to - # confirm that order from list is preserved and only elements from list - # are returned - failed_tests = ["test4", "test3", "test1"] - expected = ["test1", "test3"] - res = run._load_list_and_filter(filename, failed_tests) - - self.assertListEqual(expected, res) - mock_open.assert_called_once_with(filename, "rb") - mock_read.assert_called_once_with() diff --git a/stestr/tests/test_selection.py b/stestr/tests/test_selection.py index f491313b..7fa9e562 100644 --- a/stestr/tests/test_selection.py +++ b/stestr/tests/test_selection.py @@ -72,37 +72,9 @@ def test_invalid_regex(self): class TestConstructList(base.TestCase): def test_simple_re(self): - test_lists = [ - "fake_test(scen)[tag,bar])", - "fake_test(scen)[egg,foo])", - "fake_test(necs)[tag,bar])", - "fake_test(necs)[egg,foo])", - "fake_test(nnnn)[foo,bar])", - "fake_test(nnnn)[foo,foo])", - ] + test_lists = ["fake_test(scen)[tag,bar])", "fake_test(scen)[egg,foo])"] result = selection.construct_list(test_lists, regexes=["foo"]) - # Order must be preserved - expected = test_lists[:] - del expected[2] - del expected[0] - self.assertEqual(expected, result) - - def test_simple_re_randomized(self): - test_lists = [ - "fake_test(scen)[tag,bar])", - "fake_test(scen)[egg,foo])", - "fake_test(necs)[tag,bar])", - "fake_test(necs)[egg,foo])", - "fake_test(nnnn)[foo,bar])", - "fake_test(nnnn)[foo,foo])", - ] - result = selection.construct_list(test_lists, regexes=["foo"], randomize=True) - expected_names = test_lists[:] - del expected_names[2] - del expected_names[0] - # Order is randomized - self.assertNotEqual(expected_names, result) - self.assertEqual(set(expected_names), set(result)) + self.assertEqual(list(result), ["fake_test(scen)[egg,foo])"]) def test_simple_exclusion_re(self): test_lists = ["fake_test(scen)[tag,bar])", "fake_test(scen)[egg,foo])"]