From 0356cef03852647887e44e56457e8669fc76ea84 Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Wed, 22 May 2019 17:38:29 +0200 Subject: [PATCH 1/3] Add test graph validation routine --- reframe/frontend/dependency.py | 42 ++++++++++++++++ unittests/test_policies.py | 88 ++++++++++++++++++++++++++++++++++ 2 files changed, 130 insertions(+) diff --git a/reframe/frontend/dependency.py b/reframe/frontend/dependency.py index 674927cebc..3338eb6864 100644 --- a/reframe/frontend/dependency.py +++ b/reframe/frontend/dependency.py @@ -3,6 +3,7 @@ # import collections +import itertools import reframe as rfm from reframe.core.exceptions import DependencyError @@ -79,3 +80,44 @@ def print_deps(graph): def validate_deps(graph): """Validate dependency graph.""" + + # Reduce test case graph to a test name only graph; this disallows + # pseudo-dependencies as follows: + # + # (t0, e1) -> (t1, e1) + # (t1, e0) -> (t0, e0) + # + # This reduction step will result in a graph description with duplicate + # entries in the adjacency list; this is not a problem, cos they will be + # filtered out during the DFS traversal below. + test_graph = {} + for case, deps in graph.items(): + test_deps = [d.check.name for d in deps] + try: + test_graph[case.check.name] += test_deps + except KeyError: + test_graph[case.check.name] = test_deps + + # Check for cyclic dependencies in the test name graph + visited = set() + unvisited = list( + itertools.zip_longest(test_graph.keys(), [], fillvalue=None) + ) + path = [] + while unvisited: + node, parent = unvisited.pop() + while path and path[-1] != parent: + path.pop() + + adjacent = reversed(test_graph[node]) + path.append(node) + for n in adjacent: + if n in path: + cycle_str = '->'.join(path + [n]) + raise DependencyError( + 'found cyclic dependency between tests: ' + cycle_str) + + if n not in visited: + unvisited.append((n, node)) + + visited.add(node) diff --git a/unittests/test_policies.py b/unittests/test_policies.py index c765c1062e..d59bb70a19 100644 --- a/unittests/test_policies.py +++ b/unittests/test_policies.py @@ -489,6 +489,7 @@ def test_build_deps(self): # Build dependencies and continue testing deps = dependency.build_deps(cases) + dependency.validate_deps(deps) # Check DEPEND_FULLY dependencies assert num_deps(deps, 'Test1_fully') == 8 @@ -587,3 +588,90 @@ def test_build_deps_unknown_source_env(self): # is not executed for eX deps = dependency.build_deps(executors.generate_testcases(checks)) assert num_deps(deps, 'Test1_default') == 4 + + def create_test(self, name): + test = rfm.RegressionTest() + test.name = name + test.valid_systems = ['*'] + test.valid_prog_environs = ['*'] + test.executable = 'echo' + test.executable_opts = [name] + return test + + @rt.switch_runtime(fixtures.TEST_SITE_CONFIG, 'sys0') + def test_valid_deps(self): + # + # t0 + # ^ + # | + # +-->t1<--+ + # | | + # t2<------t3 + # ^ ^ + # | | + # +---t4---+ + # + t0 = self.create_test('t0') + t1 = self.create_test('t1') + t2 = self.create_test('t2') + t3 = self.create_test('t3') + t4 = self.create_test('t4') + t1.depends_on('t0') + t2.depends_on('t1') + t3.depends_on('t1') + t3.depends_on('t2') + t4.depends_on('t2') + t4.depends_on('t3') + dependency.validate_deps( + dependency.build_deps( + executors.generate_testcases([t0, t1, t2, t3, t4]) + ) + ) + + @rt.switch_runtime(fixtures.TEST_SITE_CONFIG, 'sys0') + def test_cyclic_deps(self): + # + # t0 + # ^ + # | + # +-->t1<--+ + # | | | + # t2 | t3 + # ^ | ^ + # | v | + # +---t4---+ + # + t0 = self.create_test('t0') + t1 = self.create_test('t1') + t2 = self.create_test('t2') + t3 = self.create_test('t3') + t4 = self.create_test('t4') + t1.depends_on('t0') + t1.depends_on('t4') + t2.depends_on('t1') + t3.depends_on('t1') + t3.depends_on('t2') + t4.depends_on('t2') + t4.depends_on('t3') + deps = dependency.build_deps( + executors.generate_testcases([t0, t1, t2, t3, t4]) + ) + + with pytest.raises(DependencyError) as exc_info: + dependency.validate_deps(deps) + + assert 't4->t2->t1->t4' in str(exc_info.value) + + @rt.switch_runtime(fixtures.TEST_SITE_CONFIG, 'sys0') + def test_cyclic_deps_by_env(self): + t0 = self.create_test('t0') + t1 = self.create_test('t1') + t1.depends_on('t0', rfm.DEPEND_EXACT, {'e0': ['e0']}) + t0.depends_on('t1', rfm.DEPEND_EXACT, {'e1': ['e1']}) + deps = dependency.build_deps( + executors.generate_testcases([t0, t1]) + ) + with pytest.raises(DependencyError) as exc_info: + dependency.validate_deps(deps) + + assert 't1->t0->t1' in str(exc_info.value) From 97bce7747c2f049174c2d4e3066b4d6ad3b88d4c Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Thu, 23 May 2019 10:53:31 +0200 Subject: [PATCH 2/3] Fix unit tests failure with Python 3.5 --- unittests/test_policies.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/unittests/test_policies.py b/unittests/test_policies.py index d59bb70a19..3757df3a82 100644 --- a/unittests/test_policies.py +++ b/unittests/test_policies.py @@ -660,7 +660,12 @@ def test_cyclic_deps(self): with pytest.raises(DependencyError) as exc_info: dependency.validate_deps(deps) - assert 't4->t2->t1->t4' in str(exc_info.value) + assert ('t4->t2->t1->t4' in str(exc_info.value) or + 't2->t1->t4->t2' in str(exc_info.value) or + 't1->t4->t2->t1' in str(exc_info.value) or + 't1->t4->t3->t1' in str(exc_info.value) or + 't4->t3->t1->t4' in str(exc_info.value) or + 't3->t1->t4->t3' in str(exc_info.value)) @rt.switch_runtime(fixtures.TEST_SITE_CONFIG, 'sys0') def test_cyclic_deps_by_env(self): @@ -674,4 +679,5 @@ def test_cyclic_deps_by_env(self): with pytest.raises(DependencyError) as exc_info: dependency.validate_deps(deps) - assert 't1->t0->t1' in str(exc_info.value) + assert ('t1->t0->t1' in str(exc_info.value) or + 't0->t1->t0' in str(exc_info.value)) From 91855bdc8c0dcb933821be0fdb356cc43ffce585 Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Sat, 25 May 2019 16:32:00 +0200 Subject: [PATCH 3/3] Fix redundant path traversal in test graph - We are now implemeting DFS for all possible sources that has not yet been visited in a previous search. - A couple of unit tests were also added. --- reframe/frontend/dependency.py | 45 +++++++++++++------------ unittests/test_policies.py | 60 ++++++++++++++++++++++++---------- 2 files changed, 67 insertions(+), 38 deletions(-) diff --git a/reframe/frontend/dependency.py b/reframe/frontend/dependency.py index 3338eb6864..4f11bf4ae6 100644 --- a/reframe/frontend/dependency.py +++ b/reframe/frontend/dependency.py @@ -100,24 +100,29 @@ def validate_deps(graph): # Check for cyclic dependencies in the test name graph visited = set() - unvisited = list( - itertools.zip_longest(test_graph.keys(), [], fillvalue=None) - ) + sources = set(test_graph.keys()) path = [] - while unvisited: - node, parent = unvisited.pop() - while path and path[-1] != parent: - path.pop() - - adjacent = reversed(test_graph[node]) - path.append(node) - for n in adjacent: - if n in path: - cycle_str = '->'.join(path + [n]) - raise DependencyError( - 'found cyclic dependency between tests: ' + cycle_str) - - if n not in visited: - unvisited.append((n, node)) - - visited.add(node) + + # Since graph may comprise multiple not connected subgraphs, we search for + # cycles starting from all possible sources + while sources: + unvisited = [(sources.pop(), None)] + while unvisited: + node, parent = unvisited.pop() + while path and path[-1] != parent: + path.pop() + + adjacent = reversed(test_graph[node]) + path.append(node) + for n in adjacent: + if n in path: + cycle_str = '->'.join(path + [n]) + raise DependencyError( + 'found cyclic dependency between tests: ' + cycle_str) + + if n not in visited: + unvisited.append((n, node)) + + visited.add(node) + + sources -= visited diff --git a/unittests/test_policies.py b/unittests/test_policies.py index 3757df3a82..903a606cdd 100644 --- a/unittests/test_policies.py +++ b/unittests/test_policies.py @@ -589,6 +589,10 @@ def test_build_deps_unknown_source_env(self): deps = dependency.build_deps(executors.generate_testcases(checks)) assert num_deps(deps, 'Test1_default') == 4 + @rt.switch_runtime(fixtures.TEST_SITE_CONFIG, 'sys0') + def test_build_deps_empty(self): + assert {} == dependency.build_deps([]) + def create_test(self, name): test = rfm.RegressionTest() test.name = name @@ -601,14 +605,14 @@ def create_test(self, name): @rt.switch_runtime(fixtures.TEST_SITE_CONFIG, 'sys0') def test_valid_deps(self): # - # t0 - # ^ - # | - # +-->t1<--+ - # | | - # t2<------t3 - # ^ ^ - # | | + # t0 +-->t5<--+ + # ^ | | + # | | | + # +-->t1<--+ t6 t7 + # | | ^ + # t2<------t3 | + # ^ ^ | + # | | t8 # +---t4---+ # t0 = self.create_test('t0') @@ -616,29 +620,37 @@ def test_valid_deps(self): t2 = self.create_test('t2') t3 = self.create_test('t3') t4 = self.create_test('t4') + t5 = self.create_test('t5') + t6 = self.create_test('t6') + t7 = self.create_test('t7') + t8 = self.create_test('t8') t1.depends_on('t0') t2.depends_on('t1') t3.depends_on('t1') t3.depends_on('t2') t4.depends_on('t2') t4.depends_on('t3') + t6.depends_on('t5') + t7.depends_on('t5') + t8.depends_on('t7') dependency.validate_deps( dependency.build_deps( - executors.generate_testcases([t0, t1, t2, t3, t4]) + executors.generate_testcases([t0, t1, t2, t3, t4, + t5, t6, t7, t8]) ) ) @rt.switch_runtime(fixtures.TEST_SITE_CONFIG, 'sys0') def test_cyclic_deps(self): # - # t0 - # ^ - # | - # +-->t1<--+ - # | | | - # t2 | t3 - # ^ | ^ - # | v | + # t0 +-->t5<--+ + # ^ | | + # | | | + # +-->t1<--+ t6 t7 + # | | | ^ + # t2 | t3 | + # ^ | ^ | + # | v | t8 # +---t4---+ # t0 = self.create_test('t0') @@ -646,6 +658,10 @@ def test_cyclic_deps(self): t2 = self.create_test('t2') t3 = self.create_test('t3') t4 = self.create_test('t4') + t5 = self.create_test('t5') + t6 = self.create_test('t6') + t7 = self.create_test('t7') + t8 = self.create_test('t8') t1.depends_on('t0') t1.depends_on('t4') t2.depends_on('t1') @@ -653,8 +669,12 @@ def test_cyclic_deps(self): t3.depends_on('t2') t4.depends_on('t2') t4.depends_on('t3') + t6.depends_on('t5') + t7.depends_on('t5') + t8.depends_on('t7') deps = dependency.build_deps( - executors.generate_testcases([t0, t1, t2, t3, t4]) + executors.generate_testcases([t0, t1, t2, t3, t4, + t5, t6, t7, t8]) ) with pytest.raises(DependencyError) as exc_info: @@ -681,3 +701,7 @@ def test_cyclic_deps_by_env(self): assert ('t1->t0->t1' in str(exc_info.value) or 't0->t1->t0' in str(exc_info.value)) + + @rt.switch_runtime(fixtures.TEST_SITE_CONFIG, 'sys0') + def test_validate_deps_empty(self): + dependency.validate_deps({})