From ff25c999a59a5d8fa82ed118c53b11ed479de9bd Mon Sep 17 00:00:00 2001 From: Gorka Eguileor Date: Wed, 10 Aug 2022 16:57:12 +0200 Subject: [PATCH] Fix #330 - Preserve load-list order When using `--load-list` to run tests the tests were always run in a different order than the one provided in the file, even when the `--random` argument was not present: $ stestr run --serial --load-list failing This patch preserves the order of the tests from the file when `--load-list` is used, and it will only randomize it when `--random` is provided: $ stestr run --serial --random --load-list failing --- stestr/commands/run.py | 27 +++-- stestr/selection.py | 36 +++--- stestr/subunit_runner/program.py | 47 +++++--- stestr/test_processor.py | 6 +- stestr/tests/subunit_runner/__init__.py | 0 stestr/tests/subunit_runner/test_program.py | 124 ++++++++++++++++++++ stestr/tests/test_load.py | 3 + stestr/tests/test_run.py | 53 +++++++++ stestr/tests/test_selection.py | 23 +++- 9 files changed, 266 insertions(+), 53 deletions(-) create mode 100644 stestr/tests/subunit_runner/__init__.py create mode 100644 stestr/tests/subunit_runner/test_program.py diff --git a/stestr/commands/run.py b/stestr/commands/run.py index 9d5e2ac9..2bef465f 100644 --- a/stestr/commands/run.py +++ b/stestr/commands/run.py @@ -276,6 +276,21 @@ 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, test_path=None, top_dir=None, group_regex=None, failing=False, serial=False, concurrency=0, load_list=None, @@ -492,17 +507,7 @@ def run_tests(): else: ids = None if load_list: - 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) + ids = _load_list_and_filter(load_list, ids) if config and os.path.isfile(config): conf = config_file.TestrConf(config) diff --git a/stestr/selection.py b/stestr/selection.py index c90aa014..1fd1a75c 100644 --- a/stestr/selection.py +++ b/stestr/selection.py @@ -11,6 +11,7 @@ # under the License. import contextlib +import random import re import sys @@ -89,7 +90,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): + include_list=None, exclude_regex=None, randomize=False): """Filters the discovered test cases :param list test_ids: The set of test_ids to be filtered @@ -100,10 +101,11 @@ def construct_list(test_ids, regexes=None, exclude_list=None, :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: set + :rtype: list """ if not regexes: @@ -137,20 +139,16 @@ def construct_list(test_ids, regexes=None, exclude_list=None, exclude_data = [record] list_of_test_cases = filter_tests(regexes, test_ids) - 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 + + 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 diff --git a/stestr/subunit_runner/program.py b/stestr/subunit_runner/program.py index c7aef398..ccd578ed 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): - """Remove tests from suite_or_case where their id is not in test_ids. + """Return a test suite with the intersection of suite_or_case and test_ids. :param suite_or_case: A test suite or test case. :param test_ids: Something that supports the __contains__ protocol. @@ -58,23 +58,31 @@ 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. """ - # 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 _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 def iterate_tests(test_suite_or_case): @@ -178,8 +186,9 @@ def __init__(self, module='__main__', defaultTest=None, argv=None, 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 612cccce..a496b9cd 100644 --- a/stestr/test_processor.py +++ b/stestr/test_processor.py @@ -96,7 +96,6 @@ def __init__(self, test_ids, cmd_template, listopt, idoption, 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 @@ -145,11 +144,14 @@ 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) + exclude_regex=self.exclude_regex, + randomize=randomize) name = self.make_listfile() variables['IDFILE'] = name idlist = ' '.join(self.test_ids) diff --git a/stestr/tests/subunit_runner/__init__.py b/stestr/tests/subunit_runner/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/stestr/tests/subunit_runner/test_program.py b/stestr/tests/subunit_runner/test_program.py new file mode 100644 index 00000000..19d268b4 --- /dev/null +++ b/stestr/tests/subunit_runner/test_program.py @@ -0,0 +1,124 @@ +# 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 1d69388c..a243ebed 100644 --- a/stestr/tests/test_load.py +++ b/stestr/tests/test_load.py @@ -13,11 +13,14 @@ 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(in_streams=[('subunit', stream)], pretty_out=True, diff --git a/stestr/tests/test_run.py b/stestr/tests/test_run.py index 0a0304d5..1b9703aa 100644 --- a/stestr/tests/test_run.py +++ b/stestr/tests/test_run.py @@ -11,6 +11,7 @@ # under the License. import io +from unittest import mock from stestr.commands import run from stestr.tests import base @@ -46,3 +47,55 @@ def test_to_int_none(self): '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 24252bd9..254d1c14 100644 --- a/stestr/tests/test_selection.py +++ b/stestr/tests/test_selection.py @@ -72,9 +72,28 @@ 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])'] + 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']) - self.assertEqual(list(result), ['fake_test(scen)[egg,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)) def test_simple_exclusion_re(self): test_lists = ['fake_test(scen)[tag,bar])', 'fake_test(scen)[egg,foo])']