diff --git a/changes.d/6772.fix.md b/changes.d/6772.fix.md new file mode 100644 index 00000000000..38bcf33347a --- /dev/null +++ b/changes.d/6772.fix.md @@ -0,0 +1 @@ +Xtriggers with an OR operator (`|`) between them now correctly fail validation as Cylc does not support this. diff --git a/cylc/flow/config.py b/cylc/flow/config.py index fbd996350fc..e9cc09c6d8f 100644 --- a/cylc/flow/config.py +++ b/cylc/flow/config.py @@ -1841,6 +1841,7 @@ def generate_edges(self, lexpr, orig_lexpr, left_nodes, right, seq, # Right is a lone node. self.edges[seq].add((right, None, suicide, conditional)) + conditional_xtriggers = [] for left in left_nodes: # if left is None: # continue @@ -1856,7 +1857,16 @@ def generate_edges(self, lexpr, orig_lexpr, left_nodes, right, seq, LOG.error(f"{orig_lexpr} => {right}") raise WorkflowConfigError( f"self-edge detected: {left} => {right}") - self.edges[seq].add((left, right, suicide, conditional)) + if left.startswith('@') and conditional: + conditional_xtriggers.append(f'{lexpr} => {right}') + else: + self.edges[seq].add((left, right, suicide, conditional)) + if conditional_xtriggers: + raise WorkflowConfigError( + 'Xtriggers cannot be used in conditional graph' + ' expressions:\n' + + '\n * '.join(conditional_xtriggers) + ) def generate_taskdef(self, orig_expr, node): """Generate task definition for node.""" diff --git a/cylc/flow/prerequisite.py b/cylc/flow/prerequisite.py index 1462f9e9666..d0c70767fe2 100644 --- a/cylc/flow/prerequisite.py +++ b/cylc/flow/prerequisite.py @@ -253,12 +253,8 @@ def _eval_satisfied(self) -> bool: # * the expression is constructed internally # * https://github.com/cylc/cylc-flow/issues/4403 except (SyntaxError, ValueError) as exc: - err_msg = str(exc) - if str(exc).find("unexpected EOF") != -1: - err_msg += ( - " (could be unmatched parentheses in the graph string?)") raise TriggerExpressionError( - '"%s":\n%s' % (self.get_raw_conditional_expression(), err_msg) + f'"{self.get_raw_conditional_expression}":\n{str(exc)}' ) from None return res diff --git a/cylc/flow/scripts/validate.py b/cylc/flow/scripts/validate.py index edabd6bdab2..aae4238760a 100755 --- a/cylc/flow/scripts/validate.py +++ b/cylc/flow/scripts/validate.py @@ -196,18 +196,7 @@ async def run( # force trigger evaluation now try: itask.state.prerequisites_eval_all() - except TriggerExpressionError as exc: - err = str(exc) - if '@' in err: - print( - f"ERROR, {name}: xtriggers can't be in conditional" - f" expressions: {err}", - file=sys.stderr, - ) - else: - print( - 'ERROR, %s: bad trigger: %s' % (name, err), file=sys.stderr - ) + except TriggerExpressionError: raise WorkflowConfigError("ERROR: bad trigger") from None except Exception as exc: print(str(exc), file=sys.stderr) diff --git a/tests/integration/test_config.py b/tests/integration/test_config.py index 1a2023a93f4..a29927df2b9 100644 --- a/tests/integration/test_config.py +++ b/tests/integration/test_config.py @@ -34,6 +34,7 @@ from cylc.flow.pathutil import get_workflow_run_pub_db_path Fixture = Any +param = pytest.param @pytest.mark.parametrize( @@ -488,6 +489,44 @@ def test_xtrig_signature_validation( validate(id_) +@pytest.mark.parametrize( + 'left', + ( + param('@xrandom | @echo', id='xtrig-or-xtrig'), + param('@xrandom | task', id='xtrig-or-task'), + param('task | @echo', id='task-or-xtrig'), + param('@xrandom | foo & bar', id='xtrig-or-complex'), + param('@xrandom & bar | foo', id='complex-or-xtrig'), + ) +) +def test_xtrig_or_fails_validation( + flow: "Fixture", + validate: "Fixture", + left: str +): + """Xtriggers cannot be chained with the 'or' + + https://github.com/cylc/cylc-flow/issues/6771 + https://github.com/cylc/cylc-flow/issues/2712 + """ + id_ = flow( + { + "scheduling": { + "initial cycle point": "2024", + "xtriggers": { + "xrandom": "xrandom(100)", + "echo": "echo(succeed=True)" + }, + "graph": {"R1": f"{left} => fin"}, + } + } + ) + expected_msg = ( + "Xtriggers cannot be used in conditional graph expressions:\n") + with pytest.raises(WorkflowConfigError, match=expected_msg): + validate(id_) + + def test_special_task_non_word_names(flow: Fixture, validate: Fixture): """Test validation of special tasks names with non-word characters""" wid = flow({ diff --git a/tests/unit/test_prerequisite.py b/tests/unit/test_prerequisite.py index 0f28150a105..2a0509b2cfb 100644 --- a/tests/unit/test_prerequisite.py +++ b/tests/unit/test_prerequisite.py @@ -21,6 +21,7 @@ from cylc.flow.cycling.integer import IntegerPoint from cylc.flow.cycling.loader import ISO8601_CYCLING_TYPE, get_point +from cylc.flow.exceptions import TriggerExpressionError from cylc.flow.id import Tokens, detokenise from cylc.flow.prerequisite import Prerequisite, SatisfiedState from cylc.flow.run_modes import RunMode @@ -259,3 +260,19 @@ def test_satisfy_me__override_truthy( prereq.satisfy_me([Tokens('//1/a:x')], forced=forced, mode=mode) assert prereq[('1', 'a', 'x')] == existing + + +@pytest.mark.parametrize( + 'expr, err', ( + ('int("df")', 'invalid literal'), + ('7 +', 'invalid syntax'), + )) +def test__eval_satisfied_raises(expr, err, monkeypatch): + monkeypatch.setattr( + 'cylc.flow.prerequisite.Prerequisite.get_raw_conditional_expression', + lambda _: expr + ) + prereq = Prerequisite(IntegerPoint('1')) + prereq.conditional_expression = expr + with pytest.raises(TriggerExpressionError, match=err): + prereq._eval_satisfied()